Skip to content
Permalink
Browse files

fix: destroy WebContents synchronously on shutdown

  • Loading branch information...
zcbenz committed Nov 2, 2018
1 parent 932ffdc commit 7416c2e3f0f7c3c2b872017c8b04c8df60640a4f
@@ -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
@@ -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,
@@ -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);
}

@@ -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"
@@ -492,19 +493,19 @@ WebContents::~WebContents() {

RenderViewDeleted(web_contents()->GetRenderViewHost());

if (type_ == WEB_VIEW) {
DestroyWebContents(false /* async */);
} else {
if (type_ == BROWSER_WINDOW && owner_window()) {
for (ExtendedWebContentsObserver& observer : observers_)
observer.OnCloseContents();
} else {
DestroyWebContents(true /* async */);
}
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();
// 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.
WebContentsDestroyed();
} else {
// Destroy WebContents asynchronously unless app is shutting down,
// because destroy() might be called inside WebContents's event handler.
DestroyWebContents(!Browser::Get()->is_shutting_down());
}
}
}
@@ -567,6 +568,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 */);
}
}
@@ -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);
@@ -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() {
@@ -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)
})
})
})
@@ -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()', () => {
@@ -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)
})
})
})
@@ -0,0 +1,6 @@
const { BrowserView, app } = require('electron')
app.on('ready', function () {
new BrowserView({}) // eslint-disable-line

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

process.nextTick(() => app.quit())
})
@@ -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())
})

0 comments on commit 7416c2e

Please sign in to comment.
You can’t perform that action at this time.