fix: destroy WebContents synchronously on shutdown (#15541)

atom/browser/api/atom_api_browser_view.cc

@@ -66,6 +66,7 @@ void BrowserView::Init(v8::Isolate* isolate,
 
   web_contents_.Reset(isolate, web_contents.ToV8());
   api_web_contents_ = web_contents.get();
+  Observe(web_contents->web_contents());
 
   view_.reset(
       NativeBrowserView::Create(api_web_contents_->managed_web_contents()));
@@ -74,7 +75,16 @@ void BrowserView::Init(v8::Isolate* isolate,
 }
 
 BrowserView::~BrowserView() {
-  api_web_contents_->DestroyWebContents(true /* async */);
+  if (api_web_contents_) {  // destroy() is called
+    // Destroy WebContents asynchronously unless app is shutting down,
+    // because destroy() might be called inside WebContents's event handler.
+    api_web_contents_->DestroyWebContents(!Browser::Get()->is_shutting_down());
+  }
+}
+
+void BrowserView::WebContentsDestroyed() {
+  api_web_contents_ = nullptr;
+  web_contents_.Reset();
 }
 
 // static

atom/browser/api/atom_api_browser_view.h

@@ -10,6 +10,7 @@
 
 #include "atom/browser/api/trackable_object.h"
 #include "atom/browser/native_browser_view.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "native_mate/handle.h"
 
 namespace gfx {
@@ -29,7 +30,8 @@ namespace api {
 
 class WebContents;
 
-class BrowserView : public mate::TrackableObject<BrowserView> {
+class BrowserView : public mate::TrackableObject<BrowserView>,
+                    public content::WebContentsObserver {
  public:
   static mate::WrappableBase* New(mate::Arguments* args);
 
@@ -47,6 +49,9 @@ class BrowserView : public mate::TrackableObject<BrowserView> {
               const mate::Dictionary& options);
   ~BrowserView() override;
 
+  // content::WebContentsObserver:
+  void WebContentsDestroyed() override;
+
  private:
   void Init(v8::Isolate* isolate,
             v8::Local<v8::Object> wrapper,

atom/browser/api/atom_api_browser_window.cc

@@ -383,7 +383,9 @@ void BrowserWindow::Cleanup() {
   if (host)
     host->GetWidget()->RemoveInputEventObserver(this);
 
-  api_web_contents_->DestroyWebContents(true /* async */);
+  // Destroy WebContents asynchronously unless app is shutting down,
+  // because destroy() might be called inside WebContents's event handler.
+  api_web_contents_->DestroyWebContents(!Browser::Get()->is_shutting_down());
   Observe(nullptr);
 }
 

atom/browser/api/atom_api_web_contents.cc

@@ -24,6 +24,7 @@
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/atom_browser_main_parts.h"
 #include "atom/browser/atom_javascript_dialog_manager.h"
+#include "atom/browser/browser.h"
 #include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/lib/bluetooth_chooser.h"
 #include "atom/browser/native_window.h"
@@ -483,14 +484,25 @@ WebContents::~WebContents() {
     RenderViewDeleted(web_contents()->GetRenderViewHost());
 
     if (type_ == WEB_VIEW) {
+      // For webview simply destroy the WebContents immediately.
+      // TODO(zcbenz): Add an internal API for webview instead of using
+      // destroy(), so we don't have to add a special branch here.
+      DestroyWebContents(false /* async */);
+    } else if (type_ == BROWSER_WINDOW && owner_window()) {
+      // For BrowserWindow we should close the window and clean up everything
+      // before WebContents is destroyed.
+      for (ExtendedWebContentsObserver& observer : observers_)
+        observer.OnCloseContents();
+      // BrowserWindow destroys WebContents asynchronously, manually emit the
+      // destroyed event here.
+      WebContentsDestroyed();
+    } else if (Browser::Get()->is_shutting_down()) {
+      // Destroy WebContents directly when app is shutting down.
       DestroyWebContents(false /* async */);
     } else {
-      if (type_ == BROWSER_WINDOW && owner_window()) {
-        for (ExtendedWebContentsObserver& observer : observers_)
-          observer.OnCloseContents();
-      } else {
-        DestroyWebContents(true /* async */);
-      }
+      // Destroy WebContents asynchronously unless app is shutting down,
+      // because destroy() might be called inside WebContents's event handler.
+      DestroyWebContents(true /* async */);
       // The WebContentsDestroyed will not be called automatically because we
       // destroy the webContents in the next tick. So we have to manually
       // call it here to make sure "destroyed" event is emitted.

atom/browser/api/atom_api_web_contents.h

@@ -99,7 +99,19 @@ class WebContents : public mate::TrackableObject<WebContents>,
 
   static int64_t GetIDForContents(content::WebContents* web_contents);
 
-  // Notifies to destroy any guest web contents before destroying self.
+  // Destroy the managed content::WebContents instance.
+  //
+  // Note: The |async| should only be |true| when users are expecting to use the
+  // webContents immediately after the call. Always pass |false| if you are not
+  // sure.
+  // See https://github.com/electron/electron/issues/8930.
+  //
+  // Note: When destroying a webContents member inside a destructor, the |async|
+  // should always be |false|, otherwise the destroy task might be delayed after
+  // normal shutdown procedure, resulting in an assertion.
+  // The normal pattern for calling this method in destructor is:
+  // api_web_contents_->DestroyWebContents(!Browser::Get()->is_shutting_down())
+  // See https://github.com/electron/electron/issues/15133.
   void DestroyWebContents(bool async);
 
   int64_t GetID() const;

atom/browser/api/atom_api_web_contents_view.cc

@@ -5,6 +5,7 @@
 #include "atom/browser/api/atom_api_web_contents_view.h"
 
 #include "atom/browser/api/atom_api_web_contents.h"
+#include "atom/browser/browser.h"
 #include "atom/common/api/constructor.h"
 #include "brightray/browser/inspectable_web_contents_view.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -64,8 +65,11 @@ WebContentsView::WebContentsView(v8::Isolate* isolate,
 }
 
 WebContentsView::~WebContentsView() {
-  if (api_web_contents_)
-    api_web_contents_->DestroyWebContents(false /* async */);
+  if (api_web_contents_) {  // destroy() is called
+    // Destroy WebContents asynchronously unless app is shutting down,
+    // because destroy() might be called inside WebContents's event handler.
+    api_web_contents_->DestroyWebContents(!Browser::Get()->is_shutting_down());
+  }
 }
 
 void WebContentsView::WebContentsDestroyed() {

atom/browser/common_web_contents_delegate.cc

@@ -194,8 +194,17 @@ void CommonWebContentsDelegate::SetOwnerWindow(
 
 void CommonWebContentsDelegate::ResetManagedWebContents(bool async) {
   if (async) {
-    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,
-                                                    web_contents_.release());
+    // Browser context should be destroyed only after the WebContents,
+    // this is guaranteed in the sync mode by the order of declaration,
+    // in the async version we maintain a reference until the WebContents
+    // is destroyed.
+    base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask(
+        FROM_HERE,
+        base::BindOnce([](scoped_refptr<AtomBrowserContext> browser_context,
+                          std::unique_ptr<brightray::InspectableWebContents>
+                              web_contents) { web_contents.reset(); },
+                       base::RetainedRef(browser_context_),
+                       std::move(web_contents_)));
   } else {
     web_contents_.reset();
   }

spec/api-browser-view-spec.js

@@ -1,8 +1,11 @@
 'use strict'
 
 const chai = require('chai')
+const ChildProcess = require('child_process')
 const dirtyChai = require('dirty-chai')
-const {closeWindow} = require('./window-helpers')
+const path = require('path')
+const { emittedOnce } = require('./events-helpers')
+const { closeWindow } = require('./window-helpers')
 
 const {remote} = require('electron')
 const {BrowserView, BrowserWindow} = remote
@@ -172,4 +175,14 @@ describe('BrowserView module', () => {
       expect(views[0].webContents.id).to.equal(view.webContents.id)
     })
   })
+
+  describe('new BrowserView()', () => {
+    it('does not crash on exit', async () => {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-browserview.js')
+      const electronPath = remote.getGlobal('process').execPath
+      const appProcess = ChildProcess.spawn(electronPath, [appPath])
+      const [code] = await emittedOnce(appProcess, 'close')
+      expect(code).to.equal(0)
+    })
+  })
 })

spec/api-web-contents-spec.js

@@ -1,6 +1,9 @@
 'use strict'
 
 const assert = require('assert')
+const chai = require('chai')
+const ChildProcess = require('child_process')
+const dirtyChai = require('dirty-chai')
 const http = require('http')
 const path = require('path')
 const {closeWindow} = require('./window-helpers')
@@ -9,6 +12,9 @@ const {emittedOnce} = require('./events-helpers')
 const {ipcRenderer, remote, clipboard} = require('electron')
 const {BrowserWindow, webContents, ipcMain, session} = remote
 
+const {expect} = chai
+chai.use(dirtyChai)
+
 const isCi = remote.getGlobal('isCi')
 
 /* The whole webContents API doesn't use standard callbacks */
@@ -667,6 +673,16 @@ describe('webContents module', () => {
     })
   })
 
+  describe('create()', () => {
+    it('does not crash on exit', async () => {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-webcontents.js')
+      const electronPath = remote.getGlobal('process').execPath
+      const appProcess = ChildProcess.spawn(electronPath, [appPath])
+      const [code] = await emittedOnce(appProcess, 'close')
+      expect(code).to.equal(0)
+    })
+  })
+
   // Destroying webContents in its event listener is going to crash when
   // Electron is built in Debug mode.
   xdescribe('destroy()', () => {

spec/api-web-contents-view-spec.js

@@ -1,8 +1,18 @@
 'use strict'
 
 const assert = require('assert')
-const {closeWindow} = require('./window-helpers')
-const {webContents, TopLevelWindow, WebContentsView} = require('electron').remote
+const chai = require('chai')
+const ChildProcess = require('child_process')
+const dirtyChai = require('dirty-chai')
+const path = require('path')
+const { emittedOnce } = require('./events-helpers')
+const { closeWindow } = require('./window-helpers')
+
+const { remote } = require('electron')
+const { webContents, TopLevelWindow, WebContentsView } = remote
+
+const { expect } = chai
+chai.use(dirtyChai)
 
 describe('WebContentsView', () => {
   let w = null
@@ -22,4 +32,14 @@ describe('WebContentsView', () => {
       w.setContentView(new WebContentsView(web))
     }, /The WebContents has already been added to a View/)
   })
+
+  describe('new WebContentsView()', () => {
+    it('does not crash on exit', async () => {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-webcontentsview.js')
+      const electronPath = remote.getGlobal('process').execPath
+      const appProcess = ChildProcess.spawn(electronPath, [appPath])
+      const [code] = await emittedOnce(appProcess, 'close')
+      expect(code).to.equal(0)
+    })
+  })
 })

spec/fixtures/api/leak-exit-browserview.js

@@ -0,0 +1,6 @@
+const { BrowserView, app } = require('electron')
+app.on('ready', function () {
+  new BrowserView({})  // eslint-disable-line
+
+  process.nextTick(() => app.quit())
+})

spec/fixtures/api/leak-exit-webcontents.js

@@ -0,0 +1,6 @@
+const { app, webContents } = require('electron')
+app.on('ready', function () {
+  webContents.create({})
+
+  process.nextTick(() => app.quit())
+})

spec/fixtures/api/leak-exit-webcontentsview.js

@@ -0,0 +1,7 @@
+const { WebContentsView, app, webContents } = require('electron')
+app.on('ready', function () {
+  const web = webContents.create({})
+  new WebContentsView(web)  // eslint-disable-line
+
+  process.nextTick(() => app.quit())
+})