From 002f9e60eb51f8a205e6f8330423eb108509acab Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 9 Nov 2018 01:09:55 +0900 Subject: [PATCH] fix: destroy WebContents synchronously on shutdown --- atom/browser/api/atom_api_browser_view.cc | 12 +++++++++- atom/browser/api/atom_api_browser_view.h | 7 +++++- atom/browser/api/atom_api_browser_window.cc | 4 +++- atom/browser/api/atom_api_web_contents.cc | 24 ++++++++++++++----- atom/browser/api/atom_api_web_contents.h | 14 ++++++++++- .../browser/api/atom_api_web_contents_view.cc | 8 +++++-- spec/api-browser-view-spec.js | 13 ++++++++++ spec/api-web-contents-spec.js | 11 +++++++++ spec/api-web-contents-view-spec.js | 17 ++++++++++++- spec/fixtures/api/leak-exit-browserview.js | 6 +++++ spec/fixtures/api/leak-exit-webcontents.js | 6 +++++ .../fixtures/api/leak-exit-webcontentsview.js | 7 ++++++ 12 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 spec/fixtures/api/leak-exit-browserview.js create mode 100644 spec/fixtures/api/leak-exit-webcontents.js create mode 100644 spec/fixtures/api/leak-exit-webcontentsview.js diff --git a/atom/browser/api/atom_api_browser_view.cc b/atom/browser/api/atom_api_browser_view.cc index bd83208694c61..6c9578c61eafe 100644 --- a/atom/browser/api/atom_api_browser_view.cc +++ b/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 diff --git a/atom/browser/api/atom_api_browser_view.h b/atom/browser/api/atom_api_browser_view.h index da99c5175b87f..932ced6dfe05d 100644 --- a/atom/browser/api/atom_api_browser_view.h +++ b/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 { +class BrowserView : public mate::TrackableObject, + public content::WebContentsObserver { public: static mate::WrappableBase* New(mate::Arguments* args); @@ -47,6 +49,9 @@ class BrowserView : public mate::TrackableObject { const mate::Dictionary& options); ~BrowserView() override; + // content::WebContentsObserver: + void WebContentsDestroyed() override; + private: void Init(v8::Isolate* isolate, v8::Local wrapper, diff --git a/atom/browser/api/atom_api_browser_window.cc b/atom/browser/api/atom_api_browser_window.cc index cab94da8a910b..5cf776b4b01a7 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -390,7 +390,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); } diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 183bfdee3bb1c..d7d959797a8fa 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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. diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index e913c4b644e98..91a16a1782644 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -99,7 +99,19 @@ class WebContents : public mate::TrackableObject, 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; diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index 60d04ff936bae..d16c5c9ace2bc 100644 --- a/atom/browser/api/atom_api_web_contents_view.cc +++ b/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() { diff --git a/spec/api-browser-view-spec.js b/spec/api-browser-view-spec.js index 968912a4abf3d..4aa4815024703 100644 --- a/spec/api-browser-view-spec.js +++ b/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) + }) + }) }) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index dc887948b236f..a382c228d4a58 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -1,6 +1,7 @@ 'use strict' const assert = require('assert') +const ChildProcess = require('child_process') const http = require('http') const path = require('path') const {closeWindow} = require('./window-helpers') @@ -667,6 +668,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') + assert.equal(code, 0) + }) + }) + // Destroying webContents in its event listener is going to crash when // Electron is built in Debug mode. xdescribe('destroy()', () => { diff --git a/spec/api-web-contents-view-spec.js b/spec/api-web-contents-view-spec.js index b61ddb6163868..d86d1e9eef8b3 100644 --- a/spec/api-web-contents-view-spec.js +++ b/spec/api-web-contents-view-spec.js @@ -1,8 +1,13 @@ 'use strict' const assert = require('assert') +const ChildProcess = require('child_process') +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 describe('WebContentsView', () => { let w = null @@ -22,4 +27,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') + assert.equal(code, 0) + }) + }) }) diff --git a/spec/fixtures/api/leak-exit-browserview.js b/spec/fixtures/api/leak-exit-browserview.js new file mode 100644 index 0000000000000..534fea59bbca7 --- /dev/null +++ b/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()) +}) diff --git a/spec/fixtures/api/leak-exit-webcontents.js b/spec/fixtures/api/leak-exit-webcontents.js new file mode 100644 index 0000000000000..5bf9e205793ce --- /dev/null +++ b/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()) +}) diff --git a/spec/fixtures/api/leak-exit-webcontentsview.js b/spec/fixtures/api/leak-exit-webcontentsview.js new file mode 100644 index 0000000000000..1196cfcca839b --- /dev/null +++ b/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()) +})