From b49c7c1a72ea5befcbf01db82489c9cf6c683064 Mon Sep 17 00:00:00 2001 From: samuelmaddock Date: Sun, 16 Feb 2020 19:25:08 -0500 Subject: [PATCH] fix(extensions): devtools now open for background pages refactor(extensions): remove unused InitWithBrowserContext method fix(extensions): release background page WebContents to avoid crash The background page WebContents instance is managed by the ExtensionHost. fix(extensions): open background page devtools detached by default test(extensions): add background page devtools test chore: test fix for null web_contents() fix: close background page devtools in test after opening --- .../browser/api/electron_api_web_contents.cc | 80 +++++++++++++++++-- shell/browser/api/electron_api_web_contents.h | 10 ++- .../electron_extensions_browser_client.h | 7 -- spec-main/extensions-spec.ts | 30 ++++--- 4 files changed, 103 insertions(+), 24 deletions(-) diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 6c7d07bcb598a..fccadab4012b5 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -132,6 +132,7 @@ #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) #include "extensions/browser/script_executor.h" +#include "extensions/browser/view_type_utils.h" #include "shell/browser/extensions/electron_extension_web_contents_observer.h" #endif @@ -409,12 +410,45 @@ const void* kElectronApiWebContentsKey = &kElectronApiWebContentsKey; } // namespace +#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) + +WebContents::Type GetTypeFromViewType(extensions::ViewType view_type) { + switch (view_type) { + case extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE: + return WebContents::Type::BACKGROUND_PAGE; + + case extensions::VIEW_TYPE_APP_WINDOW: + case extensions::VIEW_TYPE_COMPONENT: + case extensions::VIEW_TYPE_EXTENSION_DIALOG: + case extensions::VIEW_TYPE_EXTENSION_POPUP: + case extensions::VIEW_TYPE_BACKGROUND_CONTENTS: + case extensions::VIEW_TYPE_EXTENSION_GUEST: + case extensions::VIEW_TYPE_TAB_CONTENTS: + case extensions::VIEW_TYPE_INVALID: + return WebContents::Type::REMOTE; + } +} + +#endif + WebContents::WebContents(v8::Isolate* isolate, content::WebContents* web_contents) : content::WebContentsObserver(web_contents), type_(Type::REMOTE), id_(GetAllWebContents().Add(this)), weak_factory_(this) { +#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) + // WebContents created by extension host will have valid ViewType set. + extensions::ViewType view_type = extensions::GetViewType(web_contents); + if (view_type != extensions::VIEW_TYPE_INVALID) { + InitWithExtensionView(isolate, web_contents, view_type); + } + + extensions::ElectronExtensionWebContentsObserver::CreateForWebContents( + web_contents); + script_executor_.reset(new extensions::ScriptExecutor(web_contents)); +#endif + auto session = Session::CreateFrom(isolate, GetBrowserContext()); session_.Reset(isolate, session.ToV8()); @@ -424,11 +458,7 @@ WebContents::WebContents(v8::Isolate* isolate, web_contents->SetUserData(kElectronApiWebContentsKey, std::make_unique(GetWeakPtr())); InitZoomController(web_contents, gin::Dictionary::CreateEmpty(isolate)); -#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) - extensions::ElectronExtensionWebContentsObserver::CreateForWebContents( - web_contents); - script_executor_.reset(new extensions::ScriptExecutor(web_contents)); -#endif + registry_.AddInterface(base::BindRepeating(&WebContents::BindElectronBrowser, base::Unretained(this))); receivers_.set_disconnect_handler(base::BindRepeating( @@ -636,13 +666,39 @@ void WebContents::InitWithSessionAndOptions( std::make_unique(GetWeakPtr())); } +#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) +void WebContents::InitWithExtensionView(v8::Isolate* isolate, + content::WebContents* web_contents, + extensions::ViewType view_type) { + // Must reassign type prior to calling `Init`. + type_ = GetTypeFromViewType(view_type); + if (GetType() == Type::REMOTE) + return; + + // Allow toggling DevTools for background pages + Observe(web_contents); + InitWithWebContents(web_contents, GetBrowserContext(), IsGuest()); + managed_web_contents()->GetView()->SetDelegate(this); + SecurityStateTabHelper::CreateForWebContents(web_contents); +} +#endif + WebContents::~WebContents() { MarkDestroyed(); // The destroy() is called. if (managed_web_contents()) { +#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) + if (type_ == Type::BACKGROUND_PAGE) { + // Background pages are owned by extensions::ExtensionHost + managed_web_contents()->ReleaseWebContents(); + } +#endif + managed_web_contents()->GetView()->SetDelegate(nullptr); - RenderViewDeleted(web_contents()->GetRenderViewHost()); + if (web_contents()) { + RenderViewDeleted(web_contents()->GetRenderViewHost()); + } if (type_ == Type::BROWSER_WINDOW && owner_window()) { // For BrowserWindow we should close the window and clean up everything @@ -1379,6 +1435,7 @@ void WebContents::DevToolsOpened() { v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); v8::Locker locker(isolate); v8::HandleScope handle_scope(isolate); + DCHECK(managed_web_contents()); auto handle = FromOrCreate(isolate, managed_web_contents()->GetDevToolsWebContents()); devtools_web_contents_.Reset(isolate, handle.ToV8()); @@ -1720,7 +1777,8 @@ void WebContents::OpenDevTools(gin::Arguments* args) { return; std::string state; - if (type_ == Type::WEB_VIEW || !owner_window()) { + if (type_ == Type::WEB_VIEW || type_ == Type::BACKGROUND_PAGE || + !owner_window()) { state = "detach"; } bool activate = true; @@ -1731,6 +1789,8 @@ void WebContents::OpenDevTools(gin::Arguments* args) { options.Get("activate", &activate); } } + + DCHECK(managed_web_contents()); managed_web_contents()->SetDockState(state); managed_web_contents()->ShowDevTools(activate); } @@ -1739,6 +1799,7 @@ void WebContents::CloseDevTools() { if (type_ == Type::REMOTE) return; + DCHECK(managed_web_contents()); managed_web_contents()->CloseDevTools(); } @@ -1746,6 +1807,7 @@ bool WebContents::IsDevToolsOpened() { if (type_ == Type::REMOTE) return false; + DCHECK(managed_web_contents()); return managed_web_contents()->IsDevToolsViewShowing(); } @@ -1753,6 +1815,7 @@ bool WebContents::IsDevToolsFocused() { if (type_ == Type::REMOTE) return false; + DCHECK(managed_web_contents()); return managed_web_contents()->GetView()->IsDevToolsViewFocused(); } @@ -1761,6 +1824,7 @@ void WebContents::EnableDeviceEmulation( if (type_ == Type::REMOTE) return; + DCHECK(web_contents()); auto* frame_host = web_contents()->GetMainFrame(); if (frame_host) { auto* widget_host_impl = @@ -1778,6 +1842,7 @@ void WebContents::DisableDeviceEmulation() { if (type_ == Type::REMOTE) return; + DCHECK(web_contents()); auto* frame_host = web_contents()->GetMainFrame(); if (frame_host) { auto* widget_host_impl = @@ -1805,6 +1870,7 @@ void WebContents::InspectElement(int x, int y) { if (!enable_devtools_) return; + DCHECK(managed_web_contents()); if (!managed_web_contents()->GetDevToolsWebContents()) OpenDevTools(nullptr); managed_web_contents()->InspectElement(x, y); diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 0dcfbb9b556b2..022480195f219 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -48,6 +48,8 @@ #endif #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) +#include "extensions/common/view_type.h" + namespace extensions { class ScriptExecutor; } @@ -144,7 +146,7 @@ class WebContents : public gin::Wrappable, public mojom::ElectronBrowser { public: enum class Type { - BACKGROUND_PAGE, // A DevTools extension background page. + BACKGROUND_PAGE, // An extension background page. BROWSER_WINDOW, // Used by BrowserWindow. BROWSER_VIEW, // Used by BrowserView. REMOTE, // Thin wrap around an existing WebContents. @@ -453,6 +455,12 @@ class WebContents : public gin::Wrappable, gin::Handle session, const gin_helper::Dictionary& options); +#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) + void InitWithExtensionView(v8::Isolate* isolate, + content::WebContents* web_contents, + extensions::ViewType view_type); +#endif + // content::WebContentsDelegate: bool DidAddMessageToConsole(content::WebContents* source, blink::mojom::ConsoleMessageLevel level, diff --git a/shell/browser/extensions/electron_extensions_browser_client.h b/shell/browser/extensions/electron_extensions_browser_client.h index b0ffecf13b92e..a4379f03db37d 100644 --- a/shell/browser/extensions/electron_extensions_browser_client.h +++ b/shell/browser/extensions/electron_extensions_browser_client.h @@ -32,8 +32,6 @@ namespace electron { // An ExtensionsBrowserClient that supports a single content::BrowserContext // with no related incognito context. -// Must be initialized via InitWithBrowserContext() once the BrowserContext is -// created. class ElectronExtensionsBrowserClient : public extensions::ExtensionsBrowserClient { public: @@ -122,11 +120,6 @@ class ElectronExtensionsBrowserClient content::RenderFrameHost* render_frame_host, const extensions::Extension* extension) const override; - // |context| is the single BrowserContext used for IsValidContext(). - // |pref_service| is used for GetPrefServiceForContext(). - void InitWithBrowserContext(content::BrowserContext* context, - PrefService* pref_service); - // Sets the API client. void SetAPIClientForTest(extensions::ExtensionsAPIClient* api_client); diff --git a/spec-main/extensions-spec.ts b/spec-main/extensions-spec.ts index 113ddddde993e..4b50b9f33fa7d 100644 --- a/spec-main/extensions-spec.ts +++ b/spec-main/extensions-spec.ts @@ -317,16 +317,28 @@ describe('chrome extensions', () => { const receivedMessage = await w.webContents.executeJavaScript('window.completionPromise'); expect(receivedMessage).to.deep.equal({ some: 'message' }); }); - }); - it('has session in background page', async () => { - const customSession = session.fromPartition(`persist:${require('uuid').v4()}`); - await customSession.loadExtension(path.join(fixtures, 'extensions', 'persistent-background-page')); - const w = new BrowserWindow({ show: false, webPreferences: { session: customSession } }); - const promise = emittedOnce(app, 'web-contents-created'); - await w.loadURL('about:blank'); - const [, bgPageContents] = await promise; - expect(bgPageContents.session).to.not.equal(undefined); + it('has session in background page', async () => { + const customSession = session.fromPartition(`persist:${require('uuid').v4()}`); + await customSession.loadExtension(path.join(fixtures, 'extensions', 'persistent-background-page')); + const w = new BrowserWindow({ show: false, webPreferences: { session: customSession } }); + const promise = emittedOnce(app, 'web-contents-created'); + await w.loadURL('about:blank'); + const [, bgPageContents] = await promise; + expect(bgPageContents.session).to.not.equal(undefined); + }); + + it('can open devtools of background page', async () => { + const customSession = session.fromPartition(`persist:${require('uuid').v4()}`); + await customSession.loadExtension(path.join(fixtures, 'extensions', 'persistent-background-page')); + const w = new BrowserWindow({ show: false, webPreferences: { session: customSession } }); + const promise = emittedOnce(app, 'web-contents-created'); + await w.loadURL('about:blank'); + const [, bgPageContents] = await promise; + expect(bgPageContents.getType()).to.equal('backgroundPage'); + bgPageContents.openDevTools(); + bgPageContents.closeDevTools(); + }); }); describe('devtools extensions', () => {