From 9f1b24de95d38f7d1a4c7937dc0d9d40d08e81f5 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 27 Aug 2020 18:03:36 -0700 Subject: [PATCH] refactor: wire will-navigate up to a navigation throttle instead of OpenURL (#25110) * refactor: wire will-navigate up to a navigation throttle instead of OpenURL (#25065) * refactor: wire will-navigate up to a navigation throttle instead of OpenURL * spec: add test for x-site _top navigation * chore: old code be old * Update api-browser-window-spec.ts --- .../browser/api/electron_api_web_contents.cc | 6 --- shell/browser/electron_navigation_throttle.cc | 24 ++++++++++ shell/browser/electron_navigation_throttle.h | 2 + spec-main/api-browser-window-spec.ts | 44 +++++++++++++++++-- 4 files changed, 67 insertions(+), 9 deletions(-) diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index f6738334d8652..27fe9d22c931e 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -723,12 +723,6 @@ content::WebContents* WebContents::OpenURLFromTab( return nullptr; } - // Give user a chance to cancel navigation. - if (Emit("will-navigate", params.url)) - return nullptr; - - // Don't load the URL if the web contents was marked as destroyed from a - // will-navigate event listener if (IsDestroyed()) return nullptr; diff --git a/shell/browser/electron_navigation_throttle.cc b/shell/browser/electron_navigation_throttle.cc index 5c0ef054421c3..09c6df6b3a846 100644 --- a/shell/browser/electron_navigation_throttle.cc +++ b/shell/browser/electron_navigation_throttle.cc @@ -19,6 +19,30 @@ const char* ElectronNavigationThrottle::GetNameForLogging() { return "ElectronNavigationThrottle"; } +content::NavigationThrottle::ThrottleCheckResult +ElectronNavigationThrottle::WillStartRequest() { + auto* handle = navigation_handle(); + auto* contents = handle->GetWebContents(); + if (!contents) { + NOTREACHED(); + return PROCEED; + } + + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); + auto api_contents = electron::api::WebContents::From(isolate, contents); + if (api_contents.IsEmpty()) { + // No need to emit any event if the WebContents is not available in JS. + return PROCEED; + } + + if (handle->IsRendererInitiated() && handle->IsInMainFrame() && + api_contents->EmitNavigationEvent("will-navigate", handle)) { + return CANCEL; + } + return PROCEED; +} + content::NavigationThrottle::ThrottleCheckResult ElectronNavigationThrottle::WillRedirectRequest() { auto* handle = navigation_handle(); diff --git a/shell/browser/electron_navigation_throttle.h b/shell/browser/electron_navigation_throttle.h index b7f96205ea5c8..6b74d59cec70e 100644 --- a/shell/browser/electron_navigation_throttle.h +++ b/shell/browser/electron_navigation_throttle.h @@ -14,6 +14,8 @@ class ElectronNavigationThrottle : public content::NavigationThrottle { explicit ElectronNavigationThrottle(content::NavigationHandle* handle); ~ElectronNavigationThrottle() override; + ElectronNavigationThrottle::ThrottleCheckResult WillStartRequest() override; + ElectronNavigationThrottle::ThrottleCheckResult WillRedirectRequest() override; diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts index d75c18e8feebd..a29779a91a6f8 100644 --- a/spec-main/api-browser-window-spec.ts +++ b/spec-main/api-browser-window-spec.ts @@ -10,7 +10,7 @@ import { AddressInfo } from 'net' import { app, BrowserWindow, BrowserView, ipcMain, OnBeforeSendHeadersListenerDetails, protocol, screen, webContents, session, WebContents } from 'electron' import { emittedOnce } from './events-helpers' -import { ifit, ifdescribe } from './spec-helpers' +import { delay, ifit, ifdescribe } from './spec-helpers' import { closeWindow } from './window-helpers' const { expect } = chai @@ -435,7 +435,13 @@ describe('BrowserWindow module', () => { let server = null as unknown as http.Server let url = null as unknown as string before((done) => { - server = http.createServer((req, res) => { res.end('') }) + server = http.createServer((req, res) => { + if (req.url === '/navigate-top') { + res.end('navigate _top'); + } else { + res.end(''); + } + }); server.listen(0, '127.0.0.1', () => { url = `http://127.0.0.1:${(server.address() as AddressInfo).port}/` done() @@ -495,7 +501,39 @@ describe('BrowserWindow module', () => { expect(navigatedTo).to.equal(url) expect(w.webContents.getURL()).to.equal('about:blank') }) - }) + + it('is triggered when a cross-origin iframe navigates _top', async () => { + await w.loadURL(`data:text/html,`); + await delay(1000); + w.webContents.debugger.attach('1.1'); + const targets = await w.webContents.debugger.sendCommand('Target.getTargets'); + const iframeTarget = targets.targetInfos.find((t: any) => t.type === 'iframe'); + const { sessionId } = await w.webContents.debugger.sendCommand('Target.attachToTarget', { + targetId: iframeTarget.targetId, + flatten: true + }); + await w.webContents.debugger.sendCommand('Input.dispatchMouseEvent', { + type: 'mousePressed', + x: 10, + y: 10, + clickCount: 1, + button: 'left' + }, sessionId); + await w.webContents.debugger.sendCommand('Input.dispatchMouseEvent', { + type: 'mouseReleased', + x: 10, + y: 10, + clickCount: 1, + button: 'left' + }, sessionId); + let willNavigateEmitted = false; + w.webContents.on('will-navigate', () => { + willNavigateEmitted = true; + }); + await emittedOnce(w.webContents, 'did-navigate'); + expect(willNavigateEmitted).to.be.true(); + }); + }); describe('will-redirect event', () => { let server = null as unknown as http.Server