Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revert: manual management of detached WebContents #19011

Merged
merged 1 commit into from
Jul 11, 2019
Merged

revert: manual management of detached WebContents #19011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions lib/browser/guest-view-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,6 @@ const createGuest = function (embedder, params) {
}

// Clear the guest from map when it is destroyed.
//
// The guest WebContents is usually destroyed in 2 cases:
// 1. The embedder frame is closed (reloaded or destroyed), and it
// automatically closes the guest frame.
// 2. The guest frame is detached dynamically via JS, and it is manually
// destroyed when the renderer sends the GUEST_VIEW_MANAGER_DESTROY_GUEST
// message.
// The second case relies on the libcc patch:
// https://github.com/electron/libchromiumcontent/pull/676
// The patch was introduced to work around a bug in Chromium:
// https://github.com/electron/electron/issues/14211
// We should revisit the bug to see if we can remove our libcc patch, the
// patch was introduced in Chrome 66.
guest.once('destroyed', () => {
if (guestInstanceId in guestInstances) {
detachGuest(embedder, guestInstanceId)
Expand Down Expand Up @@ -340,15 +327,6 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_CREATE_GUEST', function (event, param
return createGuest(event.sender, params)
})

handleMessage('ELECTRON_GUEST_VIEW_MANAGER_DESTROY_GUEST', function (event, guestInstanceId) {
try {
const guest = getGuestForWebContents(guestInstanceId, event.sender)
guest.detachFromOuterFrame()
} catch (error) {
console.error(`Guest destroy failed: ${error}`)
}
})

handleMessage('ELECTRON_GUEST_VIEW_MANAGER_ATTACH_GUEST', function (event, embedderFrameId, elementInstanceId, guestInstanceId, params) {
try {
attachGuest(event, embedderFrameId, elementInstanceId, guestInstanceId, params)
Expand Down
5 changes: 0 additions & 5 deletions lib/renderer/web-view/guest-view-internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,6 @@ export function createGuestSync (params: Record<string, any>): number {
return invokeSync('ELECTRON_GUEST_VIEW_MANAGER_CREATE_GUEST', params)
}

export function destroyGuest (guestInstanceId: number): void {
invoke('ELECTRON_GUEST_VIEW_MANAGER_DESTROY_GUEST', guestInstanceId)
}

export function attachGuest (
elementInstanceId: number, guestInstanceId: number, params: Record<string, any>, contentWindow: Window
) {
Expand All @@ -118,6 +114,5 @@ export const guestViewInternalModule = {
deregisterEvents,
createGuest,
createGuestSync,
destroyGuest,
attachGuest
}
1 change: 0 additions & 1 deletion lib/renderer/web-view/web-view-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ export class WebViewImpl {
// heard back from createGuest yet. We will not reset the flag in this case so
// that we don't end up allocating a second guest.
if (this.guestInstanceId) {
guestViewInternal.destroyGuest(this.guestInstanceId)
this.guestInstanceId = void 0
}

Expand Down
1 change: 0 additions & 1 deletion patches/chromium/.patches
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ enable_widevine.patch
chrome_key_systems.patch
allow_nested_error_trackers.patch
blink_initialization_order.patch
disable_detach_webview_frame.patch
ssl_security_state_tab_helper.patch
exclude-a-few-test-files-from-build.patch
expose-net-observer-api.patch
Expand Down
30 changes: 0 additions & 30 deletions patches/chromium/disable_detach_webview_frame.patch
View file
Open in desktop

This file was deleted.

11 changes: 3 additions & 8 deletions shell/browser/api/atom_api_web_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,12 +495,7 @@ WebContents::~WebContents() {

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

if (type_ == Type::WEB_VIEW) {
DCHECK(!web_contents()->GetOuterWebContents())
<< "Should never manually destroy an attached webview";
// For webview simply destroy the WebContents immediately.
DestroyWebContents(false /* async */);
} else if (type_ == Type::BROWSER_WINDOW && owner_window()) {
if (type_ == Type::BROWSER_WINDOW && owner_window()) {
// For BrowserWindow we should close the window and clean up everything
// before WebContents is destroyed.
for (ExtendedWebContentsObserver& observer : observers_)
Expand All @@ -514,7 +509,7 @@ WebContents::~WebContents() {
} else {
// Destroy WebContents asynchronously unless app is shutting down,
// because destroy() might be called inside WebContents's event handler.
DestroyWebContents(true /* async */);
DestroyWebContents(!IsGuest() /* async */);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure !IsGuest() is what we expect here. @zcbenz opinion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct.

// 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.
Expand Down Expand Up @@ -1190,7 +1185,7 @@ bool WebContents::OnMessageReceived(const IPC::Message& message) {
// 2. garbage collection;
// 3. user closes the window of webContents;
// 4. the embedder detaches the frame.
// For webview both #1 and #4 may happen, for BrowserWindow both #1 and #3 may
// For webview only #4 will happen, for BrowserWindow both #1 and #3 may
// happen. The #2 should never happen for webContents, because webview is
// managed by GuestViewManager, and BrowserWindow's webContents is managed
// by api::BrowserWindow.
Expand Down