test: use WebContents event to test beforeunload (#23699)

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

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<mojom::ElectronRenderer> renderer;
+  frame->GetRemoteAssociatedInterfaces()->GetInterface(&renderer);
+  renderer->NotifyUserActivation();
+}
+
 v8::Local<v8::Promise> WebContents::TakeHeapSnapshot(
     const base::FilePath& file_path) {
   gin_helper::Promise<void> promise(isolate());

shell/browser/api/electron_api_web_contents.h

@@ -367,6 +367,9 @@ class WebContents : public gin_helper::TrackableObject<WebContents>,
   // the specified URL.
   void GrantOriginAccess(const GURL& url);
 
+  // Notifies the web page that there is user interaction.
+  void NotifyUserActivation();
+
   v8::Local<v8::Promise> TakeHeapSnapshot(const base::FilePath& file_path);
 
   // Properties.

shell/common/api/api.mojom

@@ -22,6 +22,8 @@ interface ElectronRenderer {
     string context_id,
     int32 object_id);
 
+  NotifyUserActivation();
+
   TakeHeapSnapshot(handle file) => (bool success);
 };
 

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) {

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;
 

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');

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;
     });
   });
 

spec-main/fixtures/api/beforeunload-false-prevent3.html

@@ -1,15 +1,14 @@
 <html>
 <body>
 <script type="text/javascript" charset="utf-8">
-  function preventNextBeforeUnload() {
+  function installBeforeUnload(removeAfterNTimes) {
+    let count = 0
     window.addEventListener('beforeunload', function handler(e) {
-      e.preventDefault();
-      e.returnValue = '';
-      window.removeEventListener('beforeunload', handler)
-      setTimeout(function() {
-        require('electron').ipcRenderer.sendSync('onbeforeunload')
-      }, 0);
-      return false;
+      setTimeout(() => console.log('beforeunload'))
+      if (++count <= removeAfterNTimes) {
+        e.preventDefault();
+        e.returnValue = '';
+      }
     })
   }
 </script>

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();
 </script>
 </body>
 </html>

spec-main/fixtures/api/close-beforeunload-false.html

@@ -1,23 +1,14 @@
 <html>
 <body>
 <script type="text/javascript" charset="utf-8">
-  function run() {
-    // 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;
-      }
+  // Only prevent unload on the first window close
+  var unloadPrevented = false;
+  window.onbeforeunload = function() {
+    if (!unloadPrevented) {
+      unloadPrevented = true;
+      console.log('prevent')
+      return false;
     }
-    // unload events don't get run unless load events have run.
-    if (document.readyState === 'complete')
-      window.close()
-    else
-      window.onload = () => window.close()
   }
 </script>
 </body>

spec-main/fixtures/api/close-beforeunload-undefined.html

@@ -2,11 +2,7 @@
 <body>
 <script type="text/javascript" charset="utf-8">
   window.onbeforeunload = function() {
-    setTimeout(function() {
-      require('electron').ipcRenderer.sendSync('onbeforeunload');
-    }, 0);
   }
-  window.onload = () => window.close();
 </script>
 </body>
 </html>

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;