[Attaching video file for real]

Deleted: bug_beforeunload.mov 3.0 MB

	bug_beforeunload.mov 3.0 MB View Download

I can repro on ChromeOS 69.0.3497.95.  Alex, can you take a look to see what might be causing this?

It looks like closing the window and even just navigating the red frame via DevTools correctly show the beforeunload handler, but something about the race with navigating the other frame might be canceling it.  (I'm curious if it worked without --site-per-process.)

Yes navigating both frames is part of the repro. Also I could not repro without --site-per-process (navigations wait until before unload is dismissed).

So it appears that this is where the before unload dialog gets dismissed:

https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_manager.cc?rcl=7099146dffef19375ed5330a2390fa94d8a2fb1f&l=376

which is when we have committed navigation for the other frame. I also noticed that the two navigations need not be immediately queued after each other. The other one could start a few seconds later and still as soon as it commits the dialog is dismissed.

I guess this is highly undesirable behavior. This is a simple script I could think of which keeps the user on the page forever (-ish).

<p> auto-dismiss </p>
<p> Tool </p>
<iframe></iframe>
<script>
let frame = window[0].frameElement;
let index = 0;
let url1 = "https://wikipedia.org";
let url2 = "https://ci.chromium.org/p/chromium/g/chromium/console";
window.setInterval( () => {
  frame.src =
    ( index % 2 === 0 ? url1 : url2) + "?q=foo" + index++;
}, 500);

window.onbeforeunload = (e) => {
  e.returnValue = "foo";
  return e;
};
</script>

The obvious workaround seems to be remembering the source of beforeunload (process) and block other navigations until the dialog is dismissed. But I am not sure if this is even correct (although seems compatible with the non-OOPIF case).

Thanks for reporting and analyzing this!  I can also repro this on Mac Canary.  Looks like CancelModalDialogsForRenderManager() was introduced in https://codereview.chromium.org/25434002 and was designed for main frame process swaps only, targeting alert/confirm dialogs put up by the old renderer.  Reading through some comments, it seems that we need to cancel dialogs only if they were put up by the process in which we need to swap out the old frame; otherwise the swap would hang.  So we could theoretically still allow navigations in another frame and process to go through, even while a dialog is up.  Don't know whether that's a good idea though, maybe blocking navigations in other processes while there's a beforeunload/alert/etc dialog makes more sense.  +avi@ for any thoughts.

My thoughts are that this is already a mess and that I'd be fine with any reasonable approach :(

 "  So we could theoretically still allow navigations in another frame and process to go through, even while a dialog is up"

This seems to fix this issue. Although there is still corner cases such as A->B->A which the process-based approach does not address. If B decides to navigate the inner most A and then the user wants to navigate away from the page the before unload dialog could still get dismissed if inner most A swaps out sooner.

To me it sounds like the safest approach is to block everything on a beforeunload.    

On a related note I think we should also expose (in WebContentsObserver) which frame this "beforeunload" refers to, no? My use case for MimeHandlerView is verifying if unloading a frame in a plugin element happens properly before we attach the frame to another WebContents. But I am thinking the call to BeforeUnloadFired might potentially be for a different frame than the one navigating to PDF altogether, right?

Comment 8: I think the path to WebContentsObserver::BeforeUnloadFired is only used when the tab is closing.  See https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_impl.cc?l=2257&rcl=9a0d2a34e84c0ad0e6f982bcf83fa7179e56567f, where the path to notify observers via RFHM is close-specific.  So in that sense, this can only be reached for the main frame.  The WCO comments probably need to be improved.