From 927e29e5d21f162eb5768d13cb9be19959a4c356 Mon Sep 17 00:00:00 2001 From: Samuel Maddock Date: Tue, 8 Sep 2020 07:55:40 -0400 Subject: [PATCH] fix(extensions): devtools now open for background pages (#22217) 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 59a9280a969ab..2cad029e6fa33 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -128,6 +128,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 @@ -388,11 +389,44 @@ base::string16 GetDefaultPrinterAsync() { } // 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), 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()); @@ -402,11 +436,7 @@ WebContents::WebContents(v8::Isolate* isolate, Init(isolate); AttachAsUserData(web_contents); 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))); bindings_.set_connection_error_handler(base::BindRepeating( @@ -611,12 +641,38 @@ void WebContents::InitWithSessionAndOptions( AttachAsUserData(web_contents()); } +#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() { // 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 @@ -1355,6 +1411,7 @@ void WebContents::DevToolsFocused() { void WebContents::DevToolsOpened() { 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()); @@ -1668,7 +1725,8 @@ void WebContents::OpenDevTools(gin_helper::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; @@ -1679,6 +1737,8 @@ void WebContents::OpenDevTools(gin_helper::Arguments* args) { options.Get("activate", &activate); } } + + DCHECK(managed_web_contents()); managed_web_contents()->SetDockState(state); managed_web_contents()->ShowDevTools(activate); } @@ -1687,6 +1747,7 @@ void WebContents::CloseDevTools() { if (type_ == Type::REMOTE) return; + DCHECK(managed_web_contents()); managed_web_contents()->CloseDevTools(); } @@ -1694,6 +1755,7 @@ bool WebContents::IsDevToolsOpened() { if (type_ == Type::REMOTE) return false; + DCHECK(managed_web_contents()); return managed_web_contents()->IsDevToolsViewShowing(); } @@ -1701,6 +1763,7 @@ bool WebContents::IsDevToolsFocused() { if (type_ == Type::REMOTE) return false; + DCHECK(managed_web_contents()); return managed_web_contents()->GetView()->IsDevToolsViewFocused(); } @@ -1709,6 +1772,7 @@ void WebContents::EnableDeviceEmulation( if (type_ == Type::REMOTE) return; + DCHECK(web_contents()); auto* frame_host = web_contents()->GetMainFrame(); if (frame_host) { auto* widget_host = @@ -1724,6 +1788,7 @@ void WebContents::DisableDeviceEmulation() { if (type_ == Type::REMOTE) return; + DCHECK(web_contents()); auto* frame_host = web_contents()->GetMainFrame(); if (frame_host) { auto* widget_host = @@ -1749,6 +1814,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 b4c36e2021566..b47a409dae4ea 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -43,6 +43,8 @@ #endif #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) +#include "extensions/common/view_type.h" + namespace extensions { class ScriptExecutor; } @@ -131,7 +133,7 @@ class WebContents : public gin_helper::TrackableObject, 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. @@ -410,6 +412,12 @@ class WebContents : public gin_helper::TrackableObject, 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 3d621ff055502..3c0c5f19e0877 100644 --- a/shell/browser/extensions/electron_extensions_browser_client.h +++ b/shell/browser/extensions/electron_extensions_browser_client.h @@ -30,8 +30,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: @@ -120,11 +118,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 8027bc59e8917..716fe271fc45a 100644 --- a/spec-main/extensions-spec.ts +++ b/spec-main/extensions-spec.ts @@ -312,16 +312,28 @@ ifdescribe(process.electronBinding('features').isExtensionsEnabled())('chrome ex 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', () => {