Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1002953004:
Ensure we properly set PageTransition for iframes. (Closed)
|
Created:
5 years, 9 months ago by Nate Chapin Modified:
5 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_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. |
DescriptionEnsure we properly set PageTransition for iframes.
We currently don't set subframe navigations as manual when it isn't the first
navigation of the iframe. Also, we don't propagate the state correctly in the
case of a cross-process transition.
BUG=464014
Committed: https://crrev.com/735aa03be45990a5e2feb3c38087e6f76eb920ac
Cr-Commit-Position: refs/heads/master@{#326404}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address avi's comments #
Total comments: 10
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Update DidStartProvisionalLoad() commment (no test yet) #Patch Set 5 : +test #Patch Set 6 : #
Total comments: 9
Patch Set 7 : Fix remote->remote case #Patch Set 8 : #
Total comments: 6
Patch Set 9 : Fix style #Messages
Total messages: 30 (2 generated)
https://codereview.chromium.org/1002953004/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:1063: ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) { Propagate the state back into blink. https://codereview.chromium.org/1002953004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2357: ui::PAGE_TRANSITION_MANUAL_SUBFRAME); Set the transition type correctly in the case we're breaking. https://codereview.chromium.org/1002953004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2995: WebDataSource* provisional_data_source = frame->provisionalDataSource(); I have no idea why we use the top frame here, but it seems wrong to me.
avi@chromium.org changed reviewers: + avi@chromium.org, creis@chromium.org
Some comments here, but I'm not sure about the renderer side. Have Charlie also take a look. https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:908: // for cross-process AUTO_SUBFRAME navigations. This comment seems out-of-date with this change. https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1254: return; This is the wrong fix; this would always ignore a mismatch. Remove this entire if clause and do a simple: DCHECK(ui::PageTransitionCoreTypeIs(params.transition, ui::PAGE_TRANSITION_MANUAL_SUBFRAME));
https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:908: // for cross-process AUTO_SUBFRAME navigations. On 2015/03/12 20:40:28, Avi wrote: > This comment seems out-of-date with this change. Removed. https://codereview.chromium.org/1002953004/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1254: return; On 2015/03/12 20:40:28, Avi wrote: > This is the wrong fix; this would always ignore a mismatch. > > Remove this entire if clause and do a simple: > > DCHECK(ui::PageTransitionCoreTypeIs(params.transition, > ui::PAGE_TRANSITION_MANUAL_SUBFRAME)); Done.
The browser side of things looks good, but I'm not 100% certain, and I don't know about the renderer side. Charlie?
[+site-isolation-reviews] Thanks for working on this! I'm not sure I understand the fix, so I have a few questions below. Does this CL also fix the auto-subframe case (which should be WebInitialCommitInChildFrame and not WebStandardCommit)? I only see things for the manual subframe case, but I'm not sure about that top_frame code. Also, sanity check: The changes to navigation_controller_impl.cc aren't necessary for fixing the bug, right? They're just cleanup now that the bug is fixed? Actually, maybe those help with testing it? I imagine Avi's new NavigationControllerBrowserTest.NavigationTypeClassification_* tests might fail if you did the navigation_controller_impl.cc changes and not the render_frame_impl.cc changes, right? https://codereview.chromium.org/1002953004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1002953004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:904: CHECK(!ui::PageTransitionIsMainFrame(params.transition)); DCHECK. The renderer could be exploited or buggy and send the wrong transition, and that shouldn't crash the browser process. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2987: provisional_data_source ? provisional_data_source : top_data_source; I'm pretty confused by this whole block. Shouldn't they all be using the current frame's data source, eliminating Nasko's hack? Or are things like the navigation_state only stored on the top-level frame? At any rate, it seems wrong to me to mix the data sources of top_frame and frame on this line. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1066: if (ui::PageTransitionCoreTypeIs(params.common_params.transition, This needs a comment since it's non-obvious. I'm guessing the following? Manual subframe navigation requests from the browser can only occur once the frame has already committed at least one load. Since this may be a newly created RenderFrame for a cross-process navigation, we should ensure the WebFrame knows this. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1070: CHECK_EQ(frame, frame_); nit: Wrong indent. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2359: ui::PAGE_TRANSITION_MANUAL_SUBFRAME); This is not obvious from reading the code. Avi and I have been trying to reverse engineer a lot of code like this to understand what it means, so it would help to have a comment here explaining why this is the right thing to do.
On 2015/03/12 22:54:18, Charlie Reis wrote: > [+site-isolation-reviews] > > Thanks for working on this! I'm not sure I understand the fix, so I have a few > questions below. > > Does this CL also fix the auto-subframe case (which should be > WebInitialCommitInChildFrame and not WebStandardCommit)? I only see things for > the manual subframe case, but I'm not sure about that top_frame code. The auto subframe case might be WebHistoryInertCommit instead of WebInitialCommitInChildFrame, I think there's a bug in FrameLoader::receivedFirstData that forces remote frames to the inert case too aggressively. > > Also, sanity check: The changes to navigation_controller_impl.cc aren't > necessary for fixing the bug, right? They're just cleanup now that the bug is > fixed? Correct. > > Actually, maybe those help with testing it? I imagine Avi's new > NavigationControllerBrowserTest.NavigationTypeClassification_* tests might fail > if you did the navigation_controller_impl.cc changes and not the > render_frame_impl.cc changes, right? > I posted this before writing a test because I was not very confident my technique was sane. If it's looking hopeful, I'll focus on writing a test (and actually landing the blink plumbing that is required for this to compile).
https://codereview.chromium.org/1002953004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1002953004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:904: CHECK(!ui::PageTransitionIsMainFrame(params.transition)); On 2015/03/12 22:54:17, Charlie Reis wrote: > DCHECK. The renderer could be exploited or buggy and send the wrong transition, > and that shouldn't crash the browser process. Oops, I wasn't careful about this while testing and forgot to double-check them all. Done. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2987: provisional_data_source ? provisional_data_source : top_data_source; On 2015/03/12 22:54:18, Charlie Reis wrote: > I'm pretty confused by this whole block. Shouldn't they all be using the > current frame's data source, eliminating Nasko's hack? Or are things like the > navigation_state only stored on the top-level frame? > > At any rate, it seems wrong to me to mix the data sources of top_frame and frame > on this line. Fair point. Converted to all this frame's data sources, except for some prerendering histogram collection further down that I'm less sure of. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1066: if (ui::PageTransitionCoreTypeIs(params.common_params.transition, On 2015/03/12 22:54:18, Charlie Reis wrote: > This needs a comment since it's non-obvious. I'm guessing the following? > > Manual subframe navigation requests from the browser can only occur once the > frame has already committed at least one load. Since this may be a newly > created RenderFrame for a cross-process navigation, we should ensure the > WebFrame knows this. Yep. Added a comment to this effect. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1070: CHECK_EQ(frame, frame_); On 2015/03/12 22:54:18, Charlie Reis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/1002953004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2359: ui::PAGE_TRANSITION_MANUAL_SUBFRAME); On 2015/03/12 22:54:18, Charlie Reis wrote: > This is not obvious from reading the code. Avi and I have been trying to > reverse engineer a lot of code like this to understand what it means, so it > would help to have a comment here explaining why this is the right thing to do. Commented.
Thanks for adding the comments. I had a few questions from before that you might have missed, so I'm still not sure I understand it. Can you answer them now? > Does this CL also fix the auto-subframe case (which should be > WebInitialCommitInChildFrame and not WebStandardCommit)? I only see things for > the manual subframe case, but I'm not sure about that top_frame code. > > Also, sanity check: The changes to navigation_controller_impl.cc aren't > necessary for fixing the bug, right? They're just cleanup now that the bug is > fixed? > > Actually, maybe those help with testing it? I imagine Avi's new > NavigationControllerBrowserTest.NavigationTypeClassification_* tests might fail > if you did the navigation_controller_impl.cc changes and not the > render_frame_impl.cc changes, right? (In the meantime, I'll try to patch this in to verify.) https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2362: ui::PAGE_TRANSITION_LINK)) { Why is PAGE_TRANSITION_LINK the right way to detect this? Is that somehow equivalent to "creating a new history item?" (I thought it came in as PAGE_TRANSITION_MANUAL_SUBFRAME. When did it change?) https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:3151: top_document_state->set_was_prefetcher(true); Ah, sad that we still need this hack. No worries for now; we'll fix it later.
Sorry, I think I responded to your questions over two posts. Copying the non-obviously-placed replies: > > Thanks for working on this! I'm not sure I understand the fix, so I have a few > questions below. > > Does this CL also fix the auto-subframe case (which should be > WebInitialCommitInChildFrame and not WebStandardCommit)? I only see things for > the manual subframe case, but I'm not sure about that top_frame code. The auto subframe case might be WebHistoryInertCommit instead of WebInitialCommitInChildFrame, I think there's a bug in FrameLoader::receivedFirstData that forces remote frames to the inert case too aggressively. > > Also, sanity check: The changes to navigation_controller_impl.cc aren't > necessary for fixing the bug, right? They're just cleanup now that the bug is > fixed? Correct. https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2362: ui::PAGE_TRANSITION_LINK)) { On 2015/03/13 23:22:14, Charlie Reis wrote: > Why is PAGE_TRANSITION_LINK the right way to detect this? Is that somehow > equivalent to "creating a new history item?" > > (I thought it came in as PAGE_TRANSITION_MANUAL_SUBFRAME. When did it change?) This is the initial classification of a navigation, before the browser process sees the PageTransition. It starts as PAGE_TRANSITION_LINK, and the only time a subframe will have any other state at this point is if it was set to PAGE_TRANSITION_FORM_SUBMIT in RenderFrameImpl::willSubmitForm(). The PageTransition selected by this function will be attached to the main resource request, which will then get intercepted by CrossSiteResourceHandler and transfered to a different process. Basically, this happens in the original process of a navigation, while the changes in OnNavigate() happen after it moves to the new process.
Ah, my fault for missing your first message! Thanks. Also, I guess I won't be trying it locally-- I didn't realize it depended on Blink changes. The approach is sounding good to me now that I understand it a bit more. The WebInitialCommitInChildFrame for the first load will probably still need addressing, but it's ok to do that in a second CL if that's easier. Want to proceed with a test? https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:2362: ui::PAGE_TRANSITION_LINK)) { On 2015/03/13 23:28:38, Nate Chapin wrote: > On 2015/03/13 23:22:14, Charlie Reis wrote: > > Why is PAGE_TRANSITION_LINK the right way to detect this? Is that somehow > > equivalent to "creating a new history item?" > > > > (I thought it came in as PAGE_TRANSITION_MANUAL_SUBFRAME. When did it > change?) > > This is the initial classification of a navigation, before the browser process > sees the PageTransition. It starts as PAGE_TRANSITION_LINK, and the only time a > subframe will have any other state at this point is if it was set to > PAGE_TRANSITION_FORM_SUBMIT in RenderFrameImpl::willSubmitForm(). The > PageTransition selected by this function will be attached to the main resource > request, which will then get intercepted by CrossSiteResourceHandler and > transfered to a different process. > > Basically, this happens in the original process of a navigation, while the > changes in OnNavigate() happen after it moves to the new process. Ah! It makes so much more sense now. :) Can you put that in the comment?
On 2015/03/14 00:03:50, Charlie Reis wrote: > Ah, my fault for missing your first message! Thanks. > > Also, I guess I won't be trying it locally-- I didn't realize it depended on > Blink changes. > > The approach is sounding good to me now that I understand it a bit more. The > WebInitialCommitInChildFrame for the first load will probably still need > addressing, but it's ok to do that in a second CL if that's easier. Want to > proceed with a test? > > https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... > content/renderer/render_frame_impl.cc:2362: ui::PAGE_TRANSITION_LINK)) { > On 2015/03/13 23:28:38, Nate Chapin wrote: > > On 2015/03/13 23:22:14, Charlie Reis wrote: > > > Why is PAGE_TRANSITION_LINK the right way to detect this? Is that somehow > > > equivalent to "creating a new history item?" > > > > > > (I thought it came in as PAGE_TRANSITION_MANUAL_SUBFRAME. When did it > > change?) > > > > This is the initial classification of a navigation, before the browser process > > sees the PageTransition. It starts as PAGE_TRANSITION_LINK, and the only time > a > > subframe will have any other state at this point is if it was set to > > PAGE_TRANSITION_FORM_SUBMIT in RenderFrameImpl::willSubmitForm(). The > > PageTransition selected by this function will be attached to the main resource > > request, which will then get intercepted by CrossSiteResourceHandler and > > transfered to a different process. > > > > Basically, this happens in the original process of a navigation, while the > > changes in OnNavigate() happen after it moves to the new process. > > Ah! It makes so much more sense now. :) Can you put that in the comment? Sorry, which comment should this go in? One of the comments in render_frame_impl.cc, or in the commit descriptin?
On 2015/03/16 17:16:47, Nate Chapin wrote: > https://codereview.chromium.org/1002953004/diff/40001/content/renderer/render... > > content/renderer/render_frame_impl.cc:2362: ui::PAGE_TRANSITION_LINK)) { > > On 2015/03/13 23:28:38, Nate Chapin wrote: > > > On 2015/03/13 23:22:14, Charlie Reis wrote: > > > > Why is PAGE_TRANSITION_LINK the right way to detect this? Is that somehow > > > > equivalent to "creating a new history item?" > > > > > > > > (I thought it came in as PAGE_TRANSITION_MANUAL_SUBFRAME. When did it > > > change?) > > > > > > This is the initial classification of a navigation, before the browser > process > > > sees the PageTransition. It starts as PAGE_TRANSITION_LINK, and the only > time > > a > > > subframe will have any other state at this point is if it was set to > > > PAGE_TRANSITION_FORM_SUBMIT in RenderFrameImpl::willSubmitForm(). The > > > PageTransition selected by this function will be attached to the main > resource > > > request, which will then get intercepted by CrossSiteResourceHandler and > > > transfered to a different process. > > > > > > Basically, this happens in the original process of a navigation, while the > > > changes in OnNavigate() happen after it moves to the new process. > > > > Ah! It makes so much more sense now. :) Can you put that in the comment? > > Sorry, which comment should this go in? One of the comments in > render_frame_impl.cc, or in the commit descriptin? Right there in render_frame_impl.cc, in the comment at line 2363. Thanks!
Thanks for the new patch! I think I'm ok with the fact that the test passes without the fix, since we'll be removing that IsCrossProcessSubframe hack from ClassifyNavigation as a result of this. (Avi may have some ideas about the test class changes based on his NavigationControllerBrowserTest work, but no rush on that.) I patched this in and there appear to still be two bugs: 1) If a subframe is initially a cross-site URL, we still get WebHistoryInertCommit instead of WebInitialCommitInChildFrame. 2) In the case you're testing (same site -> cross site), any subsequent navigations in the frame are being reported as WebHistoryInertCommit. Do you see these as well? I could see fixing (1) either here or in a followup CL, but (2) seems concerning for this fix. https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1964: IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, I'm torn between adding this test here (where it will run on all the bots in --site-per-process mode) and NavigationControllerBrowserTest, where it would fit in a little better but would only run in --site-per-process mode on the Site Isolation FYI bots. (It's pretty close to NavigationTypeClassification_NewAndAutoSubframe, for example.) I think I like having it run on more bots, so let's leave it here for now. https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1989: EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, Please add a similar test for an initially cross-site page in an iframe, where we will expect NAVIGATION_TYPE_AUTO_SUBFRAME. See the end of NavigationControllerBrowserTest.NavigationTypeClassification_NewAndAutoSubframe for an example of how to watch for this when creating a new frame. https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:1109: CHECK_EQ(frame, frame_); Is the assumption here that frame_to_navigate is only set when talking to the main frame? Maybe it would be better to put CHECK(!frame_->parent()) on line 1094 instead. https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2577: // FORM_SUBMIT. This state will be attached to a main resouce request nit: resource
On 2015/04/15 22:46:11, Charlie Reis wrote: > Thanks for the new patch! I think I'm ok with the fact that the test passes > without the fix, since we'll be removing that IsCrossProcessSubframe hack from > ClassifyNavigation as a result of this. > > (Avi may have some ideas about the test class changes based on his > NavigationControllerBrowserTest work, but no rush on that.) > > I patched this in and there appear to still be two bugs: > > 1) If a subframe is initially a cross-site URL, we still get > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > 2) In the case you're testing (same site -> cross site), any subsequent > navigations in the frame are being reported as WebHistoryInertCommit. > > Do you see these as well? I could see fixing (1) either here or in a followup > CL, but (2) seems concerning for this fix. How are you testing (2)? I can reproduce the behavior you're describing on csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a "Go cross-site" button, but that's expected (and the same behavior as repeatedly pressing the "Go same-site" button). If you alternate between the two, the behavior is correct, and if you use devtools to perform a same-site navigation in an out-of-process iframe, we also get the correct behavior.
On 2015/04/16 21:18:17, Nate Chapin wrote: > On 2015/04/15 22:46:11, Charlie Reis wrote: > > Thanks for the new patch! I think I'm ok with the fact that the test passes > > without the fix, since we'll be removing that IsCrossProcessSubframe hack from > > ClassifyNavigation as a result of this. > > > > (Avi may have some ideas about the test class changes based on his > > NavigationControllerBrowserTest work, but no rush on that.) > > > > I patched this in and there appear to still be two bugs: > > > > 1) If a subframe is initially a cross-site URL, we still get > > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > > > 2) In the case you're testing (same site -> cross site), any subsequent > > navigations in the frame are being reported as WebHistoryInertCommit. > > > > Do you see these as well? I could see fixing (1) either here or in a followup > > CL, but (2) seems concerning for this fix. > > How are you testing (2)? I can reproduce the behavior you're describing on > csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a "Go > cross-site" button, but that's expected (and the same behavior as repeatedly > pressing the "Go same-site" button). If you alternate between the two, the > behavior is correct, and if you use devtools to perform a same-site navigation > in an out-of-process iframe, we also get the correct behavior. Visit that page, then click "Go cross-site (simple page)" to see WebStandardCommit as expected, then click "Go cross-site codereview" to see WebHistoryInertCommit (unexpected). You're right that I was generalizing it too much in the description, though. I was also testing repeated clicks of one of the buttons, but that's expected to be WebHistoryInertCommit. Good catch.
On 2015/04/16 21:33:01, Charlie Reis wrote: > On 2015/04/16 21:18:17, Nate Chapin wrote: > > On 2015/04/15 22:46:11, Charlie Reis wrote: > > > Thanks for the new patch! I think I'm ok with the fact that the test passes > > > without the fix, since we'll be removing that IsCrossProcessSubframe hack > from > > > ClassifyNavigation as a result of this. > > > > > > (Avi may have some ideas about the test class changes based on his > > > NavigationControllerBrowserTest work, but no rush on that.) > > > > > > I patched this in and there appear to still be two bugs: > > > > > > 1) If a subframe is initially a cross-site URL, we still get > > > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > > > > > 2) In the case you're testing (same site -> cross site), any subsequent > > > navigations in the frame are being reported as WebHistoryInertCommit. > > > > > > Do you see these as well? I could see fixing (1) either here or in a > followup > > > CL, but (2) seems concerning for this fix. > > > > How are you testing (2)? I can reproduce the behavior you're describing on > > csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a "Go > > cross-site" button, but that's expected (and the same behavior as repeatedly > > pressing the "Go same-site" button). If you alternate between the two, the > > behavior is correct, and if you use devtools to perform a same-site navigation > > in an out-of-process iframe, we also get the correct behavior. > > Visit that page, then click "Go cross-site (simple page)" to see > WebStandardCommit as expected, then click "Go cross-site codereview" to see > WebHistoryInertCommit (unexpected). > > You're right that I was generalizing it too much in the description, though. I > was also testing repeated clicks of one of the buttons, but that's expected to > be WebHistoryInertCommit. Good catch. Where are you checking the history commit type? Blink is definitely sending a WebStandardCommit for those steps for me.
On 2015/04/16 21:35:31, Nate Chapin wrote: > On 2015/04/16 21:33:01, Charlie Reis wrote: > > On 2015/04/16 21:18:17, Nate Chapin wrote: > > > On 2015/04/15 22:46:11, Charlie Reis wrote: > > > > Thanks for the new patch! I think I'm ok with the fact that the test > passes > > > > without the fix, since we'll be removing that IsCrossProcessSubframe hack > > from > > > > ClassifyNavigation as a result of this. > > > > > > > > (Avi may have some ideas about the test class changes based on his > > > > NavigationControllerBrowserTest work, but no rush on that.) > > > > > > > > I patched this in and there appear to still be two bugs: > > > > > > > > 1) If a subframe is initially a cross-site URL, we still get > > > > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > > > > > > > 2) In the case you're testing (same site -> cross site), any subsequent > > > > navigations in the frame are being reported as WebHistoryInertCommit. > > > > > > > > Do you see these as well? I could see fixing (1) either here or in a > > followup > > > > CL, but (2) seems concerning for this fix. > > > > > > How are you testing (2)? I can reproduce the behavior you're describing on > > > csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a "Go > > > cross-site" button, but that's expected (and the same behavior as repeatedly > > > pressing the "Go same-site" button). If you alternate between the two, the > > > behavior is correct, and if you use devtools to perform a same-site > navigation > > > in an out-of-process iframe, we also get the correct behavior. > > > > Visit that page, then click "Go cross-site (simple page)" to see > > WebStandardCommit as expected, then click "Go cross-site codereview" to see > > WebHistoryInertCommit (unexpected). > > > > You're right that I was generalizing it too much in the description, though. > I > > was also testing repeated clicks of one of the buttons, but that's expected to > > be WebHistoryInertCommit. Good catch. > > Where are you checking the history commit type? Blink is definitely sending a > WebStandardCommit for those steps for me. Nvm, I goofed on my cmdline args.
On 2015/04/16 21:36:02, Nate Chapin wrote: > On 2015/04/16 21:35:31, Nate Chapin wrote: > > On 2015/04/16 21:33:01, Charlie Reis wrote: > > > On 2015/04/16 21:18:17, Nate Chapin wrote: > > > > On 2015/04/15 22:46:11, Charlie Reis wrote: > > > > > Thanks for the new patch! I think I'm ok with the fact that the test > > passes > > > > > without the fix, since we'll be removing that IsCrossProcessSubframe > hack > > > from > > > > > ClassifyNavigation as a result of this. > > > > > > > > > > (Avi may have some ideas about the test class changes based on his > > > > > NavigationControllerBrowserTest work, but no rush on that.) > > > > > > > > > > I patched this in and there appear to still be two bugs: > > > > > > > > > > 1) If a subframe is initially a cross-site URL, we still get > > > > > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > > > > > > > > > 2) In the case you're testing (same site -> cross site), any subsequent > > > > > navigations in the frame are being reported as WebHistoryInertCommit. > > > > > > > > > > Do you see these as well? I could see fixing (1) either here or in a > > > followup > > > > > CL, but (2) seems concerning for this fix. > > > > > > > > How are you testing (2)? I can reproduce the behavior you're describing on > > > > csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a "Go > > > > cross-site" button, but that's expected (and the same behavior as > repeatedly > > > > pressing the "Go same-site" button). If you alternate between the two, the > > > > behavior is correct, and if you use devtools to perform a same-site > > navigation > > > > in an out-of-process iframe, we also get the correct behavior. > > > > > > Visit that page, then click "Go cross-site (simple page)" to see > > > WebStandardCommit as expected, then click "Go cross-site codereview" to see > > > WebHistoryInertCommit (unexpected). > > > > > > You're right that I was generalizing it too much in the description, though. > > > I > > > was also testing repeated clicks of one of the buttons, but that's expected > to > > > be WebHistoryInertCommit. Good catch. > > > > Where are you checking the history commit type? Blink is definitely sending a > > WebStandardCommit for those steps for me. > > Nvm, I goofed on my cmdline args. Ok, I think the incorrect WebHistoryCommitType is due to a different bug. When navigating between different domains that are both cross-origin to the parent, we end up sending 2 FrameMsg_NewFrame IPCs, one in the old process, one in the new process. The old process's "new iframe" (which shouldn't be new) is incorrectly showing the initial empty document, and therefore transferring the navigation to a new process propagates the incorrect state. It's not clear to me why the spurious new frame is being created.
On 2015/04/16 22:51:37, Nate Chapin wrote: > On 2015/04/16 21:36:02, Nate Chapin wrote: > > On 2015/04/16 21:35:31, Nate Chapin wrote: > > > On 2015/04/16 21:33:01, Charlie Reis wrote: > > > > On 2015/04/16 21:18:17, Nate Chapin wrote: > > > > > On 2015/04/15 22:46:11, Charlie Reis wrote: > > > > > > Thanks for the new patch! I think I'm ok with the fact that the test > > > passes > > > > > > without the fix, since we'll be removing that IsCrossProcessSubframe > > hack > > > > from > > > > > > ClassifyNavigation as a result of this. > > > > > > > > > > > > (Avi may have some ideas about the test class changes based on his > > > > > > NavigationControllerBrowserTest work, but no rush on that.) > > > > > > > > > > > > I patched this in and there appear to still be two bugs: > > > > > > > > > > > > 1) If a subframe is initially a cross-site URL, we still get > > > > > > WebHistoryInertCommit instead of WebInitialCommitInChildFrame. > > > > > > > > > > > > 2) In the case you're testing (same site -> cross site), any > subsequent > > > > > > navigations in the frame are being reported as WebHistoryInertCommit. > > > > > > > > > > > > Do you see these as well? I could see fixing (1) either here or in a > > > > followup > > > > > > CL, but (2) seems concerning for this fix. > > > > > > > > > > How are you testing (2)? I can reproduce the behavior you're describing > on > > > > > csreis.github.io/tests/cross-site-iframe.html by repeatedly pressing a > "Go > > > > > cross-site" button, but that's expected (and the same behavior as > > repeatedly > > > > > pressing the "Go same-site" button). If you alternate between the two, > the > > > > > behavior is correct, and if you use devtools to perform a same-site > > > navigation > > > > > in an out-of-process iframe, we also get the correct behavior. > > > > > > > > Visit that page, then click "Go cross-site (simple page)" to see > > > > WebStandardCommit as expected, then click "Go cross-site codereview" to > see > > > > WebHistoryInertCommit (unexpected). > > > > > > > > You're right that I was generalizing it too much in the description, > though. > > > > > I > > > > was also testing repeated clicks of one of the buttons, but that's > expected > > to > > > > be WebHistoryInertCommit. Good catch. > > > > > > Where are you checking the history commit type? Blink is definitely sending > a > > > WebStandardCommit for those steps for me. > > > > Nvm, I goofed on my cmdline args. > > Ok, I think the incorrect WebHistoryCommitType is due to a different bug. When > navigating between different domains that are both cross-origin to the parent, > we end up sending 2 FrameMsg_NewFrame IPCs, one in the old process, one in the > new process. The old process's "new iframe" (which shouldn't be new) is > incorrectly showing the initial empty document, and therefore transferring the > navigation to a new process propagates the incorrect state. > > It's not clear to me why the spurious new frame is being created. Nice find, though I think that might be a red herring. I spent some time debugging that and it's because we're redirecting from HTTP to HTTPS on the code review page. (The button click goes through RenderFrameProxyHost::OpenURL to load http://codereview.chromium.org. We then redirect to the HTTPS version, requiring a process swap and another new frame in a new process.) We need to support this redirect case, but I also found the same InertCommit problem when there is no redirect.
https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1989: EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, On 2015/04/15 22:46:11, Charlie Reis wrote: > Please add a similar test for an initially cross-site page in an iframe, where > we will expect NAVIGATION_TYPE_AUTO_SUBFRAME. > > See the end of > NavigationControllerBrowserTest.NavigationTypeClassification_NewAndAutoSubframe > for an example of how to watch for this when creating a new frame. That case reliably returns NAVIGATION_TYPE_NEW_SUBFRAME, because of the same class of bugging that causes this test to pass with and without this change. Do you want me to add a case that asserts the wrong behavior? https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:1109: CHECK_EQ(frame, frame_); On 2015/04/15 22:46:11, Charlie Reis wrote: > Is the assumption here that frame_to_navigate is only set when talking to the > main frame? Maybe it would be better to put CHECK(!frame_->parent()) on line > 1094 instead. Uhh, now that I trace through this, it looks like frame_to_navigate only gets set non-empty by layout tests (though non-chrome embedders might do otherwise). I think the invariant I was trying to create is: "if this frame is in a different process than its parent, we ought to have sufficient information elsewhere to have figured out the right frame before we send a FrameMsg_Navigate IPC". https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2577: // FORM_SUBMIT. This state will be attached to a main resouce request On 2015/04/15 22:46:11, Charlie Reis wrote: > nit: resource Done.
Sorry for the long delay! I patched in this version and (2) is fixed. We now classify all the manual cases correctly, as far as I can tell. (1) still looks broken: If a subframe is initially a cross-site URL, we still get WebHistoryInertCommit instead of WebInitialCommitInChildFrame. That's relevant for the test you're considering adding. Do you plan to fix that here or in another CL? Apart from that, one question about transition types for RequestOpenURL, and otherwise looks good. https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:1989: EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, On 2015/04/17 21:48:18, Nate Chapin wrote: > On 2015/04/15 22:46:11, Charlie Reis wrote: > > Please add a similar test for an initially cross-site page in an iframe, where > > we will expect NAVIGATION_TYPE_AUTO_SUBFRAME. > > > > See the end of > > > NavigationControllerBrowserTest.NavigationTypeClassification_NewAndAutoSubframe > > for an example of how to watch for this when creating a new frame. > > That case reliably returns NAVIGATION_TYPE_NEW_SUBFRAME, because of the same > class of bugging that causes this test to pass with and without this change. Do > you want me to add a case that asserts the wrong behavior? Sure, with a TODO to make it AUTO. I should be able to fix it once your CL and Avi's CL land. https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:1109: CHECK_EQ(frame, frame_); On 2015/04/17 21:48:18, Nate Chapin wrote: > On 2015/04/15 22:46:11, Charlie Reis wrote: > > Is the assumption here that frame_to_navigate is only set when talking to the > > main frame? Maybe it would be better to put CHECK(!frame_->parent()) on line > > 1094 instead. > > Uhh, now that I trace through this, it looks like frame_to_navigate only gets > set non-empty by layout tests (though non-chrome embedders might do otherwise). > > I think the invariant I was trying to create is: "if this frame is in a > different process than its parent, we ought to have sufficient information > elsewhere to have figured out the right frame before we send a FrameMsg_Navigate > IPC". Hmm, ok. We can leave it as is, and I'll try to clean up Nasko's TODO so we can remove |frame| and just have |frame_|. https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1621: transition = params.should_replace_current_entry Maybe we should be preserving the actual transition from the renderer process? I imagine we could put it into FrameHostMsg_OpenURL_Params in RenderFrameImpl::OpenURL, getting it from the NavigationState. That would preserve masks like PAGE_TRANSITION_FORWARD_BACK. https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2583: navigation_state->GetTransitionType(), Style nit: This indent looks odd. git cl format might help.
The incorrect commit type in (1) is because of a known issue in blink that is relatively easy to fix at this point. I'll post a patch for that today. https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1621: transition = params.should_replace_current_entry On 2015/04/22 20:44:57, Charlie Reis wrote: > Maybe we should be preserving the actual transition from the renderer process? > > I imagine we could put it into FrameHostMsg_OpenURL_Params in > RenderFrameImpl::OpenURL, getting it from the NavigationState. That would > preserve masks like PAGE_TRANSITION_FORWARD_BACK. That doesn't really work, because the NavigationState's transition type doesn't get set until after we make the policy decision that could lead to a FrameHostMsg_OpenURL. Also, RenderFrameProxy doesn't have very robust information from which to calculate the PageTransition. https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2583: navigation_state->GetTransitionType(), On 2015/04/22 20:44:57, Charlie Reis wrote: > Style nit: This indent looks odd. git cl format might help. Done.
Deferring (1) to another patch sounds good to me. LGTM once the new patch is up. https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1621: transition = params.should_replace_current_entry On 2015/04/22 21:59:23, Nate Chapin wrote: > On 2015/04/22 20:44:57, Charlie Reis wrote: > > Maybe we should be preserving the actual transition from the renderer process? > > > > I imagine we could put it into FrameHostMsg_OpenURL_Params in > > RenderFrameImpl::OpenURL, getting it from the NavigationState. That would > > preserve masks like PAGE_TRANSITION_FORWARD_BACK. > > That doesn't really work, because the NavigationState's transition type doesn't > get set until after we make the policy decision that could lead to a > FrameHostMsg_OpenURL. Also, RenderFrameProxy doesn't have very robust > information from which to calculate the PageTransition. Ok. I'll defer on this, since we should be able to get rid of OpenURL once PlzNavigate is done. https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:2583: navigation_state->GetTransitionType(), On 2015/04/22 21:59:23, Nate Chapin wrote: > On 2015/04/22 20:44:57, Charlie Reis wrote: > > Style nit: This indent looks odd. git cl format might help. > > Done. Thanks. (I don't see the new patch uploaded yet.)
On 2015/04/22 22:04:29, Charlie Reis wrote: > Deferring (1) to another patch sounds good to me. > > LGTM once the new patch is up. > > https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/1002953004/diff/140001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:1621: transition = > params.should_replace_current_entry > On 2015/04/22 21:59:23, Nate Chapin wrote: > > On 2015/04/22 20:44:57, Charlie Reis wrote: > > > Maybe we should be preserving the actual transition from the renderer > process? > > > > > > I imagine we could put it into FrameHostMsg_OpenURL_Params in > > > RenderFrameImpl::OpenURL, getting it from the NavigationState. That would > > > preserve masks like PAGE_TRANSITION_FORWARD_BACK. > > > > That doesn't really work, because the NavigationState's transition type > doesn't > > get set until after we make the policy decision that could lead to a > > FrameHostMsg_OpenURL. Also, RenderFrameProxy doesn't have very robust > > information from which to calculate the PageTransition. > > Ok. I'll defer on this, since we should be able to get rid of OpenURL once > PlzNavigate is done. > > https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/1002953004/diff/140001/content/renderer/rende... > content/renderer/render_frame_impl.cc:2583: > navigation_state->GetTransitionType(), > On 2015/04/22 21:59:23, Nate Chapin wrote: > > On 2015/04/22 20:44:57, Charlie Reis wrote: > > > Style nit: This indent looks odd. git cl format might help. > > > > Done. > > Thanks. (I don't see the new patch uploaded yet.) Sorry, it's up now, I got distracted.
Thanks, LGTM.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1002953004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/735aa03be45990a5e2feb3c38087e6f76eb920ac Cr-Commit-Position: refs/heads/master@{#326404} |
