Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11416121:
Prevent cross-site pages when --site-per-process is passed (Closed)
|
Created:
8 years, 1 month ago by irobert Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, nasko Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrevent cross-site pages if the --site-per-process flag is passed
BUG=159215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172403
Patch Set 1 : Fix resource_type #Patch Set 2 : Fix Merger CanRequestURL API #Patch Set 3 : Fix #
Total comments: 23
Patch Set 4 : Fix CanLoadPage #Patch Set 5 : Fix Iframe Redirect Flaw #
Total comments: 16
Patch Set 6 : Fix Redirect Bug and Tests #
Total comments: 26
Patch Set 7 : Fix Comments #
Total comments: 29
Patch Set 8 : Fix Comments #
Total comments: 2
Patch Set 9 : Fix NotificationObserver #
Total comments: 1
Patch Set 10 : Fix Failure Test #Patch Set 11 : Test #Patch Set 12 : Fix Test #Patch Set 13 : Fix #
Messages
Total messages: 25 (0 generated)
https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:146: ResourceType::LAST_TYPE)); For these tests, resource type does not matter the result. https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... content/browser/renderer_host/render_view_host_impl.cc:1742: *url, ResourceType::MAIN_FRAME)) { It is hard to tell whether this navigation is main frame or iframe. But it doesn't affect our access control decision.
Thanks for trying out this approach. I'm now concerned about all the changes to CanRequestURL, now that we see how many places it affects (and how many of them don't know the resource type). Maybe something closer to your original patch, with a CanRequestPage function, is a better idea after all. Sorry for the extra work. (Also, please update the CL description to use site-per-process rather than enable-strict-site-isolation.) https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:72: GURL site_gurl = SiteInstanceImpl::GetSiteForURL(NULL, gurl); Please add the same TODO as in CanAccessCookiesForOrigin, since passing NULL here breaks hosted apps. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:505: // If --enable-strict-site-isolation flag is passed, --site-per-process https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:508: // TODO: This will break some WebUI page such as "chrome://extensions/" nit: TODO(irobert): Also, move the TODO comment inside the if, and end with a period. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:512: switches::kEnableStrictSiteIsolation) && kSitePerProcess https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:514: resource_type == ResourceType::SUB_FRAME)) { Looks like there's a ResourceType::IsFrame(resource_type) for doing this check. We should use that, since that will help if there's ever another type added to that list. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:519: if (!state->second->CanLoadIframe(url)) CanLoadIframe isn't an accurate name, if we're also using it for MAIN_FRAME requests. How about CanLoadPage? https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:136: ResourceType::MAIN_FRAME)); Just to test both code paths, let's make this one SUB_FRAME. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:146: ResourceType::LAST_TYPE)); On 2012/11/28 01:27:57, irobert wrote: > For these tests, resource type does not matter the result. Sure, but might as well use something realistic. IMAGE in this case. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:250: ResourceType::LAST_TYPE)); Let's make these all MAIN_FRAME, since the intent is to test them as a real page. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:291: ResourceType::LAST_TYPE)); Same: these should all be MAIN_FRAME. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:318: ResourceType::IMAGE)); Even though this is an image, let's treat these all as MAIN_FRAME, since we're not really testing the resource_type code paths here. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_unittest.cc:570: ResourceType::LAST_TYPE)); MAIN_FRAME https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... content/browser/renderer_host/render_view_host_impl.cc:1742: *url, ResourceType::MAIN_FRAME)) { On 2012/11/28 01:27:57, irobert wrote: > It is hard to tell whether this navigation is main frame or iframe. But it > doesn't affect our access control decision. Hmm, actually, this is problematic. FilterURL gets called for all sorts of things, like referrers, form URLs, link URLs, etc. Maybe we should rethink the decision to change CanRequestURL if it's used here... https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... content/browser/renderer_host/resource_dispatcher_host_impl.cc:533: CanRequestURL(child_id, url, ResourceType::LAST_TYPE)) { This is also making me second guess my suggestion to change CanRequestURL.
https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:72: GURL site_gurl = SiteInstanceImpl::GetSiteForURL(NULL, gurl); On 2012/11/28 18:58:26, creis wrote: > Please add the same TODO as in CanAccessCookiesForOrigin, since passing NULL > here breaks hosted apps. Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:505: // If --enable-strict-site-isolation flag is passed, On 2012/11/28 18:58:26, creis wrote: > --site-per-process Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:508: // TODO: This will break some WebUI page such as "chrome://extensions/" On 2012/11/28 18:58:26, creis wrote: > nit: TODO(irobert): > > Also, move the TODO comment inside the if, and end with a period. Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:512: switches::kEnableStrictSiteIsolation) && On 2012/11/28 18:58:26, creis wrote: > kSitePerProcess Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:514: resource_type == ResourceType::SUB_FRAME)) { On 2012/11/28 18:58:26, creis wrote: > Looks like there's a ResourceType::IsFrame(resource_type) for doing this check. > We should use that, since that will help if there's ever another type added to > that list. Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:519: if (!state->second->CanLoadIframe(url)) On 2012/11/28 18:58:26, creis wrote: > CanLoadIframe isn't an accurate name, if we're also using it for MAIN_FRAME > requests. How about CanLoadPage? Done. https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/15008/content/browser/renderer_... content/browser/renderer_host/render_view_host_impl.cc:1742: *url, ResourceType::MAIN_FRAME)) { On 2012/11/28 18:58:26, creis wrote: > On 2012/11/28 01:27:57, irobert wrote: > > It is hard to tell whether this navigation is main frame or iframe. But it > > doesn't affect our access control decision. > > Hmm, actually, this is problematic. FilterURL gets called for all sorts of > things, like referrers, form URLs, link URLs, etc. Maybe we should rethink the > decision to change CanRequestURL if it's used here... Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... content/browser/renderer_host/resource_dispatcher_host_impl.cc:537: // TODO(irobert): Should we call CanRequestPage for download request? I think you have already answered this question. We should allow X-site download.
Great. Some nits below, but the main remaining question is where to put the check so that it blocks pages but not server redirects or downloads. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:174: // apps URLs. Currently, hosted apps cannot set cookies in this mode. nit: s/set cookies/be loaded/ https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:501: int child_id, const GURL& url, ResourceType::Type resource_type) { Style nit: Each argument should be on its own line if the whole thing doesn't fit on one line. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:506: // TODO(irobert): This will break some WebUI page such as nit: s/will break/currently breaks/ https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:509: // (belongs to site chrome://uber-frame/) nit: End with period. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:114: // Returns true if the process is permitted to load the page for nit: s/the page for/pages from/ https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:115: // the given origin (applied to the page in main_frame and sub_frame). nit: the given origin in main frames or subframes. https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... File content/browser/renderer_host/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... content/browser/renderer_host/resource_dispatcher_host_impl.cc:177: } This looks good, but I wonder if it's the right place for it. I'm guessing it would prevent cross-site downloads? https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... content/browser/renderer_host/resource_dispatcher_host_impl.cc:537: // TODO(irobert): Should we call CanRequestPage for download request? On 2012/11/28 22:50:41, irobert wrote: > I think you have already answered this question. We should allow X-site > download. Correct. We only want to block cross-site pages from being delivered to the renderer process. Anything that happens in the mean time (server redirects, downloads, etc) should be allowed. https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... File content/browser/renderer_host/resource_loader.cc (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/renderer_h... content/browser/renderer_host/resource_loader.cc:227: // process, we should block it. This comment is correct. That makes me think we should not include this check here. We probably only need a single CanLoadPage check when the response starts, as long as there's a way to cancel it there. I've been alluding to CrossSiteResourceHandler::OnResponseStarted, which is when we know a page will be delivered to the renderer. CrossSiteResourceHandlers are only created for certain requests, though, so we'd probably want to put the check in someplace that calls that. I'd suggest setting a breakpoint there, doing a cross-process navigation, and then looking at the call stack to find an appropriate place. Feel free to ask me in person if you get that stack in a debugger.
Oh, you may also want to change the title and description of this CL from "Prevent third party iframes" to "Prevent third party pages", since we're applying the check to both subframes and top-level pages.
https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:174: // apps URLs. Currently, hosted apps cannot set cookies in this mode. On 2012/11/29 22:00:54, creis wrote: > nit: s/set cookies/be loaded/ Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:501: int child_id, const GURL& url, ResourceType::Type resource_type) { On 2012/11/29 22:00:54, creis wrote: > Style nit: Each argument should be on its own line if the whole thing doesn't > fit on one line. Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:506: // TODO(irobert): This will break some WebUI page such as On 2012/11/29 22:00:54, creis wrote: > nit: s/will break/currently breaks/ Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.cc:509: // (belongs to site chrome://uber-frame/) On 2012/11/29 22:00:54, creis wrote: > nit: End with period. Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:114: // Returns true if the process is permitted to load the page for On 2012/11/29 22:00:54, creis wrote: > nit: s/the page for/pages from/ Done. https://codereview.chromium.org/11416121/diff/3004/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:115: // the given origin (applied to the page in main_frame and sub_frame). On 2012/11/29 22:00:54, creis wrote: > nit: the given origin in main frames or subframes. Done.
Great. Glad you added some tests, and it looks like the check is in the right place now. Mainly some cleanup at this point. https://codereview.chromium.org/11416121/diff/9013/content/browser/child_proc... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:115: // the given origin in main frames or subframes.. nit: Just one period. https://codereview.chromium.org/11416121/diff/9013/content/browser/renderer_h... File content/browser/renderer_host/resource_loader.cc (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/renderer_h... content/browser/renderer_host/resource_loader.cc:318: // process, we should block it. This is a nice comment but it's kind of out of place here, since there's nothing specific to the site-per-process policy here. Let's simplify it to: "The CanLoadPage check should take place after any server redirects have finished, at the point in time that we know a page will commit in the renderer process." https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... File content/browser/site_per_process_test.cc (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This file should be named site_per_process_browsertest.cc. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:25: std::string script = "var iframes = document.getElementById(\"" + Please use base::StringPrintf. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:29: window->web_contents()->GetRenderViewHost(), nit: wrong indent. (Should be 4 spaces in from previous line.) https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:33: void EnableSitePerProces() { If you put this in SetUpCommandLine, it will apply to all the tests in this file without having to call it. Here's an example: https://code.google.com/searchframe#OAMlx_jo-ck/src/content/test/layout_brows... https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:34: /*switches::kSitePerProcess*/ Please use this constant in the line below. (No need for this comment.) https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:39: class SitePerProcessTestWebContentsObserver : public WebContentsObserver { nit: SitePerProcessWebContentsObserver would be a slightly shorter name. Let's use that instead. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:75: bool navigation_result_; navigation_succeeded_ https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:94: string16 actual_title = title_watcher.WaitAndGetTitle(); Do we need this TitleWatcher? I think the NavigateToURL call should be sufficient. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:100: WindowedNotificationObserver load_observer( Perhaps the load_observer should be inside NavigateIframeToURL? That's more consistent with NavigateToURL. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:103: &shell()->web_contents()->GetController())); nit: Wrong indent. (Will this fit on the previous line?) https://codereview.chromium.org/11416121/diff/9013/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/11416121/diff/9013/content/content_tests.gypi... content/content_tests.gypi:692: 'browser/site_per_process_test.cc', Please alphabetize.
https://codereview.chromium.org/11416121/diff/9013/content/browser/child_proc... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/child_proc... content/browser/child_process_security_policy_impl.h:115: // the given origin in main frames or subframes.. On 2012/12/05 02:02:58, creis wrote: > nit: Just one period. Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/renderer_h... File content/browser/renderer_host/resource_loader.cc (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/renderer_h... content/browser/renderer_host/resource_loader.cc:318: // process, we should block it. On 2012/12/05 02:02:58, creis wrote: > This is a nice comment but it's kind of out of place here, since there's nothing > specific to the site-per-process policy here. Let's simplify it to: > > "The CanLoadPage check should take place after any server redirects have > finished, at the point in time that we know a page will commit in the renderer > process." Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... File content/browser/site_per_process_test.cc (right): https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/12/05 02:02:58, creis wrote: > This file should be named site_per_process_browsertest.cc. Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:25: std::string script = "var iframes = document.getElementById(\"" + On 2012/12/05 02:02:58, creis wrote: > Please use base::StringPrintf. Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:29: window->web_contents()->GetRenderViewHost(), On 2012/12/05 02:02:58, creis wrote: > nit: wrong indent. (Should be 4 spaces in from previous line.) Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:33: void EnableSitePerProces() { On 2012/12/05 02:02:58, creis wrote: > If you put this in SetUpCommandLine, it will apply to all the tests in this file > without having to call it. Here's an example: > https://code.google.com/searchframe#OAMlx_jo-ck/src/content/test/layout_brows... Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:34: /*switches::kSitePerProcess*/ On 2012/12/05 02:02:58, creis wrote: > Please use this constant in the line below. (No need for this comment.) Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:39: class SitePerProcessTestWebContentsObserver : public WebContentsObserver { On 2012/12/05 02:02:58, creis wrote: > nit: SitePerProcessWebContentsObserver would be a slightly shorter name. Let's > use that instead. Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:75: bool navigation_result_; On 2012/12/05 02:02:58, creis wrote: > navigation_succeeded_ Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:94: string16 actual_title = title_watcher.WaitAndGetTitle(); The purpose of this is to make sure the main frame navigation complete before the redirection. I think this has been done in the NavigateToURL API. On 2012/12/05 02:02:58, creis wrote: > Do we need this TitleWatcher? I think the NavigateToURL call should be > sufficient. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:100: WindowedNotificationObserver load_observer( On 2012/12/05 02:02:58, creis wrote: > Perhaps the load_observer should be inside NavigateIframeToURL? That's more > consistent with NavigateToURL. Done. https://codereview.chromium.org/11416121/diff/9013/content/browser/site_per_p... content/browser/site_per_process_test.cc:103: &shell()->web_contents()->GetController())); Cannot fit on the previous line. 82 chars. On 2012/12/05 02:02:58, creis wrote: > nit: Wrong indent. (Will this fit on the previous line?) https://codereview.chromium.org/11416121/diff/9013/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/11416121/diff/9013/content/content_tests.gypi... content/content_tests.gypi:692: 'browser/site_per_process_test.cc', On 2012/12/05 02:02:58, creis wrote: > Please alphabetize. Done.
Almost there. Nasko, perhaps you can take a look over this as well, to make sure I'm not missing something? Thanks! https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:170: bool CanLoadPage(const GURL& gurl){ nit: Space before { https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:52: int navigation_result() const { return navigation_succeeded_; } nit: navigation_succeeded() https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:186: } Might as well toss in the other two cases: same-site server redirect to cross-site page same-site client redirect to cross-site page https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:214: // We should check until second client redirect get cancelled. nit: s/check/wait/ https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:218: &shell()->web_contents()->GetController())); The NavigateIFrameToURL line should be between the load_observer2 declaration and the load_observer2.Wait() line. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:253: // Load server-redirect page pointed to a cross-site server-redirect page, It's pointed to a cross-site client-redirect page, right?
A few nits and one important question about the NULL parameter. https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:173: // TODO(creis): We must pass the valid browser_context to convert hosted nit: Having a blank line before the comment seems more readable. https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:176: GURL site_gurl = SiteInstanceImpl::GetSiteForURL(NULL, gurl); Is NULL valid input? It seems to me that it will crash in ChromeContentBrowserClient::GetEffectiveURL when it does ExtensionSystem::Get(profile)->extension_service(). We fixed similar bug in the last few days. https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.h:117: bool CanLoadPage(int child_id, const GURL& url, Style: Each param should go on a new line. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:48: GURL navigation_url() const { Why not return const GURL&? Do we need to make a copy? https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:136: EXPECT_TRUE(NavigateIframeToURL(shell(), This function always takes "test" as the last parameter, why not eliminate it? https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:167: // Load same-site server-redirect page into Iframe. nit: , instead of . at the end of the line. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:206: // Load client-redirect page pointed to a cross-site client-redirect page, nit: s/pointed/pointing/?
https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:176: GURL site_gurl = SiteInstanceImpl::GetSiteForURL(NULL, gurl); On 2012/12/06 17:20:15, nasko wrote: > Is NULL valid input? It seems to me that it will crash in > ChromeContentBrowserClient::GetEffectiveURL when it does > ExtensionSystem::Get(profile)->extension_service(). We fixed similar bug in the > last few days. This is the same setup as CanAccessCookiesForOrigin (below), which was the NULL crash I fixed. It's fine for now. (The only remaining bug is the one in the TODO comment.)
https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:170: bool CanLoadPage(const GURL& gurl){ On 2012/12/06 01:42:45, creis wrote: > nit: Space before { Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.cc:173: // TODO(creis): We must pass the valid browser_context to convert hosted On 2012/12/06 17:20:15, nasko wrote: > nit: Having a blank line before the comment seems more readable. Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/child_pro... content/browser/child_process_security_policy_impl.h:117: bool CanLoadPage(int child_id, const GURL& url, On 2012/12/06 17:20:15, nasko wrote: > Style: Each param should go on a new line. Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:48: GURL navigation_url() const { On 2012/12/06 17:20:15, nasko wrote: > Why not return const GURL&? Do we need to make a copy? Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:52: int navigation_result() const { return navigation_succeeded_; } On 2012/12/06 01:42:45, creis wrote: > nit: navigation_succeeded() Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:136: EXPECT_TRUE(NavigateIframeToURL(shell(), When I designed the NavigateIframeToURL API, I want it to be generic since I believe the future tests may have multiple iframes in the page. Therefore, we should allow the test to choose which iframe should be navigated. On 2012/12/06 17:20:15, nasko wrote: > This function always takes "test" as the last parameter, why not eliminate it? https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:167: // Load same-site server-redirect page into Iframe. On 2012/12/06 17:20:15, nasko wrote: > nit: , instead of . at the end of the line. Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:186: } On 2012/12/06 01:42:45, creis wrote: > Might as well toss in the other two cases: > same-site server redirect to cross-site page > same-site client redirect to cross-site page Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:186: } On 2012/12/06 01:42:45, creis wrote: > Might as well toss in the other two cases: > same-site server redirect to cross-site page > same-site client redirect to cross-site page Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:206: // Load client-redirect page pointed to a cross-site client-redirect page, On 2012/12/06 17:20:15, nasko wrote: > nit: s/pointed/pointing/? Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:214: // We should check until second client redirect get cancelled. On 2012/12/06 01:42:45, creis wrote: > nit: s/check/wait/ Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:218: &shell()->web_contents()->GetController())); I think NavigateIFrameToURL line cannot be between the load_observer2 declaration. This is because load_observer2 is aim at capturing the second load commit/fail event. If put between the load_observer2 declaration, load_observer2 will get the same event as load_observer. On 2012/12/06 01:42:45, creis wrote: > The NavigateIFrameToURL line should be between the load_observer2 declaration > and the load_observer2.Wait() line. Done. https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:253: // Load server-redirect page pointed to a cross-site server-redirect page, On 2012/12/06 01:42:45, creis wrote: > It's pointed to a cross-site client-redirect page, right? Done.
https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:218: &shell()->web_contents()->GetController())); On 2012/12/06 19:10:40, irobert wrote: > I think NavigateIFrameToURL line cannot be between the load_observer2 > declaration. This is because load_observer2 is aim at capturing the second load > commit/fail event. If put between the load_observer2 declaration, load_observer2 > will get the same event as load_observer. > > On 2012/12/06 01:42:45, creis wrote: > > The NavigateIFrameToURL line should be between the load_observer2 declaration > > and the load_observer2.Wait() line. > > Done. Ah. This approach won't work either, though. It will be flaky, because the second LOAD_STOP could occur before we create load_observer2, and then the Wait() call will wait forever. Is there a way to wait for something else, like the DidFailProvisionalLoad? I'm not sure. https://codereview.chromium.org/11416121/diff/37001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/37001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:301: // Load client-redirect page pointing to a cross-site server-redirect page, This is backwards, isn't it? It should be: Load server-redirect page pointing to a cross-site client-redirect page, ...
https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/28001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:218: &shell()->web_contents()->GetController())); I wrote a new NotificationObeserver which wait until the second given signal happened. On 2012/12/06 20:20:22, creis wrote: > On 2012/12/06 19:10:40, irobert wrote: > > I think NavigateIFrameToURL line cannot be between the load_observer2 > > declaration. This is because load_observer2 is aim at capturing the second > load > > commit/fail event. If put between the load_observer2 declaration, > load_observer2 > > will get the same event as load_observer. > > > > On 2012/12/06 01:42:45, creis wrote: > > > The NavigateIFrameToURL line should be between the load_observer2 > declaration > > > and the load_observer2.Wait() line. > > > > Done. > > Ah. This approach won't work either, though. It will be flaky, because the > second LOAD_STOP could occur before we create load_observer2, and then the > Wait() call will wait forever. > > Is there a way to wait for something else, like the DidFailProvisionalLoad? I'm > not sure. https://codereview.chromium.org/11416121/diff/37001/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/37001/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:301: // Load client-redirect page pointing to a cross-site server-redirect page, You are right. I confused myself. :) On 2012/12/06 20:20:22, creis wrote: > This is backwards, isn't it? It should be: > Load server-redirect page pointing to a cross-site client-redirect page, ... Done.
Great, LGTM. Nasko, if it looks good to you, I'll land it tomorrow.
Let's try not to introduce a new observer. If UrlLoadObserver can work, it will be nice. https://codereview.chromium.org/11416121/diff/50003/content/browser/site_per_... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/11416121/diff/50003/content/browser/site_per_... content/browser/site_per_process_browsertest.cc:63: class RedirectNotificationObserver : public NotificationObserver { This doesn't look much different than other observer objects that already exist. We should be able to use UrlLoadObserver from ui_test_utils.h to monitor URL loads.
We tried using the UrlLoadObserver, but it doesn't handle iframe navigations properly. I'm fine using the new object for now and writing a proper observer to handle iframes as part of the out-of-process iframes work. LGTM
On 2012/12/07 18:11:34, nasko wrote: > We tried using the UrlLoadObserver, but it doesn't handle iframe navigations > properly. I'm fine using the new object for now and writing a proper observer to > handle iframes as part of the out-of-process iframes work. > > LGTM Thanks. I just noticed as I went to commit that the test is failing on the bots at the moment, though, so we'll need to fix that first.
Great. Looks like it's passing the tests now, so LGTM. I'll land it tomorrow morning.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/irobert@chromium.org/11416121/75002
Presubmit check for 11416121-75002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.2s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Darin, can you give an owner's review on chrome/browser/chrome_content_browser_client_browsertest.cc? We needed to change the test to use a normal URL instead of WebUI, since the --site-per-process flag is interfering with WebUI pages at the moment. (I'll look into a separate fix for that, but it only matters when using the flag.)
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/irobert@chromium.org/11416121/75002
Message was sent while issue was closed.
Change committed as 172403 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
