Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871293002:
Don't use pending NavigationEntries for navigation transfers (try #2). (Closed)
|
Created:
4 years, 8 months ago by Charlie Reis Modified:
4 years, 7 months ago Reviewers:
alexmos CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_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. |
DescriptionDon't use pending NavigationEntries for navigation transfers (try #2).
Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL). Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry. That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).
This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one. A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.
This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.
This CL is a second attempt that fixes crashes seen after
https://crrev.com/384009, and adds tests for those cases.
It works around https://crbug.com/608402 for the time being.
BUG=495161, 584739, 608402
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c717927a3712cf6391962971e0e4084a8f159ba0
Cr-Commit-Position: refs/heads/master@{#391439}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add tests and crash fix #Patch Set 4 : Fix SubframeOnEmptyPage #
Total comments: 15
Patch Set 5 : Fix review comments. #
Messages
Total messages: 21 (12 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. BUG=495161, 584739 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. This CL is a second attempt that fixes crashes seen after https://crrev.com/384009, and adds tests for those cases. It works around https://crbug.com/608402 for the time being. BUG=495161, 584739, 608402 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
creis@chromium.org changed reviewers: + alexmos@chromium.org
Patchset #4 (id:120001) has been deleted
Alex, can you take a look? This is very similar to https://codereview.chromium.org/1683593002, but handles the case that we don't actually add the FNE to the tree. I thought about trying to fix 608402 before this CL, but that's a fair amount of effort and default Chrome doesn't do any better there. It's probably better to get this in so we can switch to the new path, then fix 608402. All the new tests show various ways we would crash with the first transfer shortcut CL. They'll also be useful for testing the fix for 608402. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (left): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:481: IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, SubframeOnEmptyPage) { Sorry, had to move this test below so it could use LoadCommittedCapturer. I know that makes reviewing it a bit more of a hassle. There's a bit of a style cleanup, but the main change is adding a nested subframe at the end. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:805: if (!frame_entry) { This is the fix. It should be safe for the time being (unless anyone starts depending on the frame entry being inside the NavEntry). I think we can get by with it until either 608402 or 536906 are fixed.
Thanks, this looks great, just one question about FNE cleanup and a few nits. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:640: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); While we're here - the wait doesn't seem necessary, and EXPECT_TRUE(NavigateToURL(...)) should be enough, right? https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1601: NavigateToURL(shell(), main_url); nit: EXPECT_TRUE (in all tests) https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1739: // FrameNavigationEntry. Would it be useful to also reference the crash bugs on some of these tests? (i.e., https://crbug.com/603245, https://crbug.com/600743) https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1776: capturer.Wait(); There are a lot of these create-frame-and-wait blocks - would a helper be useful? Or would that lose proper line numbers for failures that might happen here? https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:775: referrer_to_use, PageState(), "GET", -1); Is it ok that this is always "GET" (and also in the fix below)? Maybe this needs a TODO to figure out how the POST path is going to work? https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:806: frame_entry = How will this get cleaned up?
Thanks! PTAL. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:640: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); On 2016/05/03 22:07:51, alexmos wrote: > While we're here - the wait doesn't seem necessary, and > EXPECT_TRUE(NavigateToURL(...)) should be enough, right? Done. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1601: NavigateToURL(shell(), main_url); On 2016/05/03 22:07:51, alexmos wrote: > nit: EXPECT_TRUE (in all tests) Done. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1739: // FrameNavigationEntry. On 2016/05/03 22:07:51, alexmos wrote: > Would it be useful to also reference the crash bugs on some of these tests? > (i.e., https://crbug.com/603245, https://crbug.com/600743) Yep. 603245 was due to the back/forward CL and isn't covered here, but 600743 is worth mentioning. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1776: capturer.Wait(); On 2016/05/03 22:07:51, alexmos wrote: > There are a lot of these create-frame-and-wait blocks - would a helper be > useful? Or would that lose proper line numbers for failures that might happen > here? Yeah, it's a bit of a tradeoff. I tried switching to a helper function but confirmed that we lose the line number there, which would make you go into a debugger for several of these tests if they fail. Also, some of the blocks (e.g., in the IGNORE cases in SubframeOnEmptyPage) can't verify PAGE_TRANSITION_AUTO_SUBFRAME, so we'd have to either parameterize or skip the helper for those. In general, copy/paste isn't quite as big a problem in tests, and I think the relatively small block here is probably safe to repeat given the above. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:775: referrer_to_use, PageState(), "GET", -1); On 2016/05/03 22:07:52, alexmos wrote: > Is it ok that this is always "GET" (and also in the fix below)? Maybe this > needs a TODO to figure out how the POST path is going to work? Yeah, this is a TODO. https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:806: frame_entry = On 2016/05/03 22:07:51, alexmos wrote: > How will this get cleaned up? Good point-- it would leak in this branch, since |entry| doesn't claim ownership of it. I've changed it to be a scoped_refptr above, since that's how NavEntry tracks it internally (and it's how we avoid a leak in RendererDidNavigateNewSubframe as well).
LGTM https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1871293002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:1776: capturer.Wait(); On 2016/05/03 23:43:05, Charlie Reis wrote: > On 2016/05/03 22:07:51, alexmos wrote: > > There are a lot of these create-frame-and-wait blocks - would a helper be > > useful? Or would that lose proper line numbers for failures that might happen > > here? > > Yeah, it's a bit of a tradeoff. I tried switching to a helper function but > confirmed that we lose the line number there, which would make you go into a > debugger for several of these tests if they fail. Also, some of the blocks > (e.g., in the IGNORE cases in SubframeOnEmptyPage) can't verify > PAGE_TRANSITION_AUTO_SUBFRAME, so we'd have to either parameterize or skip the > helper for those. > > In general, copy/paste isn't quite as big a problem in tests, and I think the > relatively small block here is probably safe to repeat given the above. Acknowledged.
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871293002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871293002/160001
Message was sent while issue was closed.
Description was changed from ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. This CL is a second attempt that fixes crashes seen after https://crrev.com/384009, and adds tests for those cases. It works around https://crbug.com/608402 for the time being. BUG=495161, 584739, 608402 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. This CL is a second attempt that fixes crashes seen after https://crrev.com/384009, and adds tests for those cases. It works around https://crbug.com/608402 for the time being. BUG=495161, 584739, 608402 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. This CL is a second attempt that fixes crashes seen after https://crrev.com/384009, and adds tests for those cases. It works around https://crbug.com/608402 for the time being. BUG=495161, 584739, 608402 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't use pending NavigationEntries for navigation transfers (try #2). Cross-process transfers for navigations that are about to commit had been using NavigationControllerImpl::LoadURLWithParams (as an artifact of previously going through OpenURL). Most of that code is unnecessary, in particular the fact that it creates a new pending NavigationEntry. That's problematic for subframe transfers, where we should not affect any existing pending NavigationEntry (e.g., for a slow main frame navigation). This CL shortcuts the transfer by creating a NavigationEntry without making it the pending one. A future CL can eliminate the entry entirely by validating and passing the parameters to the right RenderFrameHost directly. This also makes it possible to do cross-process navigations in subframes of the initial blank page, fixing a crash. This CL is a second attempt that fixes crashes seen after https://crrev.com/384009, and adds tests for those cases. It works around https://crbug.com/608402 for the time being. BUG=495161, 584739, 608402 TEST=Create OOPIF on initial blank page of a tab. TEST=Cross-process subframe navigation during slow main frame navigation. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c717927a3712cf6391962971e0e4084a8f159ba0 Cr-Commit-Position: refs/heads/master@{#391439} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c717927a3712cf6391962971e0e4084a8f159ba0 Cr-Commit-Position: refs/heads/master@{#391439} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
