fix: crash in BrowserWindow destructor after win.webContents.destroy() (

#18686) (#18794)
electron · Jun 15, 2019 · b04eaab · b04eaab
1 parent aae58ff
commit b04eaab
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 7 deletions.
diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc
@@ -68,7 +68,7 @@ BrowserWindow::BrowserWindow(v8::Isolate* isolate,
   }
 
   web_contents_.Reset(isolate, web_contents.ToV8());
-  api_web_contents_ = web_contents.get();
+  api_web_contents_ = web_contents->GetWeakPtr();
   api_web_contents_->AddObserver(this);
   Observe(api_web_contents_->web_contents());
 
@@ -95,7 +95,9 @@ BrowserWindow::BrowserWindow(v8::Isolate* isolate,
 }
 
 BrowserWindow::~BrowserWindow() {
-  api_web_contents_->RemoveObserver(this);
+  // FIXME This is a hack rather than a proper fix preventing shutdown crashes.
+  if (api_web_contents_)
+    api_web_contents_->RemoveObserver(this);
   // Note that the OnWindowClosed will not be called after the destructor runs,
   // since the window object is managed by the TopLevelWindow class.
   if (web_contents())

diff --git a/atom/browser/api/atom_api_browser_window.h b/atom/browser/api/atom_api_browser_window.h
@@ -116,7 +116,7 @@ class BrowserWindow : public TopLevelWindow,
 #endif
 
   v8::Global<v8::Value> web_contents_;
-  api::WebContents* api_web_contents_;
+  base::WeakPtr<api::WebContents> api_web_contents_;
 
   base::WeakPtrFactory<BrowserWindow> weak_factory_;
 

diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
@@ -287,7 +287,9 @@ struct WebContents::FrameDispatchHelper {
 
 WebContents::WebContents(v8::Isolate* isolate,
                          content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents), type_(REMOTE) {
+    : content::WebContentsObserver(web_contents),
+      type_(REMOTE),
+      weak_factory_(this) {
   web_contents->SetUserAgentOverride(GetBrowserContext()->GetUserAgent(),
                                      false);
   Init(isolate);
@@ -298,16 +300,18 @@ WebContents::WebContents(v8::Isolate* isolate,
 WebContents::WebContents(v8::Isolate* isolate,
                          std::unique_ptr<content::WebContents> web_contents,
                          Type type)
-    : content::WebContentsObserver(web_contents.get()), type_(type) {
+    : content::WebContentsObserver(web_contents.get()),
+      type_(type),
+      weak_factory_(this) {
   DCHECK(type != REMOTE) << "Can't take ownership of a remote WebContents";
   auto session = Session::CreateFrom(isolate, GetBrowserContext());
   session_.Reset(isolate, session.ToV8());
   InitWithSessionAndOptions(isolate, std::move(web_contents), session,
                             mate::Dictionary::CreateEmpty(isolate));
 }
 
-WebContents::WebContents(v8::Isolate* isolate,
-                         const mate::Dictionary& options) {
+WebContents::WebContents(v8::Isolate* isolate, const mate::Dictionary& options)
+    : weak_factory_(this) {
   // Read options.
   options.Get("backgroundThrottling", &background_throttling_);
 

diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h
@@ -111,6 +111,8 @@ class WebContents : public mate::TrackableObject<WebContents>,
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
 
+  base::WeakPtr<WebContents> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
+
   // Destroy the managed content::WebContents instance.
   //
   // Note: The |async| should only be |true| when users are expecting to use the
@@ -545,6 +547,8 @@ class WebContents : public mate::TrackableObject<WebContents>,
   // -1 means no speculative RVH has been committed yet.
   int currently_committed_process_id_ = -1;
 
+  base::WeakPtrFactory<WebContents> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(WebContents);
 };