Avoid potential use-after-free of CefIOThreadState (see issue #2969)

The problem occured while executing multiple URLRequestTest with the Chrome
runtime.

libcef/browser/browser_context.cc

@@ -7,7 +7,6 @@
 #include <map>
 #include <utility>
 
-#include "libcef/browser/iothread_state.h"
 #include "libcef/browser/media_router/media_router_manager.h"
 #include "libcef/browser/request_context_impl.h"
 #include "libcef/browser/thread_util.h"
@@ -156,13 +155,6 @@ CefBrowserContext::~CefBrowserContext() {
 #if DCHECK_IS_ON()
   DCHECK(is_shutdown_);
 #endif
-
-  if (iothread_state_) {
-    // Destruction of the CefIOThreadState will trigger destruction of all
-    // associated network requests.
-    content::BrowserThread::DeleteSoon(content::BrowserThread::IO, FROM_HERE,
-                                       iothread_state_.release());
-  }
 }
 
 void CefBrowserContext::Initialize() {
@@ -171,7 +163,7 @@ void CefBrowserContext::Initialize() {
   if (!cache_path_.empty())
     g_manager.Get().SetImplPath(this, cache_path_);
 
-  iothread_state_ = std::make_unique<CefIOThreadState>();
+  iothread_state_ = base::MakeRefCounted<CefIOThreadState>();
 }
 
 void CefBrowserContext::Shutdown() {
@@ -275,16 +267,11 @@ void CefBrowserContext::OnRenderFrameCreated(
     handler_map_.AddHandler(render_process_id, render_frame_id,
                             frame_tree_node_id, handler);
 
-    if (iothread_state_) {
+    CEF_POST_TASK(CEF_IOT,
-      // Using base::Unretained() is safe because both this callback and
+                  base::Bind(&CefIOThreadState::AddHandler, iothread_state_,
-      // possible deletion of |iothread_state_| will execute on the IO thread,
-      // and this callback will be executed first.
-      CEF_POST_TASK(CEF_IOT, base::Bind(&CefIOThreadState::AddHandler,
-                                        base::Unretained(iothread_state_.get()),
                              render_process_id, render_frame_id,
                              frame_tree_node_id, handler));
   }
-  }
 }
 
 void CefBrowserContext::OnRenderFrameDeleted(
@@ -313,15 +300,9 @@ void CefBrowserContext::OnRenderFrameDeleted(
     handler_map_.RemoveHandler(render_process_id, render_frame_id,
                                frame_tree_node_id);
 
-    if (iothread_state_) {
-      // Using base::Unretained() is safe because both this callback and
-      // possible deletion of |iothread_state_| will execute on the IO thread,
-      // and this callback will be executed first.
     CEF_POST_TASK(CEF_IOT, base::Bind(&CefIOThreadState::RemoveHandler,
-                                        base::Unretained(iothread_state_.get()),
+                                      iothread_state_, render_process_id,
-                                        render_process_id, render_frame_id,
+                                      render_frame_id, frame_tree_node_id));
-                                        frame_tree_node_id));
-    }
   }
 
   if (is_main_frame) {
@@ -425,26 +406,15 @@ void CefBrowserContext::RegisterSchemeHandlerFactory(
     const CefString& scheme_name,
     const CefString& domain_name,
     CefRefPtr<CefSchemeHandlerFactory> factory) {
-  if (iothread_state_) {
-    // Using base::Unretained() is safe because both this callback and possible
-    // deletion of |iothread_state_| will execute on the IO thread, and this
-    // callback will be executed first.
   CEF_POST_TASK(CEF_IOT,
                 base::Bind(&CefIOThreadState::RegisterSchemeHandlerFactory,
-                             base::Unretained(iothread_state_.get()),
+                           iothread_state_, scheme_name, domain_name, factory));
-                             scheme_name, domain_name, factory));
-  }
 }
 
 void CefBrowserContext::ClearSchemeHandlerFactories() {
-  if (iothread_state_) {
-    // Using base::Unretained() is safe because both this callback and possible
-    // deletion of |iothread_state_| will execute on the IO thread, and this
-    // callback will be executed first.
   CEF_POST_TASK(CEF_IOT,
                 base::Bind(&CefIOThreadState::ClearSchemeHandlerFactories,
-                             base::Unretained(iothread_state_.get())));
+                           iothread_state_));
-  }
 }
 
 void CefBrowserContext::LoadExtension(

libcef/browser/browser_context.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "include/cef_request_context_handler.h"
+#include "libcef/browser/iothread_state.h"
 #include "libcef/browser/request_context_handler_map.h"
 
 #include "base/callback.h"
@@ -80,7 +81,6 @@ class BrowserContext;
 
 class CefMediaRouterManager;
 class CefRequestContextImpl;
-class CefIOThreadState;
 class Profile;
 
 // Main entry point for configuring behavior on a per-RequestContext basis. The
@@ -208,7 +208,9 @@ class CefBrowserContext {
   // change during this object's lifespan.
   const CefRequestContextSettings& settings() const { return settings_; }
   base::FilePath cache_path() const { return cache_path_; }
-  CefIOThreadState* iothread_state() const { return iothread_state_.get(); }
+  scoped_refptr<CefIOThreadState> iothread_state() const {
+    return iothread_state_;
+  }
 
   // Used to hold a WeakPtr reference to this this object. The Getter returns
   // nullptr if this object has already been destroyed.
@@ -231,7 +233,7 @@ class CefBrowserContext {
   base::FilePath cache_path_;
 
  private:
-  std::unique_ptr<CefIOThreadState> iothread_state_;
+  scoped_refptr<CefIOThreadState> iothread_state_;
   CookieableSchemes cookieable_schemes_;
   std::unique_ptr<CefMediaRouterManager> media_router_manager_;
 

libcef/browser/iothread_state.cc

@@ -23,7 +23,9 @@ CefIOThreadState::CefIOThreadState() {
                                     base::Unretained(this)));
 }
 
-CefIOThreadState::~CefIOThreadState() {}
+CefIOThreadState::~CefIOThreadState() {
+  CEF_REQUIRE_IOT();
+}
 
 void CefIOThreadState::AddHandler(int render_process_id,
                                   int render_frame_id,

libcef/browser/iothread_state.h

@@ -12,15 +12,18 @@
 
 #include "libcef/browser/request_context_handler_map.h"
 
+#include "content/public/browser/browser_thread.h"
+
 class GURL;
 
 // Stores state that will be accessed on the IO thread. Life span is controlled
 // by CefBrowserContext. Created on the UI thread but accessed and destroyed on
 // the IO thread. See browser_context.h for an object relationship diagram.
-class CefIOThreadState {
+class CefIOThreadState : public base::RefCountedThreadSafe<
+                             CefIOThreadState,
+                             content::BrowserThread::DeleteOnIOThread> {
  public:
   CefIOThreadState();
-  virtual ~CefIOThreadState();
 
   // See comments in CefRequestContextHandlerMap.
   void AddHandler(int render_process_id,
@@ -44,6 +47,12 @@ class CefIOThreadState {
   CefRefPtr<CefSchemeHandlerFactory> GetSchemeHandlerFactory(const GURL& url);
 
  private:
+  friend struct content::BrowserThread::DeleteOnThread<
+      content::BrowserThread::IO>;
+  friend class base::DeleteHelper<CefIOThreadState>;
+
+  ~CefIOThreadState();
+
   void InitOnIOThread();
 
   // Map IDs to CefRequestContextHandler objects.

libcef/browser/net_service/resource_request_handler_wrapper.cc

@@ -264,7 +264,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler {
       auto profile = Profile::FromBrowserContext(browser_context);
       auto cef_browser_context = CefBrowserContext::FromProfile(profile);
       iothread_state_ = cef_browser_context->iothread_state();
-      DCHECK(iothread_state_);
+      CHECK(iothread_state_);
       cookieable_schemes_ = cef_browser_context->GetCookieableSchemes();
 
       // We register to be notified of CEF context or browser destruction so
@@ -314,7 +314,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler {
 
     CefRefPtr<CefBrowserHostBase> browser_;
     CefRefPtr<CefFrame> frame_;
-    CefIOThreadState* iothread_state_ = nullptr;
+    scoped_refptr<CefIOThreadState> iothread_state_;
     CefBrowserContext::CookieableSchemes cookieable_schemes_;
     int render_process_id_ = 0;
     int render_frame_id_ = -1;