Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317703004:
Simplify AreURLsInPageNavigation (Closed)
|
Created:
6 years, 6 months ago by Nate Chapin Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSimplify AreURLsInPageNavigation
The first case guarantees we trust the renderer's definition of an in-page navigation if the urls before and after navigation are on the same origin. An in-page navigation is impossible cross-origin, so this single check is sufficient.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279232
Patch Set 1 #
Total comments: 1
Patch Set 2 : Kill the renderer if it claims a cross-origin in-page navigation #
Total comments: 2
Patch Set 3 : Make layout tests happy #Patch Set 4 : + fix comment typos #
Total comments: 12
Patch Set 5 : Don't check origin if web security is disabled #Patch Set 6 : #
Messages
Total messages: 38 (0 generated)
LGTM with a nit https://codereview.chromium.org/317703004/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_controller_impl.cc:107: // See NavigationController::IsURLInPageNavigation for how this works and why. nit: I'll put the reasoning from the CL description here, so people reading the code understand why it is done this way.
lgtm
On 2014/06/06 18:12:09, Tom Sepez wrote: > lgtm Updated with a more detailed comment for AreURLsInPageNavigation, and refactored a bit to allow AreURLsInPageNavigation to kill the renderer if the renderer tries to claim that a cross-origin navigation is in-page.
> allow AreURLsInPageNavigation to kill the renderer if the renderer > tries to claim that a cross-origin navigation is in-page. That's a good idea. LGTM.
Even more LGTM! Thanks for thinking about security! https://codereview.chromium.org/317703004/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:114: // Howeer, due to reloads, even identical urls are *not* guaranteed to be nit: "However" https://codereview.chromium.org/317703004/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:116: // The one thing we do know that cross-origin navigations will *never* be nit: "is that"
sky: Would you mind reviewing the content/browser/web_contents/ changes?
LGTM
nasko, do these cases seem reasonable to you? https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || Declaring a navigation from an empty url same-origin handles the case of history.pushState in a frame's initial empty document. blink allows this, though it's not clear to me whether that's correct. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), Unlike other uses of the in-page logic, this callsite does not guarantee that it was the main frame that was navigated. Use the RenderFrameHost's state to determine the existing url to ensure we correctly handle subframe navigations.
Couple of comments. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || On 2014/06/06 22:45:23, Nate Chapin wrote: > Declaring a navigation from an empty url same-origin handles the case of > history.pushState in a frame's initial empty document. blink allows this, though > it's not clear to me whether that's correct. Isn't the initial empty document with "about:blank" URL (vs empty string)? https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), On 2014/06/06 22:45:23, Nate Chapin wrote: > Unlike other uses of the in-page logic, this callsite does not guarantee that it > was the main frame that was navigated. Use the RenderFrameHost's state to > determine the existing url to ensure we correctly handle subframe navigations. GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree node, which we save for estimating process allocation and overhead of site isolation. It looks we always set it on commit, but it was never used for such decisions before.
https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || On 2014/06/09 18:49:43, nasko wrote: > On 2014/06/06 22:45:23, Nate Chapin wrote: > > Declaring a navigation from an empty url same-origin handles the case of > > history.pushState in a frame's initial empty document. blink allows this, > though > > it's not clear to me whether that's correct. > > Isn't the initial empty document with "about:blank" URL (vs empty string)? It is (most of the time, there's a special case in blink's Document constructor), but I don't think blink reports the initial empty document's commit to the the browser process. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), On 2014/06/09 18:49:43, nasko wrote: > On 2014/06/06 22:45:23, Nate Chapin wrote: > > Unlike other uses of the in-page logic, this callsite does not guarantee that > it > > was the main frame that was navigated. Use the RenderFrameHost's state to > > determine the existing url to ensure we correctly handle subframe navigations. > > GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree node, > which we save for estimating process allocation and overhead of site isolation. > It looks we always set it on commit, but it was never used for such decisions > before. Fair enough. Is there an alternate way to get the committing frame's previous url without adding it to the DidCommitProvisionaLoad IPC?
https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || On 2014/06/09 18:53:49, Nate Chapin wrote: > On 2014/06/09 18:49:43, nasko wrote: > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > Declaring a navigation from an empty url same-origin handles the case of > > > history.pushState in a frame's initial empty document. blink allows this, > > though > > > it's not clear to me whether that's correct. > > > > Isn't the initial empty document with "about:blank" URL (vs empty string)? > > It is (most of the time, there's a special case in blink's Document > constructor), but I don't think blink reports the initial empty document's > commit to the the browser process. Ok, though I think we would have to start reporting it when session history moves to the browser. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), On 2014/06/09 18:53:49, Nate Chapin wrote: > On 2014/06/09 18:49:43, nasko wrote: > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > Unlike other uses of the in-page logic, this callsite does not guarantee > that > > it > > > was the main frame that was navigated. Use the RenderFrameHost's state to > > > determine the existing url to ensure we correctly handle subframe > navigations. > > > > GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree > node, > > which we save for estimating process allocation and overhead of site > isolation. > > It looks we always set it on commit, but it was never used for such decisions > > before. > > Fair enough. Is there an alternate way to get the committing frame's previous > url without adding it to the DidCommitProvisionaLoad IPC? It is already there: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fra...
https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || On 2014/06/09 18:59:24, nasko wrote: > On 2014/06/09 18:53:49, Nate Chapin wrote: > > On 2014/06/09 18:49:43, nasko wrote: > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > Declaring a navigation from an empty url same-origin handles the case of > > > > history.pushState in a frame's initial empty document. blink allows this, > > > though > > > > it's not clear to me whether that's correct. > > > > > > Isn't the initial empty document with "about:blank" URL (vs empty string)? > > > > It is (most of the time, there's a special case in blink's Document > > constructor), but I don't think blink reports the initial empty document's > > commit to the the browser process. > > Ok, though I think we would have to start reporting it when session history > moves to the browser. Huh, interesting. I would have thought that the initial empty document wouldn't necessarily need to be declared to the browser process, though if it was scripted or navigated to about:blank, then it would. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), On 2014/06/09 18:59:24, nasko wrote: > On 2014/06/09 18:53:49, Nate Chapin wrote: > > On 2014/06/09 18:49:43, nasko wrote: > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > Unlike other uses of the in-page logic, this callsite does not guarantee > > that > > > it > > > > was the main frame that was navigated. Use the RenderFrameHost's state to > > > > determine the existing url to ensure we correctly handle subframe > > navigations. > > > > > > GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree > > node, > > > which we save for estimating process allocation and overhead of site > > isolation. > > > It looks we always set it on commit, but it was never used for such > decisions > > > before. > > > > Fair enough. Is there an alternate way to get the committing frame's previous > > url without adding it to the DidCommitProvisionaLoad IPC? > > It is already there: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fra... I see the URL being navigated to, the base url, and the start of the redirect chain, but not the url that we're leaving. Am I missing something?
Still LGTM https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:124: bool is_same_origin = existing_url.is_empty() || On 2014/06/09 19:02:54, Nate Chapin wrote: > On 2014/06/09 18:59:24, nasko wrote: > > On 2014/06/09 18:53:49, Nate Chapin wrote: > > > On 2014/06/09 18:49:43, nasko wrote: > > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > > Declaring a navigation from an empty url same-origin handles the case of > > > > > history.pushState in a frame's initial empty document. blink allows > this, > > > > though > > > > > it's not clear to me whether that's correct. > > > > > > > > Isn't the initial empty document with "about:blank" URL (vs empty string)? > > > > > > It is (most of the time, there's a special case in blink's Document > > > constructor), but I don't think blink reports the initial empty document's > > > commit to the the browser process. > > > > Ok, though I think we would have to start reporting it when session history > > moves to the browser. > > Huh, interesting. I would have thought that the initial empty document wouldn't > necessarily need to be declared to the browser process, though if it was > scripted or navigated to about:blank, then it would. When I discussed this with Charlie, we found that having all cases reported to the browser process might make the logic more streamlined. In any case, I mentioned it as a FYI, no action item here for this CL. https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:773: details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), On 2014/06/09 19:02:54, Nate Chapin wrote: > On 2014/06/09 18:59:24, nasko wrote: > > On 2014/06/09 18:53:49, Nate Chapin wrote: > > > On 2014/06/09 18:49:43, nasko wrote: > > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > > Unlike other uses of the in-page logic, this callsite does not guarantee > > > that > > > > it > > > > > was the main frame that was navigated. Use the RenderFrameHost's state > to > > > > > determine the existing url to ensure we correctly handle subframe > > > navigations. > > > > > > > > GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree > > > node, > > > > which we save for estimating process allocation and overhead of site > > > isolation. > > > > It looks we always set it on commit, but it was never used for such > > decisions > > > > before. > > > > > > Fair enough. Is there an alternate way to get the committing frame's > previous > > > url without adding it to the DidCommitProvisionaLoad IPC? > > > > It is already there: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fra... > > I see the URL being navigated to, the base url, and the start of the redirect > chain, but not the url that we're leaving. Am I missing something? Duh, I misread what you meant. You are correct. Looking at it, we do set it always on commit, so I think it is fine to use and we can revisit this if needed in the future.
The CQ bit was checked by japhet@chromium.org
On 2014/06/09 20:41:29, nasko wrote: > Still LGTM > > https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... > File content/browser/frame_host/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... > content/browser/frame_host/navigation_controller_impl.cc:124: bool > is_same_origin = existing_url.is_empty() || > On 2014/06/09 19:02:54, Nate Chapin wrote: > > On 2014/06/09 18:59:24, nasko wrote: > > > On 2014/06/09 18:53:49, Nate Chapin wrote: > > > > On 2014/06/09 18:49:43, nasko wrote: > > > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > > > Declaring a navigation from an empty url same-origin handles the case > of > > > > > > history.pushState in a frame's initial empty document. blink allows > > this, > > > > > though > > > > > > it's not clear to me whether that's correct. > > > > > > > > > > Isn't the initial empty document with "about:blank" URL (vs empty > string)? > > > > > > > > It is (most of the time, there's a special case in blink's Document > > > > constructor), but I don't think blink reports the initial empty document's > > > > commit to the the browser process. > > > > > > Ok, though I think we would have to start reporting it when session history > > > moves to the browser. > > > > Huh, interesting. I would have thought that the initial empty document > wouldn't > > necessarily need to be declared to the browser process, though if it was > > scripted or navigated to about:blank, then it would. > > When I discussed this with Charlie, we found that having all cases reported to > the browser process might make the logic more streamlined. In any case, I > mentioned it as a FYI, no action item here for this CL. > > https://codereview.chromium.org/317703004/diff/60001/content/browser/frame_ho... > content/browser/frame_host/navigation_controller_impl.cc:773: > details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(), > On 2014/06/09 19:02:54, Nate Chapin wrote: > > On 2014/06/09 18:59:24, nasko wrote: > > > On 2014/06/09 18:53:49, Nate Chapin wrote: > > > > On 2014/06/09 18:49:43, nasko wrote: > > > > > On 2014/06/06 22:45:23, Nate Chapin wrote: > > > > > > Unlike other uses of the in-page logic, this callsite does not > guarantee > > > > that > > > > > it > > > > > > was the main frame that was navigated. Use the RenderFrameHost's state > > to > > > > > > determine the existing url to ensure we correctly handle subframe > > > > navigations. > > > > > > > > > > GetLastCommittedURL on RFH worries me a bit. It comes from the frame > tree > > > > node, > > > > > which we save for estimating process allocation and overhead of site > > > > isolation. > > > > > It looks we always set it on commit, but it was never used for such > > > decisions > > > > > before. > > > > > > > > Fair enough. Is there an alternate way to get the committing frame's > > previous > > > > url without adding it to the DidCommitProvisionaLoad IPC? > > > > > > It is already there: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fra... > > > > I see the URL being navigated to, the base url, and the start of the redirect > > chain, but not the url that we're leaving. Am I missing something? > > Duh, I misread what you meant. You are correct. Looking at it, we do set it > always on commit, so I think it is fine to use and we can revisit this if needed > in the future. Sounds good, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/japhet@chromium.org/317703004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/japhet@chromium.org/317703004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/japhet@chromium.org/317703004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/japhet@chromium.org/317703004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/japhet@chromium.org/317703004/100001
Message was sent while issue was closed.
Change committed as 279232 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
