I've got a fix started for this at https://chromium-review.googlesource.com/c/chromium/src/+/1142826.  The problem is that when the OnRunBeforeUnloadConfirm IPC to show a second dialog comes while the first dialog is still up, it correctly refuses to show the second dialog due to the intervention of at most one dialog per navigation, but then it calls JavaScriptDialogClosed(), which incorrectly restarts the beforeunload timer.  When that timer fired in 1 sec, we proceeded with the navigation, even though the first dialog was still up.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd0427d5886ccf43e7ef8875067f67fe1e6f6749

commit fd0427d5886ccf43e7ef8875067f67fe1e6f6749
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Jul 20 00:28:24 2018

Don't restart beforeunload timer when multiple frames try to show dialogs.

This CL fixes a bug where multiple frames on a page both defined a
beforeunload handler leading to a dialog and the page was navigated
away.  First, one of the frames showed the beforeunload dialog.  Next,
another frame tried to show a beforeunload dialog before the user
interacted with the first dialog.  RFHI::OnRunBeforeUnloadConfirm()
dismissed that attempt due to the policy of at most one beforeunload
dialog per navigation.  However, as part of doing that, it called
JavaScriptDialogClosed() to proceed with the navigation, which
unconditionally restarted the beforeunload timer.  This led to the
navigation proceeding as soon as the timer fired (in 1 sec), despite
still having the beforeunload dialog up from the first frame.

This CL adds plumbing so that JavaScriptDialogClosed() doesn't try to
restart the beforeunload timers in that case.

Bug:  865223 
Change-Id: I1377f4891ebc86790941d7e815853ff24dda1cfd
Reviewed-on: https://chromium-review.googlesource.com/1142826
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576715}
[modify] https://crrev.com/fd0427d5886ccf43e7ef8875067f67fe1e6f6749/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/fd0427d5886ccf43e7ef8875067f67fe1e6f6749/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/fd0427d5886ccf43e7ef8875067f67fe1e6f6749/content/browser/frame_host/render_frame_host_impl_browsertest.cc

I've verified that the fix is working correctly in Mac Canary 69.0.3497.0.  When multiple frames try to show a dialog, the dialog isn't dismissed after one second.

Thanks Alex!  I can confirm the fix is working in Windows Canary 70.0.3500.0 (after r576715 landed in 69.0.3497.0).

The fix is safe and pretty straightforward, and the bug might be a bit ugly for pages that do define multiple beforeunload handlers-- the dialog gets shown and is then automatically dismissed.  In that sense, we may want to consider merge to M68 (though it is last minute).

On the flip side, Alex found a UMA stat showing that not many pages hit the case where multiple beforeunload dialogs attempt to be shown (i.e., the "multiple confirmation panels" case):
https://uma.googleplex.com/p/chrome/histograms/?endDate=20180601&dayCount=1&histograms=Document.BeforeUnloadDialog&fixupData=true&showMax=true&filters=platform%2Ceq%2CW%2Cchannel%2Ceq%2C4%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Alex is double checking that no new crashes have appeared since the fix landed, and then we can decide whether a merge makes sense.

Great, thanks for the quick fix. We've already triggered our candidate for stable. Please keep me posted how widespread this is, and in case if this is critical enough to retrigger build for tomorrow. 

Otherwise, let's let it bake in canary/dev this week and we can consider this for a future M68 respin. 

So far I haven't seen any new crashes in 69.0.3497.0+ that might've been caused by this.  I've also checked if there are any new bugs filed related to beforeunload, and so far there haven't been any.

I don't think this is critical enough to retrigger the stable candidate build, given that the UMA shows that not many pages hit this.  I do think it's a good candidate to include in M68 if there's a respin for another reason.  While the UMA shows this is very uncommon, the fix would be very valuable for the few cases that do hit this case, as it might prevent data loss.  An example of a case where this might matter would be if a page defines its own beforeunload dialog handler but also has an embedded Google Doc with unsaved comments.

I like that plan.  Abdul, are you ok with us merging the fix for whenever the next M68 build is?  I think it's probably fine to not have the fix in the initial release, given that we don't expect it to be frequently hit in practice.

This bug requires manual review: We are only 0 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Abdul: Friendly ping for merge request to M68, given the plans for an M68 respin.  I've tested a merge of r576715 to a local M68 branch and it passes the repro steps for this bug and the new test in the CL, which will be important for any pages with multiple beforeunload handlers.  It's been baking for a week since 69.0.3497.0.  Thanks!

Approving merge to M68. Branch:3440

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7055367a80d7b6bd954ab430f11a260869ef8058

commit 7055367a80d7b6bd954ab430f11a260869ef8058
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Jul 27 18:37:15 2018

Don't restart beforeunload timer when multiple frames try to show dialogs (Merge to M68)

This CL fixes a bug where multiple frames on a page both defined a
beforeunload handler leading to a dialog and the page was navigated
away.  First, one of the frames showed the beforeunload dialog.  Next,
another frame tried to show a beforeunload dialog before the user
interacted with the first dialog.  RFHI::OnRunBeforeUnloadConfirm()
dismissed that attempt due to the policy of at most one beforeunload
dialog per navigation.  However, as part of doing that, it called
JavaScriptDialogClosed() to proceed with the navigation, which
unconditionally restarted the beforeunload timer.  This led to the
navigation proceeding as soon as the timer fired (in 1 sec), despite
still having the beforeunload dialog up from the first frame.

This CL adds plumbing so that JavaScriptDialogClosed() doesn't try to
restart the beforeunload timers in that case.

TBR=alexmos@chromium.org

(cherry picked from commit fd0427d5886ccf43e7ef8875067f67fe1e6f6749)

Bug:  865223 
Change-Id: I1377f4891ebc86790941d7e815853ff24dda1cfd
Reviewed-on: https://chromium-review.googlesource.com/1142826
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#576715}
Reviewed-on: https://chromium-review.googlesource.com/1153486
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#762}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/7055367a80d7b6bd954ab430f11a260869ef8058/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/7055367a80d7b6bd954ab430f11a260869ef8058/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/7055367a80d7b6bd954ab430f11a260869ef8058/content/browser/frame_host/render_frame_host_impl_browsertest.cc