Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738943002:
Move beforeunload hang timer duties to its own timer. (Closed)
|
Created:
3 years, 9 months ago by Avi (use Gerrit) Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove beforeunload hang timer duties to its own timer.
1. Just because a beforeunload dialog was closed, it doesn't mean that the frame
necessarily was waiting for a beforeunload ack. (If navigation happened in the
renderer entirely, it could decide to run beforeunload without the browser
knowing; the whole "waiting for beforeunload ack" deal is in the situation where
the browser forces the page to run a beforeunload handler if it has one.) Thus
blindly restarting the beforeunload ack timer just because it was a beforeunload
dialog was wrong. Thus this reverts r453323.
2. r453323 was trying to address a real problem: what if the parent frame was
waiting for a beforeunload ack but the child frame had the beforeunload handler
and showed the dialog? But the problem is more general, as _any_ parent frame
could have been asked to run beforeunload handlers and could have an ack
pending. So that's what's with the walking up the frame tree to both suspend the
timer when the dialog is shown, and to resume it when the dialog is closed.
BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2738943002
Cr-Commit-Position: refs/heads/master@{#457223}
Committed: https://chromium.googlesource.com/chromium/src/+/2d7d2c3632187e2cfaaa5c6ec26d3adb3ae784b0
Patch Set 1 #Patch Set 2 : rebase on top of another cl landing soon #Patch Set 3 : rebase to tot #Patch Set 4 : rebase #
Total comments: 13
Patch Set 5 : with test #
Total comments: 10
Patch Set 6 : better test #
Messages
Total messages: 43 (29 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
avi@chromium.org changed reviewers: + creis@chromium.org, dtapuska@chromium.org
This is a relanding of https://codereview.chromium.org/2727253005/. It's a combination of that CL, a revert of https://codereview.chromium.org/2704383007 because that was wrong, and new code in the dialog functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/08 23:02:24, Avi wrote: > This is a relanding of https://codereview.chromium.org/2727253005/. It's a > combination of that CL, a revert of https://codereview.chromium.org/2704383007 > because that was wrong, and new code in the dialog functions. I only see 1 patchset here, rather than one for the original and a diff with the fix. Can you explain what the problem was and what the diff is?
On 2017/03/09 00:51:34, Charlie Reis (OOO til Mar 8) wrote: > On 2017/03/08 23:02:24, Avi wrote: > > This is a relanding of https://codereview.chromium.org/2727253005/. It's a > > combination of that CL, a revert of https://codereview.chromium.org/2704383007 > > because that was wrong, and new code in the dialog functions. > > I only see 1 patchset here, rather than one for the original and a diff with the > fix. Can you explain what the problem was and what the diff is? Yes, sorry. The problem here is twofold. 1. Just because a beforeunload dialog was closed, it doesn't mean that the frame necessarily was waiting for a beforeunload ack. (If navigation happened in the renderer entirely, it could decide to run beforeunload without the browser knowing; the whole "waiting for beforeunload ack" deal is in the situation where the browser forces the page to run a beforeunload handler if it has one.) Thus blindly restarting the beforeunload ack timer just because it was a beforeunload dialog was wrong. Thus the revert of r453323. 2. r453323 was trying to address a real problem: what if the parent frame was waiting for a beforeunload ack but the child frame had the beforeunload handler and showed the dialog? But the problem is more general, as _any_ parent frame could have been asked to run beforeunload handlers and could have an ack pending. So that's what's with the walking up the frame tree to both suspend the timer when the dialog is shown, and to resume it when the dialog is closed.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the more detailed explanation-- that helps! Can you summarize that in the CL description for future reference? Given the subtlety here, I think it would be really useful to have a test where the timer is in a parent frame and the dialog is shown from a subframe. Is it possible to add that? Otherwise this looks mostly good. One possible concern about the is_waiting_for_beforeunload_ack_ state when closing dialogs below. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1701: // Stop any timers that are waiting. Yeah, that sounds right. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1702: for (RenderFrameHostImpl* frame = this; frame; frame = frame->GetParent()) nit: This would need braces for a multi-line body, unless we remove the conditional below (see my comment there). https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1703: if (frame->is_waiting_for_beforeunload_ack_) Is this check needed? I'm guessing that is_waiting_for_beforeunload_ack_ roughly corresponds to beforeunload_timeout_->IsRunning() now, and would even love to see us remove is_waiting_for_beforeunload_ack_ in favor of that if we can (perhaps as a followup). Whether we do that or not, though, I'm curious if there's a reason why we would potentially leave a frame's timer running if is_waiting_for_beforeunload_ack_ is false (in case they ever do differ). If not, calling Stop unconditionally seems simpler, since it's a no-op if the timer isn't running. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { Oh, I guess we can't remove this after all, since we're using it here to tell if a parent needs to restart its timer? Are we sure this is correct? It assumes that no JavaScriptDialogClosed calls will be made for normal dialogs whenever is_waiting_for_beforeunload_ack_ is true on a a frame or any of its ancestors. I could imagine a race where an OOPIF shows an alert dialog and a parent frame starts its beforeunload timer, with JavaScriptDialogClosed coming in for the subframe after that and before the real beforeunload dialog appears. Or is that not possible due to some invariant?
Description was changed from ========== Move beforeunload hang timer duties to its own timer. This also reverts r453323 as it was incorrect. BUG=418266 TEST=no user-visible change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move beforeunload hang timer duties to its own timer. 1. Just because a beforeunload dialog was closed, it doesn't mean that the frame necessarily was waiting for a beforeunload ack. (If navigation happened in the renderer entirely, it could decide to run beforeunload without the browser knowing; the whole "waiting for beforeunload ack" deal is in the situation where the browser forces the page to run a beforeunload handler if it has one.) Thus blindly restarting the beforeunload ack timer just because it was a beforeunload dialog was wrong. Thus this reverts r453323. 2. r453323 was trying to address a real problem: what if the parent frame was waiting for a beforeunload ack but the child frame had the beforeunload handler and showed the dialog? But the problem is more general, as _any_ parent frame could have been asked to run beforeunload handlers and could have an ack pending. So that's what's with the walking up the frame tree to both suspend the timer when the dialog is shown, and to resume it when the dialog is closed. BUG=418266 TEST=no user-visible change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1701: // Stop any timers that are waiting. On 2017/03/14 17:19:29, Charlie Reis (slow) wrote: > Yeah, that sounds right. Acknowledged. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1702: for (RenderFrameHostImpl* frame = this; frame; frame = frame->GetParent()) On 2017/03/14 17:19:29, Charlie Reis (slow) wrote: > nit: This would need braces for a multi-line body, unless we remove the > conditional below (see my comment there). Acknowledged. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1703: if (frame->is_waiting_for_beforeunload_ack_) On 2017/03/14 17:19:29, Charlie Reis (slow) wrote: > Is this check needed? I'm guessing that is_waiting_for_beforeunload_ack_ > roughly corresponds to beforeunload_timeout_->IsRunning() now If we don't have a dialog up, yes, is_waiting_for_beforeunload_ack_ == beforeunload_timeout_->IsRunning(). When we put up the dialog, we stop the timer and leave is_waiting_for_beforeunload_ack_ set, so we know that we have to restart the timer. > Whether we do that or not, though, I'm curious if there's a reason why we would > potentially leave a frame's timer running if is_waiting_for_beforeunload_ack_ is > false (in case they ever do differ). If not, calling Stop unconditionally seems > simpler, since it's a no-op if the timer isn't running. 1. There shouldn't be a case where the timer is running with is_waiting_for_beforeunload_ack_ false. 2. An unconditional Stop is indeed simpler. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { On 2017/03/14 17:19:29, Charlie Reis (slow) wrote: > Oh, I guess we can't remove this after all, since we're using it here to tell if > a parent needs to restart its timer? Yes. > Are we sure this is correct? No. This logic comes from pre-oopif days. It's correct in the easy case, but you tend to see the edge cases :) > It assumes that no JavaScriptDialogClosed calls > will be made for normal dialogs whenever is_waiting_for_beforeunload_ack_ is > true on a a frame or any of its ancestors. I could imagine a race where an > OOPIF shows an alert dialog and a parent frame starts its beforeunload timer, > with JavaScriptDialogClosed coming in for the subframe after that and before the > real beforeunload dialog appears. Or is that not possible due to some > invariant? That's probably possible. Lemme think hard about this.
https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1703: if (frame->is_waiting_for_beforeunload_ack_) FYI, before OldSpice, alerts were serialized in a FIFO queue, so this wouldn't be possible. With auto-dismissing dialogs, this is no longer guaranteed and that is what causes the problem here.
https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { ** this belongs to this thread ** FYI, before OldSpice, alerts were serialized in a FIFO queue, so this wouldn't be possible. With auto-dismissing dialogs, this is no longer guaranteed and that is what causes the problem here.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie: You were worried about some other call to RenderFrameHostImpl::JavaScriptDialogClosed restarting timers that they shouldn't. I believe that's impossible. 1. Suppose we have multiple frames with beforeunload. beforeunload dialogs use old-style dialogs, which are FIFO. Each old-style dialog is shown, and has to be dismissed before the next dialog is shown. Therefore, the timers will be stopped and restarted, stopped and restarted, but no crossing will happen. 2. Suppose the existing dialog is a JavaScript alert dialog, and then the beforeunload dialog is requested. You worry that the closing of the alert dialog might incorrectly trigger the restart of the beforeunload timer, but that can't happen. If we look at RenderFrameHostImpl::DispatchBeforeUnload, right after is_waiting_for_beforeunload_ack_ is set to true, whether the page is showing a dialog is shown, and in that case, an ACK is simulated and the page immediately closes. That's meant for the non-oopif case where the same renderer is showing the dialog that would have to show the beforeunload, but I don't personally mind being so aggressive.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. LGTM with the changes below. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { On 2017/03/14 21:03:14, Avi wrote: > ** this belongs to this thread ** > > FYI, before OldSpice, alerts were serialized in a FIFO queue, so this wouldn't > be possible. With auto-dismissing dialogs, this is no longer guaranteed and that > is what causes the problem here. On 2017/03/15 01:17:00, Avi wrote: > Charlie: > > You were worried about some other call to > RenderFrameHostImpl::JavaScriptDialogClosed restarting timers that they > shouldn't. I believe that's impossible. > > 1. Suppose we have multiple frames with beforeunload. beforeunload dialogs use > old-style dialogs, which are FIFO. Each old-style dialog is shown, and has to be > dismissed before the next dialog is shown. Therefore, the timers will be stopped > and restarted, stopped and restarted, but no crossing will happen. Is that true with OOPIFs, though? In my example above, an OOPIF shows an alert dialog. While that's showing, the parent frame (in a different renderer, unblocked) triggers a cross-process navigation via OpenURL (e.g., to an extension URL). Would that start the beforeunload on the parent frame before the alert is dismissed? If so, closing the alert might unexpectedly restart the timer. In retrospect, though, I think this probably can't happen, and may not even matter if it did. I don't think the parent frame renderer process can trigger is_waiting_for_beforeunload_ack_ even if it goes through OpenURL, right? And if the user navigates the tab (e.g., by editing the URL while the alert is visible, which is possible now), the subframe's dialog appears to be dismissed before we start beforeunload. (Is that right? I'm only checking on Canary, not testing in a debugger.) Even if the alert were dismissed after is_waiting_for_beforeunload_ack_ gets set on the parent, we're pretty much just restarting a 1 second timer, which shouldn't be a problem. (I suppose there's the dialog_was_suppressed case, but that's probably doing something sensible too.) Sorry for being paranoid here, but these are the kinds of things that lead to big hang monitor investigations down the road. :) > > 2. Suppose the existing dialog is a JavaScript alert dialog, and then the > beforeunload dialog is requested. You worry that the closing of the alert dialog > might incorrectly trigger the restart of the beforeunload timer, but that can't > happen. If we look at RenderFrameHostImpl::DispatchBeforeUnload, right after > is_waiting_for_beforeunload_ack_ is set to true, whether the page is showing a > dialog is shown, and in that case, an ACK is simulated and the page immediately > closes. That's meant for the non-oopif case where the same renderer is showing > the dialog that would have to show the beforeunload, but I don't personally mind > being so aggressive. Great, thanks for looking into it. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1714: if (frame->is_waiting_for_beforeunload_ack_) From your reply, I think you meant to remove this line? https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:255: // beforeunload dialog is shown. nit: shown by the subframe. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:263: DCHECK_EQ(true, main_frame->is_waiting_for_beforeunload_ack()); EXPECT_TRUE? https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:265: // Have the ACK dropped. Let's mention that the ACK won't be sent until the dialog is closed below. (At first, I wondered if this was a race with the navigation started above.) https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:272: EXPECT_TRUE(WaitForLoadStop(wc)); Let's put this WaitForLoadStop under the comment below, since this is the line that will time out. Can we also verify that the URL is web_ui_page afterward? We wouldn't want an unrelated load stop or navigation failure to make this test pass.
https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > Is that true with OOPIFs, though? In my example above, an OOPIF shows an alert > dialog. While that's showing, the parent frame (in a different renderer, > unblocked) triggers a cross-process navigation via OpenURL (e.g., to an > extension URL). Would that start the beforeunload on the parent frame before > the alert is dismissed? Yes. > If so, closing the alert might unexpectedly restart the timer. There is *no timer* in this case. Suppose the javascript alert dialog is up. The parent frame triggers a cross-process navigation. That navigation causes RenderFrameHostImpl::DispatchBeforeUnload to be called. RenderFrameHostImpl::DispatchBeforeUnload sets is_waiting_for_beforeunload_ack_ to be true, and *before starting the timer* checks to see if a dialog is being shown on the page. Yes there is, so *no timer* is started, and it calls SimulateBeforeUnloadAck, which proceeds with the navigation. > I don't think the parent frame renderer process can trigger > is_waiting_for_beforeunload_ack_ even if it goes through OpenURL, right? I believe it can set is_waiting_for_beforeunload_ack_, but there will be no timer and is_waiting_for_beforeunload_ack_ will be immediately reset, as I trace above. > And if > the user navigates the tab (e.g., by editing the URL while the alert is visible, > which is possible now), the subframe's dialog appears to be dismissed before we > start beforeunload. Navigating dismisses dialogs. I don't know offhand if this is before or after beforeunload. > Sorry for being paranoid here, but these are the kinds of things that lead to > big hang monitor investigations down the road. :) No worries. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1714: if (frame->is_waiting_for_beforeunload_ack_) On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > From your reply, I think you meant to remove this line? Yep, we might as well. I was so focused on working out a test and making sure that the timer confusion couldn't happen that I forgot to. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:255: // beforeunload dialog is shown. On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > nit: shown by the subframe. Done. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:263: DCHECK_EQ(true, main_frame->is_waiting_for_beforeunload_ack()); On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > EXPECT_TRUE? Done. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:265: // Have the ACK dropped. On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > Let's mention that the ACK won't be sent until the dialog is closed below. (At > first, I wondered if this was a race with the navigation started above.) Done. https://codereview.chromium.org/2738943002/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:272: EXPECT_TRUE(WaitForLoadStop(wc)); On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > Let's put this WaitForLoadStop under the comment below, since this is the line > that will time out. > > Can we also verify that the URL is web_ui_page afterward? We wouldn't want an > unrelated load stop or navigation failure to make this test pass. Good ideas.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM. https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2738943002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2626: if (frame->is_waiting_for_beforeunload_ack_) { On 2017/03/15 19:57:38, Avi wrote: > On 2017/03/15 17:22:02, Charlie Reis (slow) wrote: > > Is that true with OOPIFs, though? In my example above, an OOPIF shows an > alert > > dialog. While that's showing, the parent frame (in a different renderer, > > unblocked) triggers a cross-process navigation via OpenURL (e.g., to an > > extension URL). Would that start the beforeunload on the parent frame before > > the alert is dismissed? > > Yes. > > > If so, closing the alert might unexpectedly restart the timer. > > There is *no timer* in this case. > > Suppose the javascript alert dialog is up. The parent frame triggers a > cross-process navigation. That navigation causes > RenderFrameHostImpl::DispatchBeforeUnload to be called. > RenderFrameHostImpl::DispatchBeforeUnload sets is_waiting_for_beforeunload_ack_ > to be true, and *before starting the timer* checks to see if a dialog is being > shown on the page. Yes there is, so *no timer* is started, and it calls > SimulateBeforeUnloadAck, which proceeds with the navigation. Thanks-- that does explain it. And in particular, it checks with the *WebContents* whether a dialog is showing, so the parent frame would be able to notice the dialog even in the case an OOPIF put it up. Great. > > > I don't think the parent frame renderer process can trigger > > is_waiting_for_beforeunload_ack_ even if it goes through OpenURL, right? > > I believe it can set is_waiting_for_beforeunload_ack_, but there will be no > timer and is_waiting_for_beforeunload_ack_ will be immediately reset, as I trace > above. > > > And if > > the user navigates the tab (e.g., by editing the URL while the alert is > visible, > > which is possible now), the subframe's dialog appears to be dismissed before > we > > start beforeunload. > > Navigating dismisses dialogs. I don't know offhand if this is before or after > beforeunload. > > > Sorry for being paranoid here, but these are the kinds of things that lead to > > big hang monitor investigations down the road. :) > > No worries.
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489613779349440,
"parent_rev": "5e8b5e6aebcb564897e311ae4f4397eed3555457", "commit_rev":
"2d7d2c3632187e2cfaaa5c6ec26d3adb3ae784b0"}
Message was sent while issue was closed.
Description was changed from ========== Move beforeunload hang timer duties to its own timer. 1. Just because a beforeunload dialog was closed, it doesn't mean that the frame necessarily was waiting for a beforeunload ack. (If navigation happened in the renderer entirely, it could decide to run beforeunload without the browser knowing; the whole "waiting for beforeunload ack" deal is in the situation where the browser forces the page to run a beforeunload handler if it has one.) Thus blindly restarting the beforeunload ack timer just because it was a beforeunload dialog was wrong. Thus this reverts r453323. 2. r453323 was trying to address a real problem: what if the parent frame was waiting for a beforeunload ack but the child frame had the beforeunload handler and showed the dialog? But the problem is more general, as _any_ parent frame could have been asked to run beforeunload handlers and could have an ack pending. So that's what's with the walking up the frame tree to both suspend the timer when the dialog is shown, and to resume it when the dialog is closed. BUG=418266 TEST=no user-visible change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move beforeunload hang timer duties to its own timer. 1. Just because a beforeunload dialog was closed, it doesn't mean that the frame necessarily was waiting for a beforeunload ack. (If navigation happened in the renderer entirely, it could decide to run beforeunload without the browser knowing; the whole "waiting for beforeunload ack" deal is in the situation where the browser forces the page to run a beforeunload handler if it has one.) Thus blindly restarting the beforeunload ack timer just because it was a beforeunload dialog was wrong. Thus this reverts r453323. 2. r453323 was trying to address a real problem: what if the parent frame was waiting for a beforeunload ack but the child frame had the beforeunload handler and showed the dialog? But the problem is more general, as _any_ parent frame could have been asked to run beforeunload handlers and could have an ack pending. So that's what's with the walking up the frame tree to both suspend the timer when the dialog is shown, and to resume it when the dialog is closed. BUG=418266 TEST=no user-visible change CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2738943002 Cr-Commit-Position: refs/heads/master@{#457223} Committed: https://chromium.googlesource.com/chromium/src/+/2d7d2c3632187e2cfaaa5c6ec26d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2d7d2c3632187e2cfaaa5c6ec26d...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2782443002/ by thakis@chromium.org. The reason for reverting is: Speculative, might have caused https://crbug.com/705306. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
