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 ecc98c6cf8a60..e873f77af0ff2 100644 --- a/atom/browser/api/atom_api_browser_window.cc +++ b/atom/browser/api/atom_api_browser_window.cc @@ -385,7 +385,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 55eeae656465f..4f1fe3046f351 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/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()) { - 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. @@ -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 */); } } diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index a0f6b5c6f6997..984fc747d7783 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -106,7 +106,19 @@ class WebContents : public mate::TrackableObject, static void BuildPrototype(v8::Isolate* isolate, v8::Local 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); diff --git a/atom/browser/api/atom_api_web_contents_view.cc b/atom/browser/api/atom_api_web_contents_view.cc index 5abdac0a1ea1f..f0f45da98a898 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/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_) - 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 5dd1ea3615fde..5c27132aed902 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 7633146048a8d..1a9913828fe57 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 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()', () => { diff --git a/spec/api-web-contents-view-spec.js b/spec/api-web-contents-view-spec.js index 7f2008a75e35d..8610dda5a7878 100644 --- a/spec/api-web-contents-view-spec.js +++ b/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) + }) + }) }) 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()) +})