From 1f5a6973fcbc839e2e1862801339ad7e778642dc Mon Sep 17 00:00:00 2001 From: Cezary Kulakowski Date: Fri, 21 Aug 2020 11:27:23 +0200 Subject: [PATCH] fix: window.open doesn't work correctly in child window If speculative render view host is deleted during naviagation to page we try to open with call to window.open window is destroyed right after it is created. It may happen when naviagation triggers redirect from http to https. To void that we should emit event current-render-view-deleted only when render view host which was destroyed is the current webcontent's rvh. --- .../browser/api/electron_api_web_contents.cc | 11 +------- shell/browser/api/electron_api_web_contents.h | 6 ----- spec-main/api-web-contents-spec.ts | 25 +++++++++++++++++++ 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index eb9adc9ebdb2b..37ea5d61c72d3 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -1065,22 +1065,13 @@ void WebContents::RenderFrameCreated( rwh_impl->disable_hidden_ = !background_throttling_; } -void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host, - content::RenderViewHost* new_host) { - currently_committed_process_id_ = new_host->GetProcess()->GetID(); -} - void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) { // This event is necessary for tracking any states with respect to // intermediate render view hosts aka speculative render view hosts. Currently // used by object-registry.js to ref count remote objects. Emit("render-view-deleted", render_view_host->GetProcess()->GetID()); - if (-1 == currently_committed_process_id_ || - render_view_host->GetProcess()->GetID() == - currently_committed_process_id_) { - currently_committed_process_id_ = -1; - + if (web_contents()->GetRenderViewHost() == render_view_host) { // When the RVH that has been deleted is the current RVH it means that the // the web contents are being closed. This is communicated by this event. // Currently tracked by guest-window-manager.js to destroy the diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index 05f8677f2e5ea..18fb629fe531b 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -555,8 +555,6 @@ class WebContents : public gin::Wrappable, const base::TimeTicks& proceed_time) override; void RenderViewCreated(content::RenderViewHost* render_view_host) override; void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override; - void RenderViewHostChanged(content::RenderViewHost* old_host, - content::RenderViewHost* new_host) override; void RenderViewDeleted(content::RenderViewHost*) override; void RenderProcessGone(base::TerminationStatus status) override; void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override; @@ -695,10 +693,6 @@ class WebContents : public gin::Wrappable, bool initially_shown_ = true; - // The ID of the process of the currently committed RenderViewHost. - // -1 means no speculative RVH has been committed yet. - int currently_committed_process_id_ = -1; - service_manager::BinderRegistryWithArgs registry_; mojo::ReceiverSet receivers_; diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 9040496cca095..39fa6c47aadb5 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -1156,6 +1156,10 @@ describe('webContents module', () => { res.end(); } else if (req.url === '/redirected') { res.end(''); + } else if (req.url === '/first-window-open') { + res.end(``); + } else if (req.url === '/second-window-open') { + res.end(''); } else { res.end(); } @@ -1192,6 +1196,27 @@ describe('webContents module', () => { expect(currentRenderViewDeletedEmitted).to.be.false('current-render-view-deleted was emitted'); }); + it('does not emit current-render-view-deleted when speculative RVHs are deleted and nativeWindowOpen is set to true', async () => { + const parentWindow = new BrowserWindow({ show: false, webPreferences: { nativeWindowOpen: true } }); + let currentRenderViewDeletedEmitted = false; + let childWindow:BrowserWindow; + const destroyed = emittedOnce(parentWindow.webContents, 'destroyed'); + const renderViewDeletedHandler = () => { + currentRenderViewDeletedEmitted = true; + }; + app.once('browser-window-created', (event, window) => { + childWindow = window; + window.webContents.on('current-render-view-deleted' as any, renderViewDeletedHandler); + }); + parentWindow.loadURL(`${serverUrl}/first-window-open`); + setTimeout(() => { + childWindow.webContents.removeListener('current-render-view-deleted' as any, renderViewDeletedHandler); + parentWindow.close(); + }, 500); + await destroyed; + expect(currentRenderViewDeletedEmitted).to.be.false('child window was destroyed'); + }); + it('emits current-render-view-deleted if the current RVHs are deleted', async () => { const w = new BrowserWindow({ show: false }); let currentRenderViewDeletedEmitted = false;