Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704383007:
Always restart the hung page timer after a beforeunload dialog. (Closed)
|
Created:
3 years, 10 months ago by Avi (use Gerrit) Modified:
3 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, clamy Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways restart the hung page timer after a beforeunload dialog.
If a beforeunload dialog is triggered in a subframe, using is_waiting_for_beforeunload_ack_ as a sign that we're running a beforeunload dialog doesn't work.
BUG=695879
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2704383007
Cr-Commit-Position: refs/heads/master@{#453323}
Committed: https://chromium.googlesource.com/chromium/src/+/1c51fb6ff84c5df9dc9ac379e7ab9e9780fd165b
Patch Set 1 #
Total comments: 7
Patch Set 2 : fixes #
Dependent Patchsets: Messages
Total messages: 19 (10 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Description was changed from ========== Always restart the hung page timer after a beforeunload dialog. If a beforeunload dialog is triggered in a subframe, using is_waiting_for_beforeunload_ack_ as a sign that we're running a beforeunload dialog doesn't work. BUG=695879 ========== to ========== Always restart the hung page timer after a beforeunload dialog. If a beforeunload dialog is triggered in a subframe, using is_waiting_for_beforeunload_ack_ as a sign that we're running a beforeunload dialog doesn't work. BUG=695879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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.
avi@chromium.org changed reviewers: + creis@chromium.org
[+clamy to CC] Yes, I see what you mean-- different frames could be responsible for kicking off beforeunload vs showing the dialog. It's also a bit unintuitive at first why we need to restart the timer after the user has responded to the dialog-- the browser process knows the answer at that point and could technically proceed right away. At least today, though, we still rely on the renderer to get the dialog close IPC and then send the ACK, so it does make sense to restart the timer. Maybe someday we can find a safe way to optimize that part out. LGTM with nits. https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:2611: bool is_waiting = is_before_unload_dialog || IsWaitingForUnloadACK(); Could the same bug affect IsWaitingForUnloadACK()? Actually, I wonder if IsWaitingForUnloadACK() is basically dead code at this point, now that unload handlers can't show dialogs. I wonder if it's safe to remove that clause (maybe in a different CL, to avoid risk of regression here). https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:466: void JavaScriptDialogClosed(bool is_before_unload_dialog, nit: I'd suggest putting this down near dialog_was_suppressed, since it's more about the context in which the call was made than the primary arguments (reply_msg, success, user_input). https://codereview.chromium.org/2704383007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:5147: rfh->JavaScriptDialogClosed(is_showing_before_unload_dialog_, reply_msg, I suppose we could have avoided passing this in as a parameter if RFH asked its delegate for the value of is_showing_before_unload_dialog_. I think we should probably stick with your approach, though-- fewer dependencies in the delegate interface, and less tricky in the suppress_this_message case of RunBeforeUnloadConfirm above.
https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:2611: bool is_waiting = is_before_unload_dialog || IsWaitingForUnloadACK(); On 2017/02/27 18:41:41, Charlie Reis (slow) wrote: > Could the same bug affect IsWaitingForUnloadACK()? > > Actually, I wonder if IsWaitingForUnloadACK() is basically dead code at this > point, now that unload handlers can't show dialogs. I wonder if it's safe to > remove that clause (maybe in a different CL, to avoid risk of regression here). I just got my brain around is_waiting_for_beforeunload_ack_ and haven't yet around IsWaitingForUnloadACK(), so I don't know. If we can remove IsWaitingForUnloadACK() entirely I'm up for it. You're talking about removing IsWaitingForUnloadACK() just here, because we can no longer have dialogs during the time that we're waiting for an unload ack? https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.h:466: void JavaScriptDialogClosed(bool is_before_unload_dialog, On 2017/02/27 18:41:41, Charlie Reis (slow) wrote: > nit: I'd suggest putting this down near dialog_was_suppressed, since it's more > about the context in which the call was made than the primary arguments > (reply_msg, success, user_input). Done. https://codereview.chromium.org/2704383007/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:5147: rfh->JavaScriptDialogClosed(is_showing_before_unload_dialog_, reply_msg, On 2017/02/27 18:41:41, Charlie Reis (slow) wrote: > I suppose we could have avoided passing this in as a parameter if RFH asked its > delegate for the value of is_showing_before_unload_dialog_. I think we should > probably stick with your approach, though-- fewer dependencies in the delegate > interface, and less tricky in the suppress_this_message case of > RunBeforeUnloadConfirm above. Acknowledged.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2704383007/#ps20001 (title: "fixes")
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": 20001, "attempt_start_ts": 1488223804276810,
"parent_rev": "370d4c03e02c72d739151274d3eeb2959fa8383a", "commit_rev":
"1c51fb6ff84c5df9dc9ac379e7ab9e9780fd165b"}
Message was sent while issue was closed.
Description was changed from ========== Always restart the hung page timer after a beforeunload dialog. If a beforeunload dialog is triggered in a subframe, using is_waiting_for_beforeunload_ack_ as a sign that we're running a beforeunload dialog doesn't work. BUG=695879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Always restart the hung page timer after a beforeunload dialog. If a beforeunload dialog is triggered in a subframe, using is_waiting_for_beforeunload_ack_ as a sign that we're running a beforeunload dialog doesn't work. BUG=695879 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2704383007 Cr-Commit-Position: refs/heads/master@{#453323} Committed: https://chromium.googlesource.com/chromium/src/+/1c51fb6ff84c5df9dc9ac379e7ab... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1c51fb6ff84c5df9dc9ac379e7ab...
Message was sent while issue was closed.
https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2704383007/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:2611: bool is_waiting = is_before_unload_dialog || IsWaitingForUnloadACK(); On 2017/02/27 19:28:49, Avi wrote: > On 2017/02/27 18:41:41, Charlie Reis (slow) wrote: > > Could the same bug affect IsWaitingForUnloadACK()? > > > > Actually, I wonder if IsWaitingForUnloadACK() is basically dead code at this > > point, now that unload handlers can't show dialogs. I wonder if it's safe to > > remove that clause (maybe in a different CL, to avoid risk of regression > here). > > I just got my brain around is_waiting_for_beforeunload_ack_ and haven't yet > around IsWaitingForUnloadACK(), so I don't know. If we can remove > IsWaitingForUnloadACK() entirely I'm up for it. > > You're talking about removing IsWaitingForUnloadACK() just here, because we can > no longer have dialogs during the time that we're waiting for an unload ack? Yeah, throughout this method. We might want to sanity check that you can't get here during unload first.
Message was sent while issue was closed.
When did we stop allowing dialogs from unload? Is that done in the renderer?
Message was sent while issue was closed.
On 2017/02/27 21:57:09, Avi wrote: > When did we stop allowing dialogs from unload? Is that done in the renderer? Years ago, and yes, I think it's renderer code.
Message was sent while issue was closed.
On 2017/02/27 22:15:23, Charlie Reis (slow) wrote: > On 2017/02/27 21:57:09, Avi wrote: > > When did we stop allowing dialogs from unload? Is that done in the renderer? > > Years ago, and yes, I think it's renderer code. PTAL at https://codereview.chromium.org/2719133002/ that I sent. So in the dialog code, should we just return? (I'd BadMessage but I worry about IPCs legitimately crossing in the night.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
