|
|
Created:
4 years, 2 months ago by Avi (use Gerrit) Modified:
4 years, 2 months ago Reviewers:
Charlie Reis 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. |
DescriptionDon't wait to close tabs waiting for JavaScript dialogs.
They're known unresponsive, so nothing is gained.
BUG=629964
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/85ee8366930c2b8354c1ecfef99f2ff8fea4aa13
Cr-Commit-Position: refs/heads/master@{#424044}
Patch Set 1 #Patch Set 2 : fix? #Patch Set 3 : fix #
Total comments: 2
Patch Set 4 : simpler #Patch Set 5 : fix #Patch Set 6 : now a bit on webcontents, no timer fiddling #
Total comments: 2
Patch Set 7 : better #
Total comments: 4
Patch Set 8 : remove code no longer needed #
Messages
Total messages: 50 (37 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Description was changed from ========== Don't wait to close tabs waiting for JavaScript dialogs. They're known unresponsive, so nothing is gained. BUG=629964 ========== to ========== Don't wait to close tabs waiting for JavaScript dialogs. They're known unresponsive, so nothing is gained. BUG=629964 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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: 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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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.
avi@chromium.org changed reviewers: + creis@chromium.org
I'm wondering if a different approach would work, since I'd like to avoid adding SetHangMonitorKnownUnresponsive if we can. That hang monitor is a nightmare, and I've been hoping we can eventually extract the unload logic to not depend on it. This seems like it would make that harder. (It's also tough to reason about whether it's correct.) Are we seeing the delay for closing these tabs due to RenderViewHostImpl::ClosePage's StartHangMonitorTimeout? Maybe we can skip that instead if we know that a dialog is showing in that tab? https://codereview.chromium.org/2384813002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2384813002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:940: bool is_within_javascript_dialog_ = false; Any chance the delegate knows if the dialog is showing, and we can just ask it instead? I'm not opposed to adding this bit if needed, but it would be nice to avoid having even more state on RFH if possible. https://codereview.chromium.org/2384813002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/2384813002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:841: int known_unresponsive_renderer_count_ = 0; When would this be more than one?
On 2016/10/05 22:00:59, Charlie Reis wrote: > I'm wondering if a different approach would work, since I'd like to avoid adding > SetHangMonitorKnownUnresponsive if we can. That hang monitor is a nightmare, > and I've been hoping we can eventually extract the unload logic to not depend on > it. This seems like it would make that harder. (It's also tough to reason > about whether it's correct.) > > Are we seeing the delay for closing these tabs due to > RenderViewHostImpl::ClosePage's StartHangMonitorTimeout? Maybe we can skip that > instead if we know that a dialog is showing in that tab? There are two delays. The first one is RenderFrameHostImpl::DispatchBeforeUnload, and then when that times out, we run into the timeout in RenderViewHostImpl::ClosePage. The reason I pushed the timer to RenderWidgetHostImpl was because it was in those two places.
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_...) 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.
On 2016/10/06 18:38:23, Avi wrote: > On 2016/10/05 22:00:59, Charlie Reis wrote: > > I'm wondering if a different approach would work, since I'd like to avoid > adding > > SetHangMonitorKnownUnresponsive if we can. That hang monitor is a nightmare, > > and I've been hoping we can eventually extract the unload logic to not depend > on > > it. This seems like it would make that harder. (It's also tough to reason > > about whether it's correct.) > > > > Are we seeing the delay for closing these tabs due to > > RenderViewHostImpl::ClosePage's StartHangMonitorTimeout? Maybe we can skip > that > > instead if we know that a dialog is showing in that tab? > > There are two delays. > > The first one is RenderFrameHostImpl::DispatchBeforeUnload, and then when that > times out, we run into the timeout in RenderViewHostImpl::ClosePage. The reason > I pushed the timer to RenderWidgetHostImpl was because it was in those two > places. Ok. I think I'd prefer to see both of those places check whether dialogs are showing, since we want to extract both of them from the input event timer. (See https://crbug.com/636445.)
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.
Looking better! One question below, though. https://codereview.chromium.org/2384813002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2384813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: RenderWidgetHostDelegate::RENDERER_UNRESPONSIVE_DIALOG_SHOWING); Could we call OnBeforeUnloadACK(true, ...) here instead? I'm not sure it's correct to call RendererUnresponsive. I see that you would end up hitting the Close() block in there (by setting is_waiting_for_close_ack_ to true), but that only seems to make sense in the close case. We can get here for navigations as well, right? (I would also expect a similar check in RVH?)
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...
Please take a look. Much better. https://codereview.chromium.org/2384813002/diff/100001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2384813002/diff/100001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: RenderWidgetHostDelegate::RENDERER_UNRESPONSIVE_DIALOG_SHOWING); On 2016/10/07 22:44:23, Charlie Reis (OOO soon) wrote: > Could we call OnBeforeUnloadACK(true, ...) here instead? I was hoping to run past both issues with one call, but you're right that I should go more slowly. BTW that call is titled "RenderFrameHostImpl::SimulateBeforeUnloadAck()" and is called by the WC in RendererUnresponsive. > We can get here for navigations as well, right? Yes. D'oh. Sigh. > (I would also expect a similar check in RVH?) If we don't skip to always closing and mess up nav, yes.
Great! LGTM, though I don't think we need the new enum value anymore. https://codereview.chromium.org/2384813002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2384813002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:154: RENDERER_UNRESPONSIVE_DIALOG_SHOWING = 7, Don't need this anymore. https://codereview.chromium.org/2384813002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2384813002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95588: + <int value="7" label="Closing while JavaScript dialog shown"/> Don't need this anymore.
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
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/2384813002/#ps140001 (title: "remove code no longer needed")
https://codereview.chromium.org/2384813002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2384813002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_delegate.h:154: RENDERER_UNRESPONSIVE_DIALOG_SHOWING = 7, On 2016/10/08 00:20:27, Charlie Reis (Away till 10-14) wrote: > Don't need this anymore. Done. https://codereview.chromium.org/2384813002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2384813002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95588: + <int value="7" label="Closing while JavaScript dialog shown"/> On 2016/10/08 00:20:27, Charlie Reis (Away till 10-14) wrote: > Don't need this anymore. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Don't wait to close tabs waiting for JavaScript dialogs. They're known unresponsive, so nothing is gained. BUG=629964 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't wait to close tabs waiting for JavaScript dialogs. They're known unresponsive, so nothing is gained. BUG=629964 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/85ee8366930c2b8354c1ecfef99f2ff8fea4aa13 Cr-Commit-Position: refs/heads/master@{#424044} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/85ee8366930c2b8354c1ecfef99f2ff8fea4aa13 Cr-Commit-Position: refs/heads/master@{#424044} |