Skip to content

Commit

Permalink
fix(extensions): devtools now open for background pages (#25366)
Browse files Browse the repository at this point in the history 
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

Co-authored-by: samuelmaddock <samuel.maddock@gmail.com>
  • Loading branch information
@trop @samuelmaddock
trop[bot] and samuelmaddock committed Sep 10, 2020
1 parent 119cd24 commit 1e1d35f
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 24 deletions.
80 changes: 73 additions & 7 deletions shell/browser/api/electron_api_web_contents.cc
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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());

Expand All @@ -424,11 +458,7 @@ WebContents::WebContents(v8::Isolate* isolate,
web_contents->SetUserData(kElectronApiWebContentsKey,
std::make_unique<UserDataLink>(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(
Expand Down Expand Up @@ -636,13 +666,39 @@ void WebContents::InitWithSessionAndOptions(
std::make_unique<UserDataLink>(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
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -1739,20 +1799,23 @@ void WebContents::CloseDevTools() {
if (type_ == Type::REMOTE)
return;

DCHECK(managed_web_contents());
managed_web_contents()->CloseDevTools();
}

bool WebContents::IsDevToolsOpened() {
if (type_ == Type::REMOTE)
return false;

DCHECK(managed_web_contents());
return managed_web_contents()->IsDevToolsViewShowing();
}

bool WebContents::IsDevToolsFocused() {
if (type_ == Type::REMOTE)
return false;

DCHECK(managed_web_contents());
return managed_web_contents()->GetView()->IsDevToolsViewFocused();
}

Expand All @@ -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 =
Expand All @@ -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 =
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 9 additions & 1 deletion shell/browser/api/electron_api_web_contents.h
View file
Expand Up @@ -48,6 +48,8 @@
#endif

#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
#include "extensions/common/view_type.h"

namespace extensions {
class ScriptExecutor;
}
Expand Down Expand Up @@ -144,7 +146,7 @@ class WebContents : public gin::Wrappable<WebContents>,
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.
Expand Down Expand Up @@ -453,6 +455,12 @@ class WebContents : public gin::Wrappable<WebContents>,
gin::Handle<class Session> 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,
Expand Down
7 changes: 0 additions & 7 deletions shell/browser/extensions/electron_extensions_browser_client.h
View file
Expand Up @@ -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:
Expand Down Expand Up @@ -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);

Expand Down
30 changes: 21 additions & 9 deletions spec-main/extensions-spec.ts
View file
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 1e1d35f

Please sign in to comment.