From eeb49d7f9402e730c5f49d5876ad64eb2e924bb3 Mon Sep 17 00:00:00 2001
From: Milan Burda <milan.burda@gmail.com>
Date: Fri, 14 Jun 2019 04:44:36 +0200
Subject: [PATCH] fix: crash in BrowserWindow destructor after
 win.webContents.destroy() (#18686)

---
 atom/browser/api/atom_api_browser_window.cc |  6 ++++--
 atom/browser/api/atom_api_browser_window.h  |  2 +-
 atom/browser/api/atom_api_web_contents.cc   | 12 ++++++++----
 atom/browser/api/atom_api_web_contents.h    |  4 ++++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc
index b136a4a33f606..b3e7ba7c89d71 100644
--- 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
index 6cf33384f60e5..b7c225da5e58b 100644
--- 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
index c2b57ea57928b..4454af99cf4d5 100644
--- 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,7 +300,9 @@ 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());
@@ -306,8 +310,8 @@ WebContents::WebContents(v8::Isolate* isolate,
                             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
index 5930a5b9c9607..f43e235da9c0e 100644
--- 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);
 };