New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 710062 link

Starred by 2 users
Status: Untriaged
Owner: ----
Cc:
clamy@chromium.org
arthurso...@chromium.org
█ nasko@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 737835



Sign in to add a comment

Don't go to the renderer to run beforeunload handlers if we know the frame doesn't have any

Project Member Reported by jam@chromium.org, Apr 10 2017 
Currently with PlzNavigate, we always go to the renderer to run beforeunload handlers. 95% of the time, there aren't beforeunload handlers. Per https://uma.googleplex.com/p/chrome/variations?sid=6eb8189f86c6e35590de4a5b225cf0ca, this would save 7ms in median, and 109ms at 95%.

I tried doing this in 
https://codereview.chromium.org/2783723002
(and followup https://codereview.chromium.org/2781383002)

but had to revert them because some code depended on the IPC channel being flushed. I fixed a bunch of tests (see  bug 705559 ) but the main reason I'm reverting is that it's possible for a didstoploading IPC for a previous commit to cancel the loads for the next navigation.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 10 2017 

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

commit 1d0058ca68075ed731ef2d3c150b9c3aba1534f6
Author: jam <jam@chromium.org>
Date: Mon Apr 10 21:28:17 2017

Revert of Followup comments from r460581. (patchset #1 id:1 of https://codereview.chromium.org/2781383002/ )

Reason for revert:
Exposed races with FrameHostMsg_DidStopLoading

BUG=710062

Original issue's description:
> Followup comments from r460581.
>
> BUG=365039,  705559 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2781383002
> Cr-Commit-Position: refs/heads/master@{#461012}
> Committed: https://chromium.googlesource.com/chromium/src/+/8393de464114cb0a3d5e82a93a491728aa49e043

TBR=nasko@chromium.org,dcheng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=365039,  705559 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2808953002
Cr-Commit-Position: refs/heads/master@{#463399}

[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/common/frame_messages.h
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/content/renderer/render_frame_impl.cc
[delete] https://crrev.com/ded1c1cc662ecee1ecfa4b41da7ccd9cc5a52129/content/test/data/page_with_empty_beforeunload.html
[modify] https://crrev.com/1d0058ca68075ed731ef2d3c150b9c3aba1534f6/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 11 2017 

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

commit 4044498152ad71d05c57aee559cba6a97db82375
Author: jam <jam@chromium.org>
Date: Tue Apr 11 03:01:41 2017

Revert of Keep track in the browser of which frames have onunload and onbeforeunload handlers. (patchset #10 id:180001 of https://codereview.chromium.org/2783723002/ )

Reason for revert:
Exposed races with FrameHostMsg_DidStopLoading

BUG=710062

Original issue's description:
> Keep track in the browser of which frames have onunload and onbeforeunload handlers.
>
> This allows PlzNavigate to only pause fetching the request if it knows that the page has an onbeforeunload handler, where today it currently always goes to the renderer. This avoids delaying the network requests in the 95% cases that don't onbeforeunload handler on a process hop to the renderer.
>
> BUG=365039, 705559 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
> R=dcheng@chromium.org, nasko@chromium.org
>
> Review-Url: https://codereview.chromium.org/2783723002 .
> Cr-Commit-Position: refs/heads/master@{#460581}
> Committed: https://chromium.googlesource.com/chromium/src/+/a2c84a7664eceb5cbeaae5bf1ec32da0060e3097

TBR=nasko@chromium.org,dcheng@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=365039, 705559 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2812743002
Cr-Commit-Position: refs/heads/master@{#463510}

[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/chrome/browser/lifetime/browser_close_manager.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/browser_side_navigation_browsertest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/browser/web_contents/web_contents_impl_unittest.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/common/frame_messages.h
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/renderer/render_frame_impl.h
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/content/test/test_render_frame_host.h
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/4044498152ad71d05c57aee559cba6a97db82375/ui/views/controls/webview/web_dialog_view.cc

Comment 3 by horo@chromium.org, Jun 30 2017

Blocking: 737835

Comment 4 by avi@google.com, Nov 20 2017 

Probably a dup of issue 365039.
Project Member

Comment 5 by sheriffbot@chromium.org, Nov 21

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

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

Sign in to add a comment