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

@@ -381,7 +381,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

@@ -17,6 +17,7 @@
 #include "atom/browser/atom_browser_main_parts.h"
 #include "atom/browser/atom_javascript_dialog_manager.h"
 #include "atom/browser/atom_navigation_throttle.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"
@@ -493,14 +494,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()) {
+      // Destroy WebContents asynchronously unless app is shutting down,
-        for (ExtendedWebContentsObserver& observer : observers_)
+      // because destroy() might be called inside WebContents's event handler.
-          observer.OnCloseContents();
+      DestroyWebContents(true /* async */);
-      } else {
-        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.
@@ -567,6 +579,7 @@ void WebContents::AddNewContents(
   if (Emit("-add-new-contents", api_web_contents, disposition, user_gesture,
            initial_rect.x(), initial_rect.y(), initial_rect.width(),
            initial_rect.height(), tracker->url, tracker->frame_name)) {
+    // TODO(zcbenz): Can we make this sync?
     api_web_contents->DestroyWebContents(true /* async */);
   }
 }

atom/browser/api/atom_api_web_contents.h

@@ -106,7 +106,19 @@ class WebContents : public mate::TrackableObject<WebContents>,
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::FunctionTemplate> prototype);
 
-  // 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);
 
   void SetBackgroundThrottling(bool allowed);

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/browser/ui/inspectable_web_contents_view.h"
 #include "atom/common/api/constructor.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_)
+  if (api_web_contents_) {  // destroy() is called
-    api_web_contents_->DestroyWebContents(false /* 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());
+  }
 }
 
 void WebContentsView::WebContentsDestroyed() {

spec/api-browser-view-spec.js

@@ -1,7 +1,10 @@
 'use strict'
 
 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')
@@ -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,7 @@
 'use strict'
 
 const assert = require('assert')
+const ChildProcess = require('child_process')
 const fs = require('fs')
 const http = require('http')
 const path = require('path')
@@ -697,6 +698,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 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 { webContents, TopLevelWindow, WebContentsView } = require('electron').remote
+
+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())
+})