Move CookieManager callbacks to the UI thread (see issue #2622).

The Chromium content layer (which also exposes the NetworkService interface)
generally runs on the UI thread. Previous use of the IO thread for CookieManager
callbacks is an implementation detail of the old network stack that shouldn't be
exposed to clients.

To test: Run ceftests. They should pass as expected.

include/capi/cef_cookie_capi.h

@@ -33,7 +33,7 @@
 // by hand. See the translator.README.txt file in the tools directory for
 // more information.
 //
-// $hash=fd4529fcb0b4cebf7e94a5160854f13ee05bbab0$
+// $hash=7ec5f39057b2a75576fe732fc7bff7b953956fe6$
 //
 
 #ifndef CEF_INCLUDE_CAPI_CEF_COOKIE_CAPI_H_
@@ -64,7 +64,7 @@ typedef struct _cef_cookie_manager_t {
   ///
   // Set the schemes supported by this manager. The default schemes ("http",
   // "https", "ws" and "wss") will always be supported. If |callback| is non-
-  // NULL it will be executed asnychronously on the IO thread after the change
+  // NULL it will be executed asnychronously on the UI thread after the change
   // has been applied. Must be called before any cookies are accessed.
   ///
   void(CEF_CALLBACK* set_supported_schemes)(
@@ -73,7 +73,7 @@ typedef struct _cef_cookie_manager_t {
       struct _cef_completion_callback_t* callback);
 
   ///
-  // Visit all cookies on the IO thread. The returned cookies are ordered by
+  // Visit all cookies on the UI thread. The returned cookies are ordered by
   // longest path, then by earliest creation date. Returns false (0) if cookies
   // cannot be accessed.
   ///
@@ -81,7 +81,7 @@ typedef struct _cef_cookie_manager_t {
                                        struct _cef_cookie_visitor_t* visitor);
 
   ///
-  // Visit a subset of cookies on the IO thread. The results are filtered by the
+  // Visit a subset of cookies on the UI thread. The results are filtered by the
   // given url scheme, host, domain and path. If |includeHttpOnly| is true (1)
   // HTTP-only cookies will also be included in the results. The returned
   // cookies are ordered by longest path, then by earliest creation date.
@@ -98,7 +98,7 @@ typedef struct _cef_cookie_manager_t {
   // check for disallowed characters (e.g. the ';' character is disallowed
   // within the cookie value attribute) and fail without setting the cookie if
   // such characters are found. If |callback| is non-NULL it will be executed
-  // asnychronously on the IO thread after the cookie has been set. Returns
+  // asnychronously on the UI thread after the cookie has been set. Returns
   // false (0) if an invalid URL is specified or if cookies cannot be accessed.
   ///
   int(CEF_CALLBACK* set_cookie)(struct _cef_cookie_manager_t* self,
@@ -112,7 +112,7 @@ typedef struct _cef_cookie_manager_t {
   // both will be deleted. If only |url| is specified all host cookies (but not
   // domain cookies) irrespective of path will be deleted. If |url| is NULL all
   // cookies for all hosts and domains will be deleted. If |callback| is non-
-  // NULL it will be executed asnychronously on the IO thread after the cookies
+  // NULL it will be executed asnychronously on the UI thread after the cookies
   // have been deleted. Returns false (0) if a non-NULL invalid URL is specified
   // or if cookies cannot be accessed. Cookies can alternately be deleted using
   // the Visit*Cookies() functions.
@@ -125,7 +125,7 @@ typedef struct _cef_cookie_manager_t {
 
   ///
   // Flush the backing store (if any) to disk. If |callback| is non-NULL it will
-  // be executed asnychronously on the IO thread after the flush is complete.
+  // be executed asnychronously on the UI thread after the flush is complete.
   // Returns false (0) if cookies cannot be accessed.
   ///
   int(CEF_CALLBACK* flush_store)(struct _cef_cookie_manager_t* self,
@@ -135,7 +135,7 @@ typedef struct _cef_cookie_manager_t {
 ///
 // Returns the global cookie manager. By default data will be stored at
 // CefSettings.cache_path if specified or in memory otherwise. If |callback| is
-// non-NULL it will be executed asnychronously on the IO thread after the
+// non-NULL it will be executed asnychronously on the UI thread after the
 // manager's storage has been initialized. Using this function is equivalent to
 // calling cef_request_tContext::cef_request_context_get_global_context()->GetDe
 // faultCookieManager().
@@ -145,7 +145,7 @@ CEF_EXPORT cef_cookie_manager_t* cef_cookie_manager_get_global_manager(
 
 ///
 // Structure to implement for visiting cookie values. The functions of this
-// structure will always be called on the IO thread.
+// structure will always be called on the UI thread.
 ///
 typedef struct _cef_cookie_visitor_t {
   ///

include/cef_cookie.h

@@ -56,7 +56,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
   ///
   // Returns the global cookie manager. By default data will be stored at
   // CefSettings.cache_path if specified or in memory otherwise. If |callback|
-  // is non-NULL it will be executed asnychronously on the IO thread after the
+  // is non-NULL it will be executed asnychronously on the UI thread after the
   // manager's storage has been initialized. Using this method is equivalent to
   // calling CefRequestContext::GetGlobalContext()->GetDefaultCookieManager().
   ///
@@ -67,7 +67,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
   ///
   // Set the schemes supported by this manager. The default schemes ("http",
   // "https", "ws" and "wss") will always be supported. If |callback| is non-
-  // NULL it will be executed asnychronously on the IO thread after the change
+  // NULL it will be executed asnychronously on the UI thread after the change
   // has been applied. Must be called before any cookies are accessed.
   ///
   /*--cef(optional_param=callback)--*/
@@ -76,7 +76,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
       CefRefPtr<CefCompletionCallback> callback) = 0;
 
   ///
-  // Visit all cookies on the IO thread. The returned cookies are ordered by
+  // Visit all cookies on the UI thread. The returned cookies are ordered by
   // longest path, then by earliest creation date. Returns false if cookies
   // cannot be accessed.
   ///
@@ -84,7 +84,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
   virtual bool VisitAllCookies(CefRefPtr<CefCookieVisitor> visitor) = 0;
 
   ///
-  // Visit a subset of cookies on the IO thread. The results are filtered by the
+  // Visit a subset of cookies on the UI thread. The results are filtered by the
   // given url scheme, host, domain and path. If |includeHttpOnly| is true
   // HTTP-only cookies will also be included in the results. The returned
   // cookies are ordered by longest path, then by earliest creation date.
@@ -101,7 +101,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
   // check for disallowed characters (e.g. the ';' character is disallowed
   // within the cookie value attribute) and fail without setting the cookie if
   // such characters are found. If |callback| is non-NULL it will be executed
-  // asnychronously on the IO thread after the cookie has been set. Returns
+  // asnychronously on the UI thread after the cookie has been set. Returns
   // false if an invalid URL is specified or if cookies cannot be accessed.
   ///
   /*--cef(optional_param=callback)--*/
@@ -115,7 +115,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
   // both will be deleted. If only |url| is specified all host cookies (but not
   // domain cookies) irrespective of path will be deleted. If |url| is empty all
   // cookies for all hosts and domains will be deleted. If |callback| is
-  // non-NULL it will be executed asnychronously on the IO thread after the
+  // non-NULL it will be executed asnychronously on the UI thread after the
   // cookies have been deleted. Returns false if a non-empty invalid URL is
   // specified or if cookies cannot be accessed. Cookies can alternately be
   // deleted using the Visit*Cookies() methods.
@@ -128,7 +128,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
 
   ///
   // Flush the backing store (if any) to disk. If |callback| is non-NULL it will
-  // be executed asnychronously on the IO thread after the flush is complete.
+  // be executed asnychronously on the UI thread after the flush is complete.
   // Returns false if cookies cannot be accessed.
   ///
   /*--cef(optional_param=callback)--*/
@@ -137,7 +137,7 @@ class CefCookieManager : public virtual CefBaseRefCounted {
 
 ///
 // Interface to implement for visiting cookie values. The methods of this class
-// will always be called on the IO thread.
+// will always be called on the UI thread.
 ///
 /*--cef(source=client)--*/
 class CefCookieVisitor : public virtual CefBaseRefCounted {

libcef/browser/net/cookie_manager_old_impl.cc

@@ -32,7 +32,8 @@ using content::BrowserThread;
 namespace {
 
 // Callback class for visiting cookies.
-class VisitCookiesCallback : public base::RefCounted<VisitCookiesCallback> {
+class VisitCookiesCallback
+    : public base::RefCountedThreadSafe<VisitCookiesCallback> {
  public:
   explicit VisitCookiesCallback(
       const CefCookieManagerOldImpl::CookieStoreGetter& cookie_store_getter,
@@ -41,7 +42,11 @@ class VisitCookiesCallback : public base::RefCounted<VisitCookiesCallback> {
 
   void Run(const net::CookieList& list,
            const net::CookieStatusList& excluded_list) {
-    CEF_REQUIRE_IOT();
+    if (!CEF_CURRENTLY_ON_UIT()) {
+      CEF_POST_TASK(CEF_UIT, base::Bind(&VisitCookiesCallback::Run, this, list,
+                                        excluded_list));
+      return;
+    }
 
     int total = list.size(), count = 0;
 
@@ -54,11 +59,9 @@ class VisitCookiesCallback : public base::RefCounted<VisitCookiesCallback> {
       bool deleteCookie = false;
       bool keepLooping = visitor_->Visit(cookie, count, total, deleteCookie);
       if (deleteCookie) {
-        net::CookieStore* cookie_store = cookie_store_getter_.Run();
+        CEF_POST_TASK(
-        if (cookie_store) {
+            CEF_IOT,
-          cookie_store->DeleteCanonicalCookieAsync(
+            base::Bind(&VisitCookiesCallback::DeleteOnIOThread, this, cc));
-              cc, net::CookieMonster::DeleteCallback());
-        }
       }
       if (!keepLooping)
         break;
@@ -66,10 +69,18 @@ class VisitCookiesCallback : public base::RefCounted<VisitCookiesCallback> {
   }
 
  private:
-  friend class base::RefCounted<VisitCookiesCallback>;
+  friend class base::RefCountedThreadSafe<VisitCookiesCallback>;
 
   ~VisitCookiesCallback() {}
 
+  void DeleteOnIOThread(const net::CanonicalCookie& cc) {
+    net::CookieStore* cookie_store = cookie_store_getter_.Run();
+    if (cookie_store) {
+      cookie_store->DeleteCanonicalCookieAsync(
+          cc, net::CookieMonster::DeleteCallback());
+    }
+  }
+
   CefCookieManagerOldImpl::CookieStoreGetter cookie_store_getter_;
   CefRefPtr<CefCookieVisitor> visitor_;
 };
@@ -88,10 +99,10 @@ bool GetCookieDomain(const GURL& url,
 }
 
 // Always execute the callback asynchronously.
-void RunAsyncCompletionOnIOThread(CefRefPtr<CefCompletionCallback> callback) {
+void RunAsyncCompletionOnUIThread(CefRefPtr<CefCompletionCallback> callback) {
   if (!callback.get())
     return;
-  CEF_POST_TASK(CEF_IOT,
+  CEF_POST_TASK(CEF_UIT,
                 base::Bind(&CefCompletionCallback::OnComplete, callback.get()));
 }
 
@@ -100,7 +111,7 @@ void DeleteCookiesCallbackImpl(CefRefPtr<CefDeleteCookiesCallback> callback,
                                uint32_t num_deleted) {
   if (!callback.get())
     return;
-  CEF_POST_TASK(CEF_IOT, base::Bind(&CefDeleteCookiesCallback::OnComplete,
+  CEF_POST_TASK(CEF_UIT, base::Bind(&CefDeleteCookiesCallback::OnComplete,
                                     callback.get(), num_deleted));
 }
 
@@ -110,7 +121,7 @@ void SetCookieCallbackImpl(CefRefPtr<CefSetCookieCallback> callback,
   if (!callback.get())
     return;
   CEF_POST_TASK(
-      CEF_IOT,
+      CEF_UIT,
       base::Bind(
           &CefSetCookieCallback::OnComplete, callback.get(),
           status == net::CanonicalCookie::CookieInclusionStatus::INCLUDE));
@@ -344,7 +355,7 @@ void CefCookieManagerOldImpl::InitWithContext(
   // reference to this object) and this object.
   request_context_ = NULL;
 
-  RunAsyncCompletionOnIOThread(callback);
+  RunAsyncCompletionOnUIThread(callback);
 }
 
 void CefCookieManagerOldImpl::SetSupportedSchemesWithContext(
@@ -355,7 +366,7 @@ void CefCookieManagerOldImpl::SetSupportedSchemesWithContext(
 
   request_context->SetCookieSupportedSchemes(schemes);
 
-  RunAsyncCompletionOnIOThread(callback);
+  RunAsyncCompletionOnUIThread(callback);
 }
 
 void CefCookieManagerOldImpl::GetCookieStoreWithContext(
@@ -506,9 +517,9 @@ void CefCookieManagerOldImpl::FlushStoreInternal(
 
   net::CookieStore* cookie_store = cookie_store_getter.Run();
   if (!cookie_store) {
-    RunAsyncCompletionOnIOThread(callback);
+    RunAsyncCompletionOnUIThread(callback);
     return;
   }
 
-  cookie_store->FlushStore(base::Bind(RunAsyncCompletionOnIOThread, callback));
+  cookie_store->FlushStore(base::Bind(RunAsyncCompletionOnUIThread, callback));
 }

libcef/browser/net_service/cookie_manager_impl.cc

@@ -25,10 +25,10 @@ CookieManager* GetCookieManager(CefRequestContextImpl* request_context) {
 }
 
 // Always execute the callback asynchronously.
-void RunAsyncCompletionOnIOThread(CefRefPtr<CefCompletionCallback> callback) {
+void RunAsyncCompletionOnUIThread(CefRefPtr<CefCompletionCallback> callback) {
   if (!callback.get())
     return;
-  CEF_POST_TASK(CEF_IOT,
+  CEF_POST_TASK(CEF_UIT,
                 base::Bind(&CefCompletionCallback::OnComplete, callback.get()));
 }
 
@@ -37,7 +37,7 @@ void SetCookieCallbackImpl(CefRefPtr<CefSetCookieCallback> callback,
                            bool success) {
   if (!callback.get())
     return;
-  CEF_POST_TASK(CEF_IOT, base::Bind(&CefSetCookieCallback::OnComplete,
+  CEF_POST_TASK(CEF_UIT, base::Bind(&CefSetCookieCallback::OnComplete,
                                     callback.get(), success));
 }
 
@@ -46,7 +46,7 @@ void DeleteCookiesCallbackImpl(CefRefPtr<CefDeleteCookiesCallback> callback,
                                uint32_t num_deleted) {
   if (!callback.get())
     return;
-  CEF_POST_TASK(CEF_IOT, base::Bind(&CefDeleteCookiesCallback::OnComplete,
+  CEF_POST_TASK(CEF_UIT, base::Bind(&CefDeleteCookiesCallback::OnComplete,
                                     callback.get(), num_deleted));
 }
 
@@ -64,23 +64,12 @@ void GetCefCookie(const net::CanonicalCookie& cc, CefCookie& cookie) {
     cef_time_from_basetime(cc.ExpiryDate(), cookie.expires);
 }
 
-void DeleteCanonicalCookie(CefRefPtr<CefRequestContextImpl> request_context,
-                           const net::CanonicalCookie& cc) {
-  if (!CEF_CURRENTLY_ON_UIT()) {
-    CEF_POST_TASK(CEF_UIT,
-                  base::Bind(&DeleteCanonicalCookie, request_context, cc));
-    return;
-  }
-
-  GetCookieManager(request_context.get())
-      ->DeleteCanonicalCookie(cc,
-                              CookieManager::DeleteCanonicalCookieCallback());
-}
-
 void ExecuteVisitor(CefRefPtr<CefCookieVisitor> visitor,
                     CefRefPtr<CefRequestContextImpl> request_context,
                     const std::vector<net::CanonicalCookie>& cookies) {
-  CEF_REQUIRE_IOT();
+  CEF_REQUIRE_UIT();
+
+  CookieManager* cookie_manager = GetCookieManager(request_context.get());
 
   int total = cookies.size(), count = 0;
   for (const auto& cc : cookies) {
@@ -90,7 +79,8 @@ void ExecuteVisitor(CefRefPtr<CefCookieVisitor> visitor,
     bool deleteCookie = false;
     bool keepLooping = visitor->Visit(cookie, count, total, deleteCookie);
     if (deleteCookie) {
-      DeleteCanonicalCookie(request_context, cc);
+      cookie_manager->DeleteCanonicalCookie(
+          cc, CookieManager::DeleteCanonicalCookieCallback());
     }
     if (!keepLooping)
       break;
@@ -102,7 +92,7 @@ void ExecuteVisitor(CefRefPtr<CefCookieVisitor> visitor,
 void GetCookiesCallbackImpl(CefRefPtr<CefCookieVisitor> visitor,
                             CefRefPtr<CefRequestContextImpl> request_context,
                             const std::vector<net::CanonicalCookie>& cookies) {
-  CEF_POST_TASK(CEF_IOT,
+  CEF_POST_TASK(CEF_UIT,
                 base::Bind(&ExecuteVisitor, visitor, request_context, cookies));
 }
 
@@ -116,7 +106,7 @@ void CefCookieManagerImpl::Initialize(
   DCHECK(request_context);
   DCHECK(!request_context_);
   request_context_ = request_context;
-  RunAsyncCompletionOnIOThread(callback);
+  RunAsyncCompletionOnUIThread(callback);
 }
 
 void CefCookieManagerImpl::SetSupportedSchemes(
@@ -132,7 +122,7 @@ void CefCookieManagerImpl::SetSupportedSchemes(
   // TODO(network): Figure out how to route this to
   // CookieMonster::SetCookieableSchemes via the NetworkService.
   NOTIMPLEMENTED();
-  RunAsyncCompletionOnIOThread(callback);
+  RunAsyncCompletionOnUIThread(callback);
 }
 
 bool CefCookieManagerImpl::VisitAllCookies(
@@ -271,7 +261,7 @@ bool CefCookieManagerImpl::FlushStore(
   }
 
   GetCookieManager(request_context_.get())
-      ->FlushCookieStore(base::Bind(RunAsyncCompletionOnIOThread, callback));
+      ->FlushCookieStore(base::Bind(RunAsyncCompletionOnUIThread, callback));
   return true;
 }
 

tests/ceftests/cookie_unittest.cc

@@ -30,7 +30,10 @@ class TestCompletionCallback : public CefCompletionCallback {
   explicit TestCompletionCallback(CefRefPtr<CefWaitableEvent> event)
       : event_(event) {}
 
-  void OnComplete() override { event_->Signal(); }
+  void OnComplete() override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
+    event_->Signal();
+  }
 
  private:
   CefRefPtr<CefWaitableEvent> event_;
@@ -46,6 +49,7 @@ class TestSetCookieCallback : public CefSetCookieCallback {
       : expected_success_(expected_success), event_(event) {}
 
   void OnComplete(bool success) override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
     EXPECT_EQ(expected_success_, success);
     event_->Signal();
   }
@@ -65,6 +69,7 @@ class TestDeleteCookiesCallback : public CefDeleteCookiesCallback {
       : expected_num_deleted_(expected_num_deleted), event_(event) {}
 
   void OnComplete(int num_deleted) override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
     if (expected_num_deleted_ != kIgnoreNumDeleted) {
       EXPECT_EQ(expected_num_deleted_, num_deleted);
     }
@@ -94,6 +99,7 @@ class TestVisitor : public CefCookieVisitor {
              int count,
              int total,
              bool& deleteCookie) override {
+    EXPECT_TRUE(CefCurrentlyOn(TID_UI));
     cookies_->push_back(cookie);
     if (delete_cookies_)
       deleteCookie = true;