Window.onbeforeunload not working in Brave

[Sh1d0w]

Did you search for similar issues before submitting this one?
No

Describe the issue you encountered:
Create a webpage with the following js :

window.onbeforeunload = function(e) {
  var dialogText = 'Dialog text here';
  e.returnValue = dialogText;
  return dialogText;
};

If you try to navigate you are stuck in the page with no alert or nothing. In FF and Chrome it works ok, giving you an alert according to https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload

Note that the following code does not work on jsfiddle for some reason it has to be on a regular web page.

Expected behavior:

Brave should show an alert.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  All
• Brave Version:
  0.12.1
• Steps to reproduce:
  1. Open https://jsfiddle.net/L5grdj3a/
  2. Close the window
  3. You don't get alert as you should.

[willy-b]

FYI, not getting an alert from this jsfiddle in Firefox (47) either.

[Sh1d0w]

Edit: If you execute the following example code in FF and Chrome it will work:

window.onbeforeunload = function(e) {
  var dialogText = 'Dialog text here';
  e.returnValue = dialogText;
  return dialogText;
};

Brave does not seem to work if you execite this code in the console. For some reason the jsfiddle is not handling this event correctly so can't test it there.

I have updated the original issue.

[bridiver]

looks like we need to handle BeforeUnloadFired in electron

[Sh1d0w]

@bridiver Can you set a milestone for this issue, because all websites that take advantage of this callback function in order the user to confirm their navigation, are simply unusable since you can't navigate out of the page?

[bridiver]

this does seem like a pretty serious issue. I also noticed that the urlbar displays any domain you attempt to navigate to as if it were the current url which makes this a potential attack vector for urlbar spoofing @diracdeltas @bbondy

[bridiver]

actually I think it might be the result of a patch that electron applies to chromium. I'm not sure about it yet, but I came across it during the 54 upgrade

@@ -4652,6 +4659,7 @@ void WebContentsImpl::RendererUnresponsive(
   // We might have been waiting for both beforeunload and unload ACK.
   // Check if tab is to be unloaded first.
   if (rfhi->IsWaitingForUnloadACK()) {
+#if 0
     // Hang occurred while firing the unload handler.
     // Pretend the handler fired so tab closing continues as if it had.
     GetRenderViewHost()->set_sudden_termination_allowed(true);
@@ -4663,6 +4671,7 @@ void WebContentsImpl::RendererUnresponsive(
     // to recover. Pretend the unload listeners have all fired and close
     // the tab.
     Close();
+#endif
     return;
   }

[diracdeltas]

@darkdh would you mind looking at this?

[darkdh]

sure, no problem

[bridiver]

@darkdh I haven't test it yet, but I have a potential fix in the chromium54 branch

void WebContents::BeforeUnloadFired(content::WebContents* tab,
                                    bool proceed,
                                    bool* proceed_to_fire_unload) {
  if (type_ == BROWSER_WINDOW)
    *proceed_to_fire_unload = proceed;
  else
    *proceed_to_fire_unload = true;
}

[darkdh]

fixed by brave/muon@5f8f5a9

[luixxiul]

Test plan:

Open https://jsfiddle.net/L5grdj3a/
Press Run
Get alerted

[luixxiul]

Please update it to a correct one if it's wrong, thanks