Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136873002:
Clear child FrameNavigationEntries if a history navigation redirects. (Closed)
|
Created:
4 years, 5 months ago by Charlie Reis Modified:
3 years, 11 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, blink-reviews, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/913c63ce72b8783efda4971ff35ddbc01c8b69a6
Cr-Commit-Position: refs/heads/master@{#405948}
Patch Set 1 #Patch Set 2 : Update tests and fixes #Patch Set 3 : Rebase #Patch Set 4 : Clean up and disable test in --site-per-process. #
Total comments: 19
Patch Set 5 : Update test. #Patch Set 6 : Disable test on PlzNavigate. #
Messages
Total messages: 37 (19 generated)
Description was changed from
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
==========
to
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
The CQ bit was checked by creis@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
creis@chromium.org changed reviewers: + nasko@chromium.org
Nasko, can you take a look? There's a couple different bugs here being fixed, but I've left some explanations below. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1324: // TODO(creis): Update the FrameNavigationEntry in --site-per-process. This was buggy for redirects before, and the new test covers it. Now we update the FNE below. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1344: // See https://crbug.com/495161. This is just stale code now that https://crbug.com/495161 is fixed. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1189: entry->ClearChildren(rfh->frame_tree_node()); This handles the main frame case. In the old path, we did it in RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad, but doing it at commit time is better: we don't have to worry about whether there's a pending entry, and it handles location.replace as well. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1348: last_committed->AddOrUpdateFrameEntry( We clear the children for subframe redirects inside AddOrUpdateFrameEntry. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (left): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:4700: if (!SiteIsolationPolicy::UseSubframeNavigationEntries()) This is fixed now as well. :) https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5069: SubframeForwardRedirect) { Tossed this test in as a simpler sanity check that back/forwards with redirects work in all modes (and in the right process in --site-per-process). It doesn't catch https://crbug.com/628782. https://codereview.chromium.org/2136873002/diff/60001/content/renderer/histor... File content/renderer/history_controller.cc (right): https://codereview.chromium.org/2136873002/diff/60001/content/renderer/histor... content/renderer/history_controller.cc:186: } This is actually the same fix I made to UpdateForCommit earlier this week in https://codereview.chromium.org/2144823002/. I didn't realize at the time that the InitialLoadInChildFrame back/forward case had the same bug, so I'm fixing it here as well. (The new test in this CL pointed this out.) Note: The previous CL was fixing a case that caused NC_AUTO_SUBFRAME kills. I don't think this instance of the bug can cause that, because the main frame won't ever look cross-origin. As a result, I don't think it's necessary to pull this change out into a separate CL for merging. https://codereview.chromium.org/2136873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions (left): https://codereview.chromium.org/2136873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/FlagExpectations/isolate-extensions:5: http/tests/navigation/back-to-redirect-with-frame.php [ Failure ] Heh, I almost just enabled this test and called it a day. Glad I wrote the content_browsertest as well, since it uncovered about 5 other bugs to fix, across default mode, --isolate-extensions, and --site-per-process. :)
LGTM with a few comments. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1344: // See https://crbug.com/495161. On 2016/07/15 22:42:59, Charlie Reis wrote: > This is just stale code now that https://crbug.com/495161 is fixed. Nice! https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2961: NavigateToURL(shell(), initial_url); nit: EXPECT_TRUE https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2979: FrameNavigateParamsCapturer capturer(root->child_at(0)); No need for FrameNavigateParamsCapturer, since NavigateFrameToURL will wait for the load to complete. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3037: NavigateToURL(shell(), url2); EXPECT_TRUE https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3148: EXPECT_EQ(2, controller.GetLastCommittedEntryIndex()); This is one heck of a test : ). I wonder if it will manage to execute in the usual time and not create timeouts that cause it to be flaky. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5102: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); You use TestNavigationObserver in nested scope in all other history navigations. Is there a good reason not to do it here too?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
creis@chromium.org changed reviewers: + clamy@chromium.org
Thanks! I also had to disable the test in PlzNavigate, since it's failing a bunch there in step 4. TBR'ing clamy@ to let her know. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2961: NavigateToURL(shell(), initial_url); On 2016/07/15 23:21:54, nasko wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:2979: FrameNavigateParamsCapturer capturer(root->child_at(0)); On 2016/07/15 23:21:54, nasko wrote: > No need for FrameNavigateParamsCapturer, since NavigateFrameToURL will wait for > the load to complete. Done. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3037: NavigateToURL(shell(), url2); On 2016/07/15 23:21:54, nasko wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3148: EXPECT_EQ(2, controller.GetLastCommittedEntryIndex()); On 2016/07/15 23:21:54, nasko wrote: > This is one heck of a test : ). I wonder if it will manage to execute in the > usual time and not create timeouts that cause it to be flaky. Hope so. I can split it up later if needed. https://codereview.chromium.org/2136873002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5102: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); On 2016/07/15 23:21:54, nasko wrote: > You use TestNavigationObserver in nested scope in all other history navigations. > Is there a good reason not to do it here too? Nope, better to be consistent.
Description was changed from
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
The CQ bit was checked by creis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2136873002/#ps40002 (title: "Disable test on PlzNavigate.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Description was changed from
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
Message was sent while issue was closed.
Committed patchset #6 (id:40002)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/913c63ce72b8783efda4971ff35ddbc01c8b69a6
Cr-Commit-Position: refs/heads/master@{#405948}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/913c63ce72b8783efda4971ff35ddbc01c8b69a6 Cr-Commit-Position: refs/heads/master@{#405948}
Message was sent while issue was closed.
hanifiqah9496@gmail.com changed reviewers: + hanifiqah9496@gmail.com
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Description was changed from
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/913c63ce72b8783efda4971ff35ddbc01c8b69a6
Cr-Commit-Position: refs/heads/master@{#405948}
==========
to
==========
Clear child FrameNavigationEntries if a history navigation redirects.
After the redirect, we shouldn't attempt to load the previous child
items in the new page.
BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
in --isolate-sites-for-testing=*.is mode.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/913c63ce72b8783efda4971ff35ddbc01c8b69a6
Cr-Commit-Position: refs/heads/master@{#405948}
==========
Message was sent while issue was closed.
creis@chromium.org changed reviewers: - hanifiqah9496@gmail.com |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
