diff --git a/shell/browser/api/electron_api_browser_window.cc b/shell/browser/api/electron_api_browser_window.cc index bbc8ea563b72..2499ac3e6860 100644 --- a/shell/browser/api/electron_api_browser_window.cc +++ b/shell/browser/api/electron_api_browser_window.cc @@ -266,6 +266,9 @@ void BrowserWindow::OnCloseButtonClicked(bool* prevent_default) { // Already closed by renderer return; + // Required to make beforeunload handler work. + api_web_contents_->NotifyUserActivation(); + if (web_contents()->NeedToFireBeforeUnloadOrUnload()) web_contents()->DispatchBeforeUnload(false /* auto_cancel */); else diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index 82e4f779f499..7a7af3f2e3a1 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -756,6 +756,8 @@ void WebContents::BeforeUnloadFired(content::WebContents* tab, *proceed_to_fire_unload = proceed; else *proceed_to_fire_unload = true; + // Note that Chromium does not emit this for navigations. + Emit("before-unload-fired", proceed); } void WebContents::SetContentsBounds(content::WebContents* source, @@ -1546,6 +1548,9 @@ void WebContents::LoadURL(const GURL& url, // Calling LoadURLWithParams() can trigger JS which destroys |this|. auto weak_this = GetWeakPtr(); + // Required to make beforeunload handler work. + NotifyUserActivation(); + params.transition_type = ui::PAGE_TRANSITION_TYPED; params.should_clear_history_list = true; params.override_user_agent = content::NavigationController::UA_OVERRIDE_TRUE; @@ -2675,6 +2680,15 @@ void WebContents::GrantOriginAccess(const GURL& url) { url::Origin::Create(url)); } +void WebContents::NotifyUserActivation() { + auto* frame = web_contents()->GetMainFrame(); + if (!frame) + return; + mojo::AssociatedRemote renderer; + frame->GetRemoteAssociatedInterfaces()->GetInterface(&renderer); + renderer->NotifyUserActivation(); +} + v8::Local WebContents::TakeHeapSnapshot( const base::FilePath& file_path) { gin_helper::Promise promise(isolate()); diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index e3b9faba5cb6..646a6dd8fdd7 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -367,6 +367,9 @@ class WebContents : public gin_helper::TrackableObject, // the specified URL. void GrantOriginAccess(const GURL& url); + // Notifies the web page that there is user interaction. + void NotifyUserActivation(); + v8::Local TakeHeapSnapshot(const base::FilePath& file_path); // Properties. diff --git a/shell/common/api/api.mojom b/shell/common/api/api.mojom index 8ff1efd2d14f..be8758f70980 100644 --- a/shell/common/api/api.mojom +++ b/shell/common/api/api.mojom @@ -22,6 +22,8 @@ interface ElectronRenderer { string context_id, int32 object_id); + NotifyUserActivation(); + TakeHeapSnapshot(handle file) => (bool success); }; diff --git a/shell/renderer/electron_api_service_impl.cc b/shell/renderer/electron_api_service_impl.cc index 5999dd9c0bc8..24682657362a 100644 --- a/shell/renderer/electron_api_service_impl.cc +++ b/shell/renderer/electron_api_service_impl.cc @@ -236,6 +236,12 @@ void ElectronApiServiceImpl::DereferenceRemoteJSCallback( } #endif +void ElectronApiServiceImpl::NotifyUserActivation() { + blink::WebLocalFrame* frame = render_frame()->GetWebFrame(); + if (frame) + frame->NotifyUserActivation(); +} + void ElectronApiServiceImpl::TakeHeapSnapshot( mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) { diff --git a/shell/renderer/electron_api_service_impl.h b/shell/renderer/electron_api_service_impl.h index 1b48d10bbf36..8bcc3de2a359 100644 --- a/shell/renderer/electron_api_service_impl.h +++ b/shell/renderer/electron_api_service_impl.h @@ -39,6 +39,7 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer, void DereferenceRemoteJSCallback(const std::string& context_id, int32_t object_id) override; #endif + void NotifyUserActivation() override; void TakeHeapSnapshot(mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) override; diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts index 5f08fbb8c690..6aaa059ece53 100644 --- a/spec-main/api-browser-window-spec.ts +++ b/spec-main/api-browser-window-spec.ts @@ -7,7 +7,7 @@ import * as http from 'http'; import { AddressInfo } from 'net'; import { app, BrowserWindow, BrowserView, ipcMain, OnBeforeSendHeadersListenerDetails, protocol, screen, webContents, session, WebContents } from 'electron/main'; -import { emittedOnce } from './events-helpers'; +import { emittedOnce, emittedUntil } from './events-helpers'; import { ifit, ifdescribe } from './spec-helpers'; import { closeWindow, closeAllWindows } from './window-helpers'; @@ -38,6 +38,10 @@ const expectBoundsEqual = (actual: any, expected: any) => { } }; +const isBeforeUnload = (event: Event, level: number, message: string) => { + return (message === 'beforeunload'); +}; + describe('BrowserWindow module', () => { describe('BrowserWindow constructor', () => { it('allows passing void 0 as the webContents', async () => { @@ -95,16 +99,11 @@ describe('BrowserWindow module', () => { fs.unlinkSync(test); expect(String(content)).to.equal('unload'); }); + it('should emit beforeunload handler', async () => { await w.loadFile(path.join(fixtures, 'api', 'beforeunload-false.html')); - const beforeunload = new Promise(resolve => { - ipcMain.once('onbeforeunload', (e) => { - e.returnValue = null; - resolve(); - }); - }); w.close(); - await beforeunload; + await emittedOnce(w.webContents, 'before-unload-fired'); }); describe('when invoked synchronously inside navigation observer', () => { @@ -185,13 +184,11 @@ describe('BrowserWindow module', () => { fs.unlinkSync(test); expect(content).to.equal('close'); }); + it('should emit beforeunload event', async function () { - // TODO(nornagon): deflake this test. - this.retries(3); await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-false.html')); - w.webContents.executeJavaScript('run()', true); - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; + w.webContents.executeJavaScript('window.close()', true); + await emittedOnce(w.webContents, 'before-unload-fired'); }); }); @@ -2629,32 +2626,31 @@ describe('BrowserWindow module', () => { }); describe('beforeunload handler', function () { - // TODO(nornagon): I feel like these tests _oughtn't_ be flakey, but - // beforeunload is in general not reliable on the web, so i'm not going to - // worry about it too much for now. - this.retries(3); - let w: BrowserWindow = null as unknown as BrowserWindow; beforeEach(() => { w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } }); }); - afterEach(() => { - ipcMain.removeAllListeners('onbeforeunload'); - }); afterEach(closeAllWindows); - it('returning undefined would not prevent close', (done) => { - w.once('closed', () => { done(); }); - w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-undefined.html')); + + it('returning undefined would not prevent close', async () => { + await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-undefined.html')); + const wait = emittedOnce(w, 'closed'); + w.close(); + await wait; }); + it('returning false would prevent close', async () => { await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-false.html')); - w.webContents.executeJavaScript('run()', true); - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; + w.close(); + const [, proceed] = await emittedOnce(w.webContents, 'before-unload-fired'); + expect(proceed).to.equal(false); }); - it('returning empty string would prevent close', (done) => { - ipcMain.once('onbeforeunload', (e) => { e.returnValue = null; done(); }); - w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-empty-string.html')); + + it('returning empty string would prevent close', async () => { + await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-empty-string.html')); + w.close(); + const [, proceed] = await emittedOnce(w.webContents, 'before-unload-fired'); + expect(proceed).to.equal(false); }); it('emits for each close attempt', async () => { @@ -2663,46 +2659,16 @@ describe('BrowserWindow module', () => { const destroyListener = () => { expect.fail('Close was not prevented'); }; w.webContents.once('destroyed', destroyListener); - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); - { - const p = emittedOnce(ipcMain, 'onbeforeunload'); - w.close(); - const [e] = await p; - e.returnValue = null; - } - - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); - - // Hi future test refactorer! I don't know what event this timeout allows - // to occur, but without it, this test becomes flaky at this point and - // sometimes the window gets closed even though a `beforeunload` handler - // has been installed. I looked for events being emitted by the - // `webContents` during this timeout period and found nothing, so it - // might be some sort of internal timeout being applied by the content/ - // layer, or blink? - // - // In any case, this incantation reduces flakiness. I'm going to add a - // summoning circle for good measure. - // - // 🕯 🕯 - // 🕯 🕯 - // 🕯 🕯 - await new Promise(resolve => setTimeout(resolve, 1000)); - // 🕯 🕯 - // 🕯 🕯 - // 🕯 🕯 - - { - const p = emittedOnce(ipcMain, 'onbeforeunload'); - w.close(); - const [e] = await p; - e.returnValue = null; - } + await w.webContents.executeJavaScript('installBeforeUnload(2)', true); + w.close(); + await emittedOnce(w.webContents, 'before-unload-fired'); + w.close(); + await emittedOnce(w.webContents, 'before-unload-fired'); w.webContents.removeListener('destroyed', destroyListener); - const p = emittedOnce(w.webContents, 'destroyed'); + const wait = emittedOnce(w, 'closed'); w.close(); - await p; + await wait; }); it('emits for each reload attempt', async () => { @@ -2711,19 +2677,14 @@ describe('BrowserWindow module', () => { const navigationListener = () => { expect.fail('Reload was not prevented'); }; w.webContents.once('did-start-navigation', navigationListener); - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); + await w.webContents.executeJavaScript('installBeforeUnload(2)', true); w.reload(); - { - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; - } - - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); + // Chromium does not emit 'before-unload-fired' on WebContents for + // navigations, so we have to use other ways to know if beforeunload + // is fired. + await emittedUntil(w.webContents, 'console-message', isBeforeUnload); w.reload(); - { - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; - } + await emittedUntil(w.webContents, 'console-message', isBeforeUnload); w.webContents.removeListener('did-start-navigation', navigationListener); w.reload(); @@ -2736,19 +2697,14 @@ describe('BrowserWindow module', () => { const navigationListener = () => { expect.fail('Reload was not prevented'); }; w.webContents.once('did-start-navigation', navigationListener); - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); + await w.webContents.executeJavaScript('installBeforeUnload(2)', true); w.loadURL('about:blank'); - { - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; - } - - await w.webContents.executeJavaScript('preventNextBeforeUnload()', true); + // Chromium does not emit 'before-unload-fired' on WebContents for + // navigations, so we have to use other ways to know if beforeunload + // is fired. + await emittedUntil(w.webContents, 'console-message', isBeforeUnload); w.loadURL('about:blank'); - { - const [e] = await emittedOnce(ipcMain, 'onbeforeunload'); - e.returnValue = null; - } + await emittedUntil(w.webContents, 'console-message', isBeforeUnload); w.webContents.removeListener('did-start-navigation', navigationListener); w.loadURL('about:blank'); diff --git a/spec-main/api-web-contents-spec.ts b/spec-main/api-web-contents-spec.ts index 09991be4fba2..6e39d231a02d 100644 --- a/spec-main/api-web-contents-spec.ts +++ b/spec-main/api-web-contents-spec.ts @@ -42,23 +42,22 @@ describe('webContents module', () => { }); describe('will-prevent-unload event', function () { - // TODO(nornagon): de-flake this properly - this.retries(3); - afterEach(closeAllWindows); - it('does not emit if beforeunload returns undefined', (done) => { + it('does not emit if beforeunload returns undefined', async () => { const w = new BrowserWindow({ show: false }); - w.once('closed', () => done()); w.webContents.once('will-prevent-unload', () => { expect.fail('should not have fired'); }); - w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-undefined.html')); + await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-undefined.html')); + const wait = emittedOnce(w, 'closed'); + w.close(); + await wait; }); it('emits if beforeunload returns false', async () => { const w = new BrowserWindow({ show: false }); await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-false.html')); - w.webContents.executeJavaScript('run()', true); + w.close(); await emittedOnce(w.webContents, 'will-prevent-unload'); }); @@ -66,8 +65,9 @@ describe('webContents module', () => { const w = new BrowserWindow({ show: false }); w.webContents.once('will-prevent-unload', event => event.preventDefault()); await w.loadFile(path.join(__dirname, 'fixtures', 'api', 'close-beforeunload-false.html')); - w.webContents.executeJavaScript('run()', true); - await emittedOnce(w, 'closed'); + const wait = emittedOnce(w, 'closed'); + w.close(); + await wait; }); }); diff --git a/spec-main/fixtures/api/beforeunload-false-prevent3.html b/spec-main/fixtures/api/beforeunload-false-prevent3.html index 0a784e349163..39a744b8f065 100644 --- a/spec-main/fixtures/api/beforeunload-false-prevent3.html +++ b/spec-main/fixtures/api/beforeunload-false-prevent3.html @@ -1,15 +1,14 @@ diff --git a/spec-main/fixtures/api/close-beforeunload-empty-string.html b/spec-main/fixtures/api/close-beforeunload-empty-string.html index 9ae9c418fb30..671a2ec99189 100644 --- a/spec-main/fixtures/api/close-beforeunload-empty-string.html +++ b/spec-main/fixtures/api/close-beforeunload-empty-string.html @@ -4,16 +4,11 @@ // Only prevent unload on the first window close var unloadPrevented = false; window.onbeforeunload = function() { - setTimeout(function() { - require('electron').ipcRenderer.sendSync('onbeforeunload'); - }, 0); - if (!unloadPrevented) { unloadPrevented = true; return ''; } } - window.onload = () => window.close(); diff --git a/spec-main/fixtures/api/close-beforeunload-false.html b/spec-main/fixtures/api/close-beforeunload-false.html index 24f871f45479..6401ccc26945 100644 --- a/spec-main/fixtures/api/close-beforeunload-false.html +++ b/spec-main/fixtures/api/close-beforeunload-false.html @@ -1,23 +1,14 @@ diff --git a/spec-main/fixtures/api/close-beforeunload-undefined.html b/spec-main/fixtures/api/close-beforeunload-undefined.html index e5376623313a..e6eb2c13b28b 100644 --- a/spec-main/fixtures/api/close-beforeunload-undefined.html +++ b/spec-main/fixtures/api/close-beforeunload-undefined.html @@ -2,11 +2,7 @@ diff --git a/spec/fixtures/api/beforeunload-false.html b/spec/fixtures/api/beforeunload-false.html index 71c0d319c0f0..d9504a8c7ef3 100644 --- a/spec/fixtures/api/beforeunload-false.html +++ b/spec/fixtures/api/beforeunload-false.html @@ -4,9 +4,6 @@ // Only prevent unload on the first window close var unloadPrevented = false; window.onbeforeunload = function() { - setTimeout(function() { - require('electron').ipcRenderer.sendSync('onbeforeunload'); - }, 0); if (!unloadPrevented) { unloadPrevented = true; return false;