Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1872313003:
PlzNavigate: don't discard pending entry in DidFailProvisionalLoad (Closed)
|
Created:
4 years, 8 months ago by clamy Modified:
4 years, 8 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, asvitkine+watch_chromium.org, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: don't discard pending entry in DidFailProvisionalLoad
It is already done in FailedNavigation, we should not be doing it twice.
BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3ea589d5d24fac504b816a35a3bae26ea2849b6d
Cr-Commit-Position: refs/heads/master@{#387330}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase + addressed comments #
Total comments: 12
Patch Set 3 : Rebase + addressed comments #
Total comments: 1
Patch Set 4 : Rebase + removed the content_unittests filter #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in DidFailNavigation, we should not be doing it twice. BUG=439423 ========== to ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in DidFailNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in DidFailNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in DidFailNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org
@creis: PTAL
Yes, seems reasonable to only call it once. One thought about the kill logic below. (Do you want to move that to a separate CL? There's a risk this will have to be reverted if it causes too many kills, and that would undo your test fix as well.) https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_unittest.cc:425: SUCCEED() << "PlzNavigate: test not applicable."; I'm a bit nervous removing coverage for this without an equivalent PlzNavigate test, but I'll take your word for it. :) https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:231: // PlzNavigate: the entry has already been discarded in DidFailNavigation. s/DidFailNavigation/FailedNavigation/ (Same in CL description and throughout the CL.) https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:911: GetProcess(), bad_message::RFH_UNEXPECTED_FAIL_PROVISIONAL_LOAD); The conditions surrounding this will make it hard to diagnose if we see it fail in practice. We'll get reports if (1) there's no handle (whether we're in PlzNavigate or not) or (2) we're in PlzNavigate and the handle says net::OK. Those would be very different problems, so maybe we should have separate codes for them. That would also let us restructure this method to make it easier to read. Something like: if (!navigation_handle_) { ReceivedBadMessage(RFH_FAIL_PROVISIONAL_LOAD_NO_HANDLE) return; } if (PlzNavigate && net::OK) { ReceivedBadMessage(RFH_FAIL_PROVISIONAL_LOAD_NO_ERROR) return; } if (!PlzNavigate) set_net_error_code... ...DidFailProvisionalLoadWithError...
Description was changed from ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in DidFailNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in FailedNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks! I'm not sure about the killing logic. I had missed it could be executed in a non-PlzNavigate case. https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_unittest.cc:425: SUCCEED() << "PlzNavigate: test not applicable."; On 2016/04/11 21:10:57, Charlie Reis wrote: > I'm a bit nervous removing coverage for this without an equivalent PlzNavigate > test, but I'll take your word for it. :) I've made a few changes that enable the test to test something a bit similar on PlzNavigate (what if we start a new navigation in the middle of committing an error page, which would send a DidFailProvisionalLoad after we created a pending entry for the new navigation). https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:231: // PlzNavigate: the entry has already been discarded in DidFailNavigation. On 2016/04/11 21:10:57, Charlie Reis wrote: > s/DidFailNavigation/FailedNavigation/ > > (Same in CL description and throughout the CL.) Done. https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1872313003/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_impl.cc:911: GetProcess(), bad_message::RFH_UNEXPECTED_FAIL_PROVISIONAL_LOAD); On 2016/04/11 21:10:57, Charlie Reis wrote: > The conditions surrounding this will make it hard to diagnose if we see it fail > in practice. We'll get reports if (1) there's no handle (whether we're in > PlzNavigate or not) or (2) we're in PlzNavigate and the handle says net::OK. > Those would be very different problems, so maybe we should have separate codes > for them. > > That would also let us restructure this method to make it easier to read. > Something like: > > if (!navigation_handle_) { > ReceivedBadMessage(RFH_FAIL_PROVISIONAL_LOAD_NO_HANDLE) > return; > } > > if (PlzNavigate && net::OK) { > ReceivedBadMessage(RFH_FAIL_PROVISIONAL_LOAD_NO_ERROR) > return; > } > > if (!PlzNavigate) > set_net_error_code... > ...DidFailProvisionalLoadWithError... Done. It was actually hard enough to read that I though it would only trigger when PlzNavigate was enabled. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:426: GURL url_1("http://www.google.com/foo"); I had to make a few changes to make the test pass on PlzNavigate. First the failing navigation has to be same-process, otherwise the start of the second navigation will destroy the speculative RFH that is trying to commit the navigation. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:444: main_test_rfh()->SimulateNavigationError(url_1, net::ERR_TIMED_OUT); We need to get the navigation to fail here with PlzNavigate, and the error cannot be net::ERR_ABORTED, as we would not show an error page in that case. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:449: error.error_code = net::ERR_TIMED_OUT; Matching error here with the PlzNavigate case. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:454: main_test_rfh()->OnMessageReceived( We just want to simulate getting the DidFailProvisionalLoad IPC in both the current architecture of navigations and PlzNavigate. So I switched to just receiving the IPC, in order to not try do more work.
Great. I'm almost ready to approve, but I think you might have missed my high level question before: On 2016/04/11 21:10:58, Charlie Reis wrote: > One thought about the kill logic below. (Do you want to move that to a separate > CL? There's a risk this will have to be reverted if it causes too many kills, > and that would undo your test fix as well.) I do think it's worth splitting the kill logic into a separate CL if you can, since that sort of thing is always at high risk for revert. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:422: // page won't reset the pending entry of a navigation that just started. Thanks for adding a test for this! I feel like it might be better to have separate tests for the default case and the PlzNavigate case, even if there's a fair amount of copy/paste. (That's ok for tests.) Otherwise it gets hard to follow what's being tested in each mode, as I mention below. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:426: GURL url_1("http://www.google.com/foo"); On 2016/04/12 14:08:02, clamy wrote: > I had to make a few changes to make the test pass on PlzNavigate. First the > failing navigation has to be same-process, otherwise the start of the second > navigation will destroy the speculative RFH that is trying to commit the > navigation. Seems ok. (I'm not 100% sure whether the original test cared if this were cross-process, but I don't think it did.) https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:444: main_test_rfh()->SimulateNavigationError(url_1, net::ERR_TIMED_OUT); On 2016/04/12 14:08:02, clamy wrote: > We need to get the navigation to fail here with PlzNavigate, and the error > cannot be net::ERR_ABORTED, as we would not show an error page in that case. Acknowledged. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:449: error.error_code = net::ERR_TIMED_OUT; On 2016/04/12 14:08:02, clamy wrote: > Matching error here with the PlzNavigate case. Shouldn't it be ERR_ABORTED in the non-PlzNavigate case? https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:454: main_test_rfh()->OnMessageReceived( On 2016/04/12 14:08:02, clamy wrote: > We just want to simulate getting the DidFailProvisionalLoad IPC in both the > current architecture of navigations and PlzNavigate. So I switched to just > receiving the IPC, in order to not try do more work. Acknowledged. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:917: if (!IsBrowserSideNavigationEnabled() && navigation_handle_) { nit: We already know navigation_handle_ is non-null, or we would have returned on line 903.
Thanks! I've split the killing logic in https://codereview.chromium.org/1880423002/. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:422: // page won't reset the pending entry of a navigation that just started. On 2016/04/12 23:59:16, Charlie Reis wrote: > Thanks for adding a test for this! > > I feel like it might be better to have separate tests for the default case and > the PlzNavigate case, even if there's a fair amount of copy/paste. (That's ok > for tests.) Otherwise it gets hard to follow what's being tested in each mode, > as I mention below. Done. They are now separate, and I skip this one when PlzNavigate is enabled. https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1872313003/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:917: if (!IsBrowserSideNavigationEnabled() && navigation_handle_) { On 2016/04/12 23:59:17, Charlie Reis wrote: > nit: We already know navigation_handle_ is non-null, or we would have returned > on line 903. Addressed in the other patch.
Thanks, LGTM! Looks like two try jobs failed, but it's not clear to me if those are due to the CL. I'll run them again. The nit below is inconsequential, so if they pass I'll go ahead and land this for you. https://codereview.chromium.org/1872313003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1872313003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:473: // Tests hat receiving a DidFailProvisionalLoad from the renderer that is nit: s/hat/that/
On 2016/04/13 17:09:13, Charlie Reis wrote: > Thanks, LGTM! > > Looks like two try jobs failed, but it's not clear to me if those are due to the > CL. I'll run them again. The nit below is inconsequential, so if they pass > I'll go ahead and land this for you. There's some failures on linux_chromium_browser_side_navigation_rel that continue to look concerning, so I'll let you check them out. (Ignore the WebRTC failures on linux_site_isolation, which are unrelated to your CL.)
Thanks! There was an issue with content_unittests not being able to run with an empty filter, so I removed the filter. The other issue are layout tests, but I can't reproduce the failure locally, and the failures just make no sense to me based on this patch. I'm going to go ahead and land it, and see what happens on the bot.
The CQ bit was checked by clamy@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/1872313003/#ps60001 (title: "Rebase + removed the content_unittests filter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872313003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872313003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1872313003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1872313003/60001
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in FailedNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in FailedNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in FailedNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: don't discard pending entry in DidFailProvisionalLoad It is already done in FailedNavigation, we should not be doing it twice. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/3ea589d5d24fac504b816a35a3bae26ea2849b6d Cr-Commit-Position: refs/heads/master@{#387330} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3ea589d5d24fac504b816a35a3bae26ea2849b6d Cr-Commit-Position: refs/heads/master@{#387330} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
