diff --git a/docs/api/browser-view.md b/docs/api/browser-view.md index 895534767b..a9dd89a33f 100644 --- a/docs/api/browser-view.md +++ b/docs/api/browser-view.md @@ -28,25 +28,6 @@ view.webContents.loadURL('https://electronjs.org') * `options` Object (optional) * `webPreferences` Object (optional) - See [BrowserWindow](browser-window.md). -### Static Methods - -#### `BrowserView.getAllViews()` - -Returns `BrowserView[]` - An array of all opened BrowserViews. - -#### `BrowserView.fromWebContents(webContents)` - -* `webContents` [WebContents](web-contents.md) - -Returns `BrowserView | null` - The BrowserView that owns the given `webContents` -or `null` if the contents are not owned by a BrowserView. - -#### `BrowserView.fromId(id)` - -* `id` Integer - -Returns `BrowserView` - The view with the given `id`. - ### Instance Properties Objects created with `new BrowserView` have the following properties: @@ -55,10 +36,6 @@ Objects created with `new BrowserView` have the following properties: A [`WebContents`](web-contents.md) object owned by this view. -#### `view.id` _Experimental_ - -A `Integer` representing the unique ID of the view. - ### Instance Methods Objects created with `new BrowserView` have the following instance methods: diff --git a/lib/browser/api/browser-view.ts b/lib/browser/api/browser-view.ts index 5dca1fd51f..45f7b0721f 100644 --- a/lib/browser/api/browser-view.ts +++ b/lib/browser/api/browser-view.ts @@ -1,15 +1,3 @@ -import { EventEmitter } from 'events'; - const { BrowserView } = process._linkedBinding('electron_browser_browser_view'); -Object.setPrototypeOf(BrowserView.prototype, EventEmitter.prototype); - -BrowserView.fromWebContents = (webContents: Electron.WebContents) => { - for (const view of BrowserView.getAllViews()) { - if (view.webContents.equal(webContents)) return view; - } - - return null; -}; - export default BrowserView; diff --git a/shell/browser/api/electron_api_base_window.cc b/shell/browser/api/electron_api_base_window.cc index e09b368b50..33c4874828 100644 --- a/shell/browser/api/electron_api_base_window.cc +++ b/shell/browser/api/electron_api_base_window.cc @@ -738,11 +738,11 @@ void BaseWindow::AddBrowserView(v8::Local value) { gin::Handle browser_view; if (value->IsObject() && gin::ConvertFromV8(isolate(), value, &browser_view)) { - auto get_that_view = browser_views_.find(browser_view->weak_map_id()); + auto get_that_view = browser_views_.find(browser_view->ID()); if (get_that_view == browser_views_.end()) { window_->AddBrowserView(browser_view->view()); browser_view->web_contents()->SetOwnerWindow(window_.get()); - browser_views_[browser_view->weak_map_id()].Reset(isolate(), value); + browser_views_[browser_view->ID()].Reset(isolate(), value); } } } @@ -751,7 +751,7 @@ void BaseWindow::RemoveBrowserView(v8::Local value) { gin::Handle browser_view; if (value->IsObject() && gin::ConvertFromV8(isolate(), value, &browser_view)) { - auto get_that_view = browser_views_.find(browser_view->weak_map_id()); + auto get_that_view = browser_views_.find(browser_view->ID()); if (get_that_view != browser_views_.end()) { window_->RemoveBrowserView(browser_view->view()); browser_view->web_contents()->SetOwnerWindow(nullptr); diff --git a/shell/browser/api/electron_api_browser_view.cc b/shell/browser/api/electron_api_browser_view.cc index 071dea2564..42ef7cc1b7 100644 --- a/shell/browser/api/electron_api_browser_view.cc +++ b/shell/browser/api/electron_api_browser_view.cc @@ -52,12 +52,24 @@ struct Converter { } // namespace gin +namespace { + +int32_t GetNextId() { + static int32_t next_id = 1; + return next_id++; +} + +} // namespace + namespace electron { namespace api { +gin::WrapperInfo BrowserView::kWrapperInfo = {gin::kEmbedderNativeGin}; + BrowserView::BrowserView(gin::Arguments* args, - const gin_helper::Dictionary& options) { + const gin_helper::Dictionary& options) + : id_(GetNextId()) { v8::Isolate* isolate = args->isolate(); gin_helper::Dictionary web_preferences = gin::Dictionary::CreateEmpty(isolate); @@ -72,8 +84,6 @@ BrowserView::BrowserView(gin::Arguments* args, view_.reset( NativeBrowserView::Create(api_web_contents_->managed_web_contents())); - - InitWithArgs(args); } BrowserView::~BrowserView() { @@ -87,24 +97,24 @@ BrowserView::~BrowserView() { void BrowserView::WebContentsDestroyed() { api_web_contents_ = nullptr; web_contents_.Reset(); + Unpin(); } // static -gin_helper::WrappableBase* BrowserView::New(gin_helper::ErrorThrower thrower, - gin::Arguments* args) { +gin::Handle BrowserView::New(gin_helper::ErrorThrower thrower, + gin::Arguments* args) { if (!Browser::Get()->is_ready()) { thrower.ThrowError("Cannot create BrowserView before app is ready"); - return nullptr; + return gin::Handle(); } gin::Dictionary options = gin::Dictionary::CreateEmpty(args->isolate()); args->GetNext(&options); - return new BrowserView(args, options); -} - -int32_t BrowserView::ID() const { - return weak_map_id(); + auto handle = + gin::CreateHandle(args->isolate(), new BrowserView(args, options)); + handle->Pin(args->isolate()); + return handle; } void BrowserView::SetAutoResize(AutoResizeFlags flags) { @@ -123,26 +133,25 @@ void BrowserView::SetBackgroundColor(const std::string& color_name) { view_->SetBackgroundColor(ParseHexColor(color_name)); } -v8::Local BrowserView::GetWebContents() { +v8::Local BrowserView::GetWebContents(v8::Isolate* isolate) { if (web_contents_.IsEmpty()) { - return v8::Null(isolate()); + return v8::Null(isolate); } - return v8::Local::New(isolate(), web_contents_); + return v8::Local::New(isolate, web_contents_); } // static -void BrowserView::BuildPrototype(v8::Isolate* isolate, - v8::Local prototype) { - prototype->SetClassName(gin::StringToV8(isolate, "BrowserView")); - gin_helper::Destroyable::MakeDestroyable(isolate, prototype); - gin_helper::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate()) +v8::Local BrowserView::FillObjectTemplate( + v8::Isolate* isolate, + v8::Local templ) { + return gin::ObjectTemplateBuilder(isolate, "BrowserView", templ) .SetMethod("setAutoResize", &BrowserView::SetAutoResize) .SetMethod("setBounds", &BrowserView::SetBounds) .SetMethod("getBounds", &BrowserView::GetBounds) .SetMethod("setBackgroundColor", &BrowserView::SetBackgroundColor) .SetProperty("webContents", &BrowserView::GetWebContents) - .SetProperty("id", &BrowserView::ID); + .Build(); } } // namespace api @@ -158,16 +167,9 @@ void Initialize(v8::Local exports, v8::Local context, void* priv) { v8::Isolate* isolate = context->GetIsolate(); - BrowserView::SetConstructor(isolate, base::BindRepeating(&BrowserView::New)); - gin_helper::Dictionary browser_view(isolate, - BrowserView::GetConstructor(isolate) - ->GetFunction(context) - .ToLocalChecked()); - browser_view.SetMethod("fromId", &BrowserView::FromWeakMapID); - browser_view.SetMethod("getAllViews", &BrowserView::GetAll); gin_helper::Dictionary dict(isolate, exports); - dict.Set("BrowserView", browser_view); + dict.Set("BrowserView", BrowserView::GetConstructor(context)); } } // namespace diff --git a/shell/browser/api/electron_api_browser_view.h b/shell/browser/api/electron_api_browser_view.h index 72ea427456..8e1ad0e9b5 100644 --- a/shell/browser/api/electron_api_browser_view.h +++ b/shell/browser/api/electron_api_browser_view.h @@ -10,9 +10,11 @@ #include "content/public/browser/web_contents_observer.h" #include "gin/handle.h" +#include "gin/wrappable.h" #include "shell/browser/native_browser_view.h" +#include "shell/common/gin_helper/constructible.h" #include "shell/common/gin_helper/error_thrower.h" -#include "shell/common/gin_helper/trackable_object.h" +#include "shell/common/gin_helper/pinnable.h" namespace gfx { class Rect; @@ -30,19 +32,25 @@ namespace api { class WebContents; -class BrowserView : public gin_helper::TrackableObject, +class BrowserView : public gin::Wrappable, + public gin_helper::Constructible, + public gin_helper::Pinnable, public content::WebContentsObserver { public: - static gin_helper::WrappableBase* New(gin_helper::ErrorThrower thrower, - gin::Arguments* args); + // gin_helper::Constructible + static gin::Handle New(gin_helper::ErrorThrower thrower, + gin::Arguments* args); + static v8::Local FillObjectTemplate( + v8::Isolate*, + v8::Local); - static void BuildPrototype(v8::Isolate* isolate, - v8::Local prototype); + // gin::Wrappable + static gin::WrapperInfo kWrapperInfo; WebContents* web_contents() const { return api_web_contents_; } NativeBrowserView* view() const { return view_.get(); } - int32_t ID() const; + int32_t ID() const { return id_; } protected: BrowserView(gin::Arguments* args, const gin_helper::Dictionary& options); @@ -56,13 +64,15 @@ class BrowserView : public gin_helper::TrackableObject, void SetBounds(const gfx::Rect& bounds); gfx::Rect GetBounds(); void SetBackgroundColor(const std::string& color_name); - v8::Local GetWebContents(); + v8::Local GetWebContents(v8::Isolate*); v8::Global web_contents_; class WebContents* api_web_contents_ = nullptr; std::unique_ptr view_; + int32_t id_; + DISALLOW_COPY_AND_ASSIGN(BrowserView); }; diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index 179391ca11..8fb7ee7597 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -276,6 +276,10 @@ void BrowserWindow::OnCloseButtonClicked(bool* prevent_default) { } void BrowserWindow::OnWindowClosed() { + // We need to reset the browser views before we call Cleanup, because the + // browser views are child views of the main web contents view, which will be + // deleted by Cleanup. + BaseWindow::ResetBrowserViews(); Cleanup(); // See BaseWindow::OnWindowClosed on why calling InvalidateWeakPtrs. weak_factory_.InvalidateWeakPtrs(); diff --git a/shell/common/gin_helper/constructible.h b/shell/common/gin_helper/constructible.h index 7222222019..7dc7074093 100644 --- a/shell/common/gin_helper/constructible.h +++ b/shell/common/gin_helper/constructible.h @@ -5,7 +5,9 @@ #ifndef SHELL_COMMON_GIN_HELPER_CONSTRUCTIBLE_H_ #define SHELL_COMMON_GIN_HELPER_CONSTRUCTIBLE_H_ +#include "gin/per_isolate_data.h" #include "gin/wrappable.h" +#include "shell/browser/event_emitter_mixin.h" #include "shell/common/gin_helper/function_template_extensions.h" namespace gin_helper { diff --git a/spec-main/api-browser-view-spec.ts b/spec-main/api-browser-view-spec.ts index 2c17badce8..366b18154b 100644 --- a/spec-main/api-browser-view-spec.ts +++ b/spec-main/api-browser-view-spec.ts @@ -1,9 +1,9 @@ import { expect } from 'chai'; -import * as ChildProcess from 'child_process'; import * as path from 'path'; import { emittedOnce } from './events-helpers'; -import { BrowserView, BrowserWindow } from 'electron/main'; +import { BrowserView, BrowserWindow, webContents } from 'electron/main'; import { closeWindow } from './window-helpers'; +import { defer, startRemoteControlApp } from './spec-helpers'; describe('BrowserView module', () => { const fixtures = path.resolve(__dirname, '..', 'spec', 'fixtures'); @@ -12,6 +12,7 @@ describe('BrowserView module', () => { let view: BrowserView; beforeEach(() => { + expect(webContents.getAllWebContents()).to.have.length(0); w = new BrowserWindow({ show: false, width: 400, @@ -23,27 +24,19 @@ describe('BrowserView module', () => { }); afterEach(async () => { + const p = emittedOnce(w.webContents, 'destroyed'); await closeWindow(w); + w = null as any; + await p; if (view) { - view.destroy(); + const p = emittedOnce(view.webContents, 'destroyed'); + (view.webContents as any).destroy(); + view = null as any; + await p; } - }); - describe('BrowserView.destroy()', () => { - it('does not throw', () => { - view = new BrowserView(); - view.destroy(); - }); - }); - - describe('BrowserView.isDestroyed()', () => { - it('returns correct value', () => { - view = new BrowserView(); - expect(view.isDestroyed()).to.be.false('view is destroyed'); - view.destroy(); - expect(view.isDestroyed()).to.be.true('view is destroyed'); - }); + expect(webContents.getAllWebContents()).to.have.length(0); }); describe('BrowserView.setBackgroundColor()', () => { @@ -119,7 +112,6 @@ describe('BrowserView module', () => { it('returns the set view', () => { view = new BrowserView(); w.setBrowserView(view); - expect(view.id).to.not.be.null('view id'); const view2 = w.getBrowserView(); expect(view2!.webContents.id).to.equal(view.webContents.id); @@ -134,11 +126,13 @@ describe('BrowserView module', () => { describe('BrowserWindow.addBrowserView()', () => { it('does not throw for valid args', () => { const view1 = new BrowserView(); + defer(() => (view1.webContents as any).destroy()); w.addBrowserView(view1); + defer(() => w.removeBrowserView(view1)); const view2 = new BrowserView(); + defer(() => (view2.webContents as any).destroy()); w.addBrowserView(view2); - view1.destroy(); - view2.destroy(); + defer(() => w.removeBrowserView(view2)); }); it('does not throw if called multiple times with same view', () => { view = new BrowserView(); @@ -160,18 +154,18 @@ describe('BrowserView module', () => { describe('BrowserWindow.getBrowserViews()', () => { it('returns same views as was added', () => { const view1 = new BrowserView(); + defer(() => (view1.webContents as any).destroy()); w.addBrowserView(view1); + defer(() => w.removeBrowserView(view1)); const view2 = new BrowserView(); + defer(() => (view2.webContents as any).destroy()); w.addBrowserView(view2); + defer(() => w.removeBrowserView(view2)); - expect(view1.id).to.be.not.null('view id'); const views = w.getBrowserViews(); expect(views).to.have.lengthOf(2); expect(views[0].webContents.id).to.equal(view1.webContents.id); expect(views[1].webContents.id).to.equal(view2.webContents.id); - - view1.destroy(); - view2.destroy(); }); }); @@ -188,46 +182,33 @@ describe('BrowserView module', () => { }); }); - describe('BrowserView.fromId()', () => { - it('returns the view with given id', () => { - view = new BrowserView(); - w.setBrowserView(view); - expect(view.id).to.not.be.null('view id'); - - const view2 = BrowserView.fromId(view.id); - expect(view2.webContents.id).to.equal(view.webContents.id); - }); - }); - - describe('BrowserView.fromWebContents()', () => { - it('returns the view with given id', () => { - view = new BrowserView(); - w.setBrowserView(view); - expect(view.id).to.not.be.null('view id'); - - const view2 = BrowserView.fromWebContents(view.webContents); - expect(view2!.webContents.id).to.equal(view.webContents.id); - }); - }); - - describe('BrowserView.getAllViews()', () => { - it('returns all views', () => { - view = new BrowserView(); - w.setBrowserView(view); - expect(view.id).to.not.be.null('view id'); - - const views = BrowserView.getAllViews(); - expect(views).to.be.an('array').that.has.lengthOf(1); - expect(views[0].webContents.id).to.equal(view.webContents.id); - }); - }); - - describe('new BrowserView()', () => { + describe('shutdown behavior', () => { it('does not crash on exit', async () => { - const appPath = path.join(__dirname, 'fixtures', 'api', 'leak-exit-browserview.js'); - const electronPath = process.execPath; - const appProcess = ChildProcess.spawn(electronPath, [appPath]); - const [code] = await emittedOnce(appProcess, 'exit'); + const rc = await startRemoteControlApp(); + await rc.remotely(() => { + const { BrowserView, app } = require('electron'); + new BrowserView({}) // eslint-disable-line + setTimeout(() => { + app.quit(); + }); + }); + const [code] = await emittedOnce(rc.process, 'exit'); + expect(code).to.equal(0); + }); + + it('does not crash on exit if added to a browser window', async () => { + const rc = await startRemoteControlApp(); + await rc.remotely(() => { + const { app, BrowserView, BrowserWindow } = require('electron'); + const bv = new BrowserView(); + bv.webContents.loadURL('about:blank'); + const bw = new BrowserWindow({ show: false }); + bw.addBrowserView(bv); + setTimeout(() => { + app.quit(); + }); + }); + const [code] = await emittedOnce(rc.process, 'exit'); expect(code).to.equal(0); }); }); diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts index 5d3f54396b..65d658b04c 100644 --- a/spec-main/api-browser-window-spec.ts +++ b/spec-main/api-browser-window-spec.ts @@ -9,7 +9,7 @@ import { AddressInfo } from 'net'; import { app, BrowserWindow, BrowserView, ipcMain, OnBeforeSendHeadersListenerDetails, protocol, screen, webContents, session, WebContents } from 'electron/main'; import { emittedOnce, emittedUntil } from './events-helpers'; -import { ifit, ifdescribe, delay } from './spec-helpers'; +import { ifit, ifdescribe, defer, delay } from './spec-helpers'; import { closeWindow, closeAllWindows } from './window-helpers'; const features = process._linkedBinding('electron_common_features'); @@ -1518,16 +1518,19 @@ describe('BrowserWindow module', () => { const w = new BrowserWindow({ show: false }); const bv = new BrowserView(); w.setBrowserView(bv); + defer(() => { + w.removeBrowserView(bv); + (bv.webContents as any).destroy(); + }); expect(BrowserWindow.fromBrowserView(bv)!.id).to.equal(w.id); - // if BrowserView isn't explicitly destroyed, it will crash in GC later - bv.destroy(); }); it('returns undefined if not attached', () => { const bv = new BrowserView(); + defer(() => { + (bv.webContents as any).destroy(); + }); expect(BrowserWindow.fromBrowserView(bv)).to.be.null('BrowserWindow associated with bv'); - // if BrowserView isn't explicitly destroyed, it will crash in GC later - bv.destroy(); }); }); diff --git a/spec-main/fixtures/api/leak-exit-browserview.js b/spec-main/fixtures/api/leak-exit-browserview.js deleted file mode 100644 index 16d347afb9..0000000000 --- a/spec-main/fixtures/api/leak-exit-browserview.js +++ /dev/null @@ -1,6 +0,0 @@ -const { BrowserView, app } = require('electron'); -app.whenReady().then(function () { - new BrowserView({}) // eslint-disable-line - - app.quit(); -}); diff --git a/spec-main/fixtures/apps/remote-control/main.js b/spec-main/fixtures/apps/remote-control/main.js index c960cac465..05d7e609a5 100644 --- a/spec-main/fixtures/apps/remote-control/main.js +++ b/spec-main/fixtures/apps/remote-control/main.js @@ -1,22 +1,25 @@ +const { app } = require('electron'); const http = require('http'); const v8 = require('v8'); -const server = http.createServer((req, res) => { - const chunks = []; - req.on('data', chunk => { chunks.push(chunk); }); - req.on('end', () => { - const js = Buffer.concat(chunks).toString('utf8'); - (async () => { - try { - const result = await Promise.resolve(eval(js)); // eslint-disable-line no-eval - res.end(v8.serialize({ result })); - } catch (e) { - res.end(v8.serialize({ error: e.stack })); - } - })(); +app.whenReady().then(() => { + const server = http.createServer((req, res) => { + const chunks = []; + req.on('data', chunk => { chunks.push(chunk); }); + req.on('end', () => { + const js = Buffer.concat(chunks).toString('utf8'); + (async () => { + try { + const result = await Promise.resolve(eval(js)); // eslint-disable-line no-eval + res.end(v8.serialize({ result })); + } catch (e) { + res.end(v8.serialize({ error: e.stack })); + } + })(); + }); + }).listen(0, '127.0.0.1', () => { + process.stdout.write(`Listening: ${server.address().port}\n`); }); -}).listen(0, '127.0.0.1', () => { - process.stdout.write(`Listening: ${server.address().port}\n`); }); setTimeout(() => { diff --git a/spec-main/spec-helpers.ts b/spec-main/spec-helpers.ts index 711d462cb2..ae25e2054c 100644 --- a/spec-main/spec-helpers.ts +++ b/spec-main/spec-helpers.ts @@ -53,7 +53,6 @@ class RemoteControlApp { req.end(); }); } - remotely = (script: Function, ...args: any[]): Promise => { return this.remoteEval(`(${script})(...${JSON.stringify(args)})`); }