Continue OnDownloadUpdated notifications after navigation (issue #1833)

libcef/browser/browser_host_impl.cc

@@ -1290,6 +1290,7 @@ void CefBrowserHostImpl::DestroyBrowser() {
   if (menu_manager_.get())
     menu_manager_->Destroy();
 
+  FOR_EACH_OBSERVER(Observer, observers_, OnBrowserDestroyed(this));
   platform_delegate_->BrowserDestroyed(this);
 
   while (!queued_messages_.empty()) {
@@ -2496,6 +2497,21 @@ bool CefBrowserHostImpl::Send(IPC::Message* message) {
   }
 }
 
+void CefBrowserHostImpl::AddObserver(Observer* observer) {
+  CEF_REQUIRE_UIT();
+  observers_.AddObserver(observer);
+}
+
+void CefBrowserHostImpl::RemoveObserver(Observer* observer) {
+  CEF_REQUIRE_UIT();
+  observers_.RemoveObserver(observer);
+}
+
+bool CefBrowserHostImpl::HasObserver(Observer* observer) const {
+  CEF_REQUIRE_UIT();
+  return observers_.HasObserver(observer);
+}
+
 
 // content::WebContentsObserver::OnMessageReceived() message handlers.
 // -----------------------------------------------------------------------------

libcef/browser/browser_host_impl.h

@@ -24,6 +24,7 @@
 #include "libcef/common/response_manager.h"
 
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
 #include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
 #include "content/public/browser/notification_observer.h"
@@ -71,6 +72,18 @@ class CefBrowserHostImpl : public CefBrowserHost,
      virtual void OnResponse(const std::string& response) =0;
   };
 
+  // Interface to implement for observers that wish to be informed of changes
+  // to the CefBrowserHostImpl. All methods will be called on the UI thread.
+  class Observer {
+   public:
+    // Called before |browser| is destroyed. Any references to |browser| should
+    // be cleared when this method is called.
+    virtual void OnBrowserDestroyed(CefBrowserHostImpl* browser) =0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
   ~CefBrowserHostImpl() override;
 
   // Create a new CefBrowserHostImpl instance.
@@ -420,6 +433,13 @@ class CefBrowserHostImpl : public CefBrowserHost,
   // Override to provide a thread safe implementation.
   bool Send(IPC::Message* message) override;
 
+  // Manage observer objects. The observer must either outlive this object or
+  // remove itself before destruction. These methods can only be called on the
+  // UI thread.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+  bool HasObserver(Observer* observer) const;
+
  private:
   class DevToolsWebContentsObserver;
 
@@ -570,6 +590,9 @@ class CefBrowserHostImpl : public CefBrowserHost,
   // destroyed.
   CefDevToolsFrontend* devtools_frontend_;
 
+  // Observers that want to be notified of changes to this object.
+  base::ObserverList<Observer> observers_;
+
   IMPLEMENT_REFCOUNTING(CefBrowserHostImpl);
   DISALLOW_COPY_AND_ASSIGN(CefBrowserHostImpl);
 };

libcef/browser/download_manager_delegate.cc

@@ -5,9 +5,6 @@
 #include "libcef/browser/download_manager_delegate.h"
 
 #include "include/cef_download_handler.h"
-#include "libcef/browser/browser_context.h"
-#include "libcef/browser/browser_host_impl.h"
-#include "libcef/browser/context.h"
 #include "libcef/browser/download_item_impl.h"
 #include "libcef/browser/thread_util.h"
 
@@ -26,18 +23,8 @@ using content::DownloadItem;
 using content::DownloadManager;
 using content::WebContents;
 
-
 namespace {
 
-// Helper function to retrieve the CefBrowserHostImpl.
-CefRefPtr<CefBrowserHostImpl> GetBrowser(DownloadItem* item) {
-  content::WebContents* contents = item->GetWebContents();
-  if (!contents)
-    return NULL;
-
-  return CefBrowserHostImpl::GetBrowserForContents(contents).get();
-}
-
 // Helper function to retrieve the CefDownloadHandler.
 CefRefPtr<CefDownloadHandler> GetDownloadHandler(
     CefRefPtr<CefBrowserHostImpl> browser) {
@@ -274,10 +261,8 @@ CefDownloadManagerDelegate::CefDownloadManagerDelegate(
   DownloadManager::DownloadVector items;
   manager->GetAllDownloads(&items);
   DownloadManager::DownloadVector::const_iterator it = items.begin();
-  for (; it != items.end(); ++it) {
+  for (; it != items.end(); ++it)
-    (*it)->AddObserver(this);
+    OnDownloadCreated(manager, *it);
-    observing_.insert(*it);
-  }
 }
 
 CefDownloadManagerDelegate::~CefDownloadManagerDelegate() {
@@ -286,10 +271,8 @@ CefDownloadManagerDelegate::~CefDownloadManagerDelegate() {
     manager_->RemoveObserver(this);
   }
 
-  std::set<DownloadItem*>::const_iterator it = observing_.begin();
+  while (!item_browser_map_.empty())
-  for (; it != observing_.end(); ++it)
+    OnDownloadDestroyed(item_browser_map_.begin()->first);
-    (*it)->RemoveObserver(this);
-  observing_.clear();
 }
 
 void CefDownloadManagerDelegate::OnDownloadUpdated(
@@ -313,16 +296,52 @@ void CefDownloadManagerDelegate::OnDownloadUpdated(
 }
 
 void CefDownloadManagerDelegate::OnDownloadDestroyed(
-    DownloadItem* download) {
+    DownloadItem* item) {
-  download->RemoveObserver(this);
+  item->RemoveObserver(this);
-  observing_.erase(download);
+
+  CefBrowserHostImpl* browser = nullptr;
+
+  ItemBrowserMap::iterator it = item_browser_map_.find(item);
+  DCHECK(it != item_browser_map_.end());
+  if (it != item_browser_map_.end()) {
+    browser = it->second;
+    item_browser_map_.erase(it);
+  }
+
+  if (browser) {
+    // Determine if any remaining DownloadItems are associated with the same
+    // browser. If not, then unregister as an observer.
+    bool has_remaining = false;
+    ItemBrowserMap::const_iterator it2 = item_browser_map_.begin();
+    for (; it2 != item_browser_map_.end(); ++it2) {
+      if (it2->second == browser) {
+        has_remaining = true;
+        break;
+      }
+    }
+
+    if (!has_remaining)
+      browser->RemoveObserver(this);
+  }
 }
 
 void CefDownloadManagerDelegate::OnDownloadCreated(
     DownloadManager* manager,
     DownloadItem* item) {
   item->AddObserver(this);
-  observing_.insert(item);
+
+  CefBrowserHostImpl* browser = nullptr;
+  content::WebContents* contents = item->GetWebContents();
+  if (contents)
+    browser = CefBrowserHostImpl::GetBrowserForContents(contents).get();
+  DCHECK(browser);
+
+  item_browser_map_.insert(std::make_pair(item, browser));
+
+  // Register as an observer so that we can cancel associated DownloadItems when
+  // the browser is destroyed.
+  if (!browser->HasObserver(this))
+    browser->AddObserver(this);
 }
 
 void CefDownloadManagerDelegate::ManagerGoingDown(
@@ -379,3 +398,26 @@ void CefDownloadManagerDelegate::GetNextId(
   static uint32 next_id = DownloadItem::kInvalidId + 1;
   callback.Run(next_id++);
 }
+
+void CefDownloadManagerDelegate::OnBrowserDestroyed(
+    CefBrowserHostImpl* browser) {
+  ItemBrowserMap::iterator it = item_browser_map_.begin();
+  for (; it != item_browser_map_.end(); ++it) {
+    if (it->second == browser) {
+      // Don't call back into browsers that have been destroyed. We're not
+      // canceling the download so it will continue silently until it completes
+      // or until the associated browser context is destroyed.
+      it->second = nullptr;
+    }
+  }
+}
+
+CefBrowserHostImpl* CefDownloadManagerDelegate::GetBrowser(DownloadItem* item) {
+  ItemBrowserMap::const_iterator it = item_browser_map_.find(item);
+  if (it != item_browser_map_.end())
+    return it->second;
+
+  // An entry should always exist for a DownloadItem.
+  NOTREACHED();
+  return nullptr;
+}

libcef/browser/download_manager_delegate.h

@@ -8,6 +8,8 @@
 
 #include <set>
 
+#include "libcef/browser/browser_host_impl.h"
+
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #include "content/public/browser/download_item.h"
@@ -17,15 +19,16 @@
 class CefDownloadManagerDelegate
     : public content::DownloadItem::Observer,
       public content::DownloadManager::Observer,
-      public content::DownloadManagerDelegate {
+      public content::DownloadManagerDelegate,
+      public CefBrowserHostImpl::Observer {
  public:
   explicit CefDownloadManagerDelegate(content::DownloadManager* manager);
   ~CefDownloadManagerDelegate() override;
 
  private:
   // DownloadItem::Observer methods.
-  void OnDownloadUpdated(content::DownloadItem* download) override;
+  void OnDownloadUpdated(content::DownloadItem* item) override;
-  void OnDownloadDestroyed(content::DownloadItem* download) override;
+  void OnDownloadDestroyed(content::DownloadItem* item) override;
 
   // DownloadManager::Observer methods.
   void OnDownloadCreated(content::DownloadManager* manager,
@@ -38,9 +41,19 @@ class CefDownloadManagerDelegate
       const content::DownloadTargetCallback& callback) override;
   void GetNextId(const content::DownloadIdCallback& callback) override;
 
+  // CefBrowserHostImpl::Observer methods.
+  void OnBrowserDestroyed(CefBrowserHostImpl* browser) override;
+
+  CefBrowserHostImpl* GetBrowser(content::DownloadItem* item);
+
   content::DownloadManager* manager_;
   base::WeakPtrFactory<content::DownloadManager> manager_ptr_factory_;
-  std::set<content::DownloadItem*> observing_;
+
+  // Map of DownloadItem to originating CefBrowserHostImpl. Maintaining this
+  // map is necessary because DownloadItem::GetWebContents() may return NULL if
+  // the browser navigates while the download is in progress.
+  typedef std::map<content::DownloadItem*, CefBrowserHostImpl* > ItemBrowserMap;
+  ItemBrowserMap item_browser_map_;
 
   DISALLOW_COPY_AND_ASSIGN(CefDownloadManagerDelegate);
 };

tests/unittests/download_unittest.cc

@@ -9,24 +9,31 @@
 #include "base/files/scoped_temp_dir.h"
 
 #include "include/cef_scheme.h"
+#include "include/wrapper/cef_closure_task.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "tests/unittests/test_handler.h"
 
 namespace {
 
-const char kTestDomain[] = "test-download";
+const char kTestDomain[] = "test-download.com";
-const char kTestEntryUrl[] = "http://test-download/test.html";
+const char kTestEntryUrl[] = "http://test-download.com/test.html";
-const char kTestDownloadUrl[] = "http://test-download/download.txt";
+const char kTestDownloadUrl[] = "http://test-download.com/download.txt";
+const char kTestNavUrl[] = "http://test-download-nav.com/nav.html";
 const char kTestFileName[] = "download_test.txt";
 const char kTestContentDisposition[] =
     "attachment; filename=\"download_test.txt\"";
 const char kTestMimeType[] = "text/plain";
 const char kTestContent[] = "Download test text";
 
+typedef base::Callback<void(CefRefPtr<CefCallback>/*callback*/)> DelayCallback;
+
 class DownloadSchemeHandler : public CefResourceHandler {
  public:
-  explicit DownloadSchemeHandler(TrackCallback* got_download_request)
+  DownloadSchemeHandler(const DelayCallback& delay_callback,
-      : got_download_request_(got_download_request),
+                        TrackCallback* got_download_request)
+      : delay_callback_(delay_callback),
+        got_download_request_(got_download_request),
+        should_delay_(false),
         offset_(0) {}
 
   bool ProcessRequest(CefRefPtr<CefRequest> request,
@@ -40,6 +47,7 @@ class DownloadSchemeHandler : public CefResourceHandler {
       content_ = kTestContent;
       mime_type_ = kTestMimeType;
       content_disposition_ = kTestContentDisposition;
+      should_delay_ = true;
     } else {
       EXPECT_TRUE(false); // Not reached.
       return false;
@@ -70,9 +78,16 @@ class DownloadSchemeHandler : public CefResourceHandler {
                     int bytes_to_read,
                     int& bytes_read,
                     CefRefPtr<CefCallback> callback) override {
-    bool has_data = false;
     bytes_read = 0;
 
+    if (should_delay_ && !delay_callback_.is_null()) {
+      // Delay the download response a single time.
+      delay_callback_.Run(callback);
+      delay_callback_.Reset();
+      return true;
+    }
+
+    bool has_data = false;
     size_t size = content_.size();
     if (offset_ < size) {
       int transfer_size =
@@ -91,7 +106,9 @@ class DownloadSchemeHandler : public CefResourceHandler {
   }
 
  private:
+  DelayCallback delay_callback_;
   TrackCallback* got_download_request_;
+  bool should_delay_;
   std::string content_;
   std::string mime_type_;
   std::string content_disposition_;
@@ -102,18 +119,21 @@ class DownloadSchemeHandler : public CefResourceHandler {
 
 class DownloadSchemeHandlerFactory : public CefSchemeHandlerFactory {
  public:
-  explicit DownloadSchemeHandlerFactory(TrackCallback* got_download_request)
+  DownloadSchemeHandlerFactory(const DelayCallback& delay_callback,
-    : got_download_request_(got_download_request) {}
+                               TrackCallback* got_download_request)
+    : delay_callback_(delay_callback),
+      got_download_request_(got_download_request) {}
 
   CefRefPtr<CefResourceHandler> Create(
       CefRefPtr<CefBrowser> browser,
       CefRefPtr<CefFrame> frame,
       const CefString& scheme_name,
       CefRefPtr<CefRequest> request) override {
-    return new DownloadSchemeHandler(got_download_request_);
+    return new DownloadSchemeHandler(delay_callback_, got_download_request_);
   }
 
  private:
+  DelayCallback delay_callback_;
   TrackCallback* got_download_request_;
 
   IMPLEMENT_REFCOUNTING(DownloadSchemeHandlerFactory);
@@ -121,16 +141,34 @@ class DownloadSchemeHandlerFactory : public CefSchemeHandlerFactory {
 
 class DownloadTestHandler : public TestHandler {
  public:
-  DownloadTestHandler() {}
+  enum TestMode {
+    NORMAL,
+    NAVIGATED,
+    PENDING,
+  };
+
+  DownloadTestHandler(TestMode test_mode)
+      : test_mode_(test_mode) {}
 
   void RunTest() override {
+    DelayCallback delay_callback;
+    if (test_mode_ == NAVIGATED || test_mode_ == PENDING)
+      delay_callback = base::Bind(&DownloadTestHandler::OnDelayCallback, this);
+
     CefRegisterSchemeHandlerFactory("http", kTestDomain,
-        new DownloadSchemeHandlerFactory(&got_download_request_));
+        new DownloadSchemeHandlerFactory(delay_callback,
+                                         &got_download_request_));
 
     // Create a new temporary directory.
     EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
     test_path_ = temp_dir_.path().AppendASCII(kTestFileName);
 
+    if (test_mode_ == NAVIGATED) {
+      // Add the resource that we'll navigate to.
+      AddResource(kTestNavUrl, "<html><body>Navigated</body></html>",
+                  "text/html");
+    }
+
     // Create the browser
     CreateBrowser(kTestEntryUrl);
 
@@ -141,10 +179,51 @@ class DownloadTestHandler : public TestHandler {
   void OnLoadEnd(CefRefPtr<CefBrowser> browser,
                  CefRefPtr<CefFrame> frame,
                  int httpStatusCode) override {
-    EXPECT_STREQ(kTestEntryUrl, frame->GetURL().ToString().c_str());
+    const std::string& url = frame->GetURL().ToString();
+    if (url == kTestEntryUrl) {
+      // Begin the download.
+      browser->GetHost()->StartDownload(kTestDownloadUrl);
+    } else if (url == kTestNavUrl) {
+      got_nav_load_.yes();
+      ContinueNavigatedIfReady();
+    }
+  }
 
-    // Begin the download.
+  // Callback from the scheme handler when the download request is delayed.
-    browser->GetHost()->StartDownload(kTestDownloadUrl);
+  void OnDelayCallback(CefRefPtr<CefCallback> callback) {
+    if (!CefCurrentlyOn(TID_UI)) {
+      CefPostTask(TID_UI,
+          base::Bind(&DownloadTestHandler::OnDelayCallback, this, callback));
+      return;
+    }
+
+    got_delay_callback_.yes();
+
+    if (test_mode_ == NAVIGATED) {
+      delay_callback_ = callback;
+      ContinueNavigatedIfReady();
+    } else if (test_mode_ == PENDING) {
+      ContinuePendingIfReady();
+    } else {
+      EXPECT_TRUE(false);  // Not reached.
+    }
+  }
+
+  void ContinueNavigatedIfReady() {
+    DCHECK_EQ(test_mode_, NAVIGATED);
+    if (got_delay_callback_ && got_nav_load_) {
+      delay_callback_->Continue();
+      delay_callback_ = nullptr;
+    }
+  }
+
+  void ContinuePendingIfReady() {
+    DCHECK_EQ(test_mode_, PENDING);
+    if (got_delay_callback_ && got_on_before_download_ &&
+        got_on_download_updated_) {
+      // Destroy the test without waiting for the download to complete.
+      DestroyTest();
+    }
   }
 
   void OnBeforeDownload(
@@ -179,6 +258,11 @@ class DownloadTestHandler : public TestHandler {
     EXPECT_STREQ(kTestMimeType, download_item->GetMimeType().ToString().c_str());
 
     callback->Continue(test_path_.value(), false);
+
+    if (test_mode_ == NAVIGATED)
+      browser->GetMainFrame()->LoadURL(kTestNavUrl);
+    else if (test_mode_ == PENDING)
+      ContinuePendingIfReady();
   }
 
   void OnDownloadUpdated(
@@ -215,6 +299,8 @@ class DownloadTestHandler : public TestHandler {
     }
 
     if (download_item->IsComplete()) {
+      got_download_complete_.yes();
+
       EXPECT_FALSE(download_item->IsInProgress());
       EXPECT_EQ(100, download_item->GetPercentComplete());
       EXPECT_EQ(static_cast<int64>(sizeof(kTestContent)-1),
@@ -227,6 +313,9 @@ class DownloadTestHandler : public TestHandler {
       EXPECT_TRUE(download_item->IsInProgress());
       EXPECT_LE(0LL, download_item->GetReceivedBytes());
     }
+
+    if (test_mode_ == PENDING)
+      ContinuePendingIfReady();
   }
 
   void DestroyTest() override {
@@ -235,12 +324,24 @@ class DownloadTestHandler : public TestHandler {
     EXPECT_TRUE(got_download_request_);
     EXPECT_TRUE(got_on_before_download_);
     EXPECT_TRUE(got_on_download_updated_);
-    EXPECT_TRUE(got_full_path_);
 
-    // Verify the file contents.
+    if (test_mode_ == NAVIGATED)
-    std::string contents;
+      EXPECT_TRUE(got_nav_load_);
-    EXPECT_TRUE(base::ReadFileToString(test_path_, &contents));
+    else
-    EXPECT_STREQ(kTestContent, contents.c_str());
+      EXPECT_FALSE(got_nav_load_);
+
+    if (test_mode_ == PENDING) {
+      EXPECT_FALSE(got_download_complete_);
+      EXPECT_FALSE(got_full_path_);
+    } else {
+      EXPECT_TRUE(got_download_complete_);
+      EXPECT_TRUE(got_full_path_);
+
+      // Verify the file contents.
+      std::string contents;
+      EXPECT_TRUE(base::ReadFileToString(test_path_, &contents));
+      EXPECT_STREQ(kTestContent, contents.c_str());
+    }
 
     EXPECT_TRUE(temp_dir_.Delete());
 
@@ -248,6 +349,11 @@ class DownloadTestHandler : public TestHandler {
   }
 
  private:
+  const TestMode test_mode_;
+
+  // Used with NAVIGATED test mode.
+  CefRefPtr<CefCallback> delay_callback_;
+
   base::ScopedTempDir temp_dir_;
   base::FilePath test_path_;
   uint32 download_id_;
@@ -256,15 +362,35 @@ class DownloadTestHandler : public TestHandler {
   TrackCallback got_on_before_download_;
   TrackCallback got_on_download_updated_;
   TrackCallback got_full_path_;
+  TrackCallback got_download_complete_;
+  TrackCallback got_delay_callback_;
+  TrackCallback got_nav_load_;
 
   IMPLEMENT_REFCOUNTING(DownloadTestHandler);
 };
 
 }  // namespace
 
-// Verify that downloads work.
+// Test a basic download.
 TEST(DownloadTest, Download) {
-  CefRefPtr<DownloadTestHandler> handler = new DownloadTestHandler();
+  CefRefPtr<DownloadTestHandler> handler =
+      new DownloadTestHandler(DownloadTestHandler::NORMAL);
+  handler->ExecuteTest();
+  ReleaseAndWaitForDestructor(handler);
+}
+
+// Test where the download completes after cross-origin navigation.
+TEST(DownloadTest, DownloadNavigated) {
+  CefRefPtr<DownloadTestHandler> handler =
+      new DownloadTestHandler(DownloadTestHandler::NAVIGATED);
+  handler->ExecuteTest();
+  ReleaseAndWaitForDestructor(handler);
+}
+
+// Test where the download is still pending when the browser is destroyed.
+TEST(DownloadTest, DownloadPending) {
+  CefRefPtr<DownloadTestHandler> handler =
+      new DownloadTestHandler(DownloadTestHandler::PENDING);
   handler->ExecuteTest();
   ReleaseAndWaitForDestructor(handler);
 }