|
|
Created:
4 years, 2 months ago by jam Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate.
The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread.
This fixes
ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed
with PlzNavigate.
BUG=504347, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d
Cr-Commit-Position: refs/heads/master@{#424937}
Patch Set 1 #
Total comments: 13
Patch Set 2 : review comments #
Total comments: 10
Patch Set 3 : remove assert #Patch Set 4 : review comments #
Total comments: 5
Patch Set 5 : review nits #
Dependent Patchsets: Messages
Total messages: 37 (26 generated)
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with PlzNavigate (although the latter one also needs another fix that Alex is working on). BUG=504347,645028 ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with PlzNavigate (although the latter one also needs another fix that Alex is working on). BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with PlzNavigate (although the latter one also needs another fix that Alex is working on). BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with PlzNavigate (although the latter one also needs the fix for http://crbug.com/651980 that Alex is working on). BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + alexmos@chromium.org, nasko@chromium.org
This change supercedes https://codereview.chromium.org/2401443002/. Per Nasko's suggestion, I moved the blocking to the navigation throttle. https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... extensions/browser/extension_navigation_throttle.cc:41: is_extension = !!registry->enabled_extensions().GetExtensionOrAppByURL( fyi this is based on translating the previous check in ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest, which did: xtension_info_map_->process_map().Contains(info->GetChildID()) The map entry is inserted in ChromeContentBrowserClientExtensionsPart::SiteInstanceGotProcess which calls: registry->enabled_extensions().GetExtensionOrAppByURL( site_instance->GetSiteURL()); to get the extension
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some preliminary comments. Alex and I have some brainstorming to do, so we'll update you when we have an update. https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:341: GURL site_url_; Let's store the SiteInstance here instead of the URL. Just the URL won't be good enough for process model decisions and we can ask the SiteInstance for its underlying process. Also, let's name it something like creator_site_instance_, so it is clear that this is for the RFH that created the request. We will have to consolidate it later with support for knowing who initiated the navigation (which can be different), but that is for other CLs. https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:302: frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); This is not going to give you the correct RFH. It will pick the RFH in which we must commit this the navigation, not the one that started it. Alex and I are looking through code to see what is the correct thing to use here and will get back to you, as there isn't a convenient way right now that will do the right thing. https://codereview.chromium.org/2411693003/diff/1/content/public/browser/navi... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2411693003/diff/1/content/public/browser/navi... content/public/browser/navigation_handle.h:52: virtual const GURL& GetSiteURL() = 0; Let's expose the SiteInstance, as we will need it for other checks too. https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... extensions/browser/extension_navigation_throttle.cc:41: is_extension = !!registry->enabled_extensions().GetExtensionOrAppByURL( On 2016/10/11 19:09:06, jam wrote: > fyi this is based on translating the previous check in > ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest, which did: > xtension_info_map_->process_map().Contains(info->GetChildID()) > > The map entry is inserted in > ChromeContentBrowserClientExtensionsPart::SiteInstanceGotProcess > which calls: > registry->enabled_extensions().GetExtensionOrAppByURL( > site_instance->GetSiteURL()); > to get the extension Ideally, I'd like to keep the process id check as well. If we have the SiteInstance here, we can ensure that the process for the SiteInstance is in the extensions process map, which I think is accessible on the UI thread too.
https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:302: frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); To double-check what happens (since I was confused by some of the PlzNav logic), I patched in the CL and ran the ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked test with PlzNav, and I saw failures that indicated that the bad navigations were allowed. So yes, this picks the wrong SiteInstance. By the way, to get around the test crash due to DCHECK(!frame_widget_) (which you'd get if the bad navigations are blocked), you can try patching in my draft fix at https://codereview.chromium.org/2410153005/. Actually, the Blocked test passes without any further modifications with just that fix. https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:199: render_frame_host->GetSiteInstance()->GetSiteURL(), Similar concern here about whether this picks the right SiteInstance on the non-PlzNav path. Isn't this also going get the RFH in which the navigation will commit, rather than the one in which it originated? https://codereview.chromium.org/2411693003/diff/1/content/public/browser/navi... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2411693003/diff/1/content/public/browser/navi... content/public/browser/navigation_handle.h:52: virtual const GURL& GetSiteURL() = 0; Could we name this so that it's a bit more clear what SiteInstance this is (i.e., the creator's and not the target's)?
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with PlzNavigate (although the latter one also needs the fix for http://crbug.com/651980 that Alex is working on). BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:341: GURL site_url_; On 2016/10/12 17:00:35, nasko (slow) wrote: > Let's store the SiteInstance here instead of the URL. Just the URL won't be good > enough for process model decisions and we can ask the SiteInstance for its > underlying process. > > Also, let's name it something like creator_site_instance_, so it is clear that > this is for the RFH that created the request. We will have to consolidate it > later with support for knowing who initiated the navigation (which can be > different), but that is for other CLs. Done. https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:302: frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); On 2016/10/12 17:00:35, nasko (slow) wrote: > This is not going to give you the correct RFH. It will pick the RFH in which we > must commit this the navigation, not the one that started it. > > Alex and I are looking through code to see what is the correct thing to use here > and will get back to you, as there isn't a convenient way right now that will do > the right thing. thanks, I added a test based on your description and got the site instance from the frame tree node's current frame host. It's not clear to me if this is sufficient, or if sometimes I need to use the pending/speculative frame host if the former doesn't exist because it's a new frame? https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... extensions/browser/extension_navigation_throttle.cc:41: is_extension = !!registry->enabled_extensions().GetExtensionOrAppByURL( On 2016/10/12 17:00:35, nasko (slow) wrote: > On 2016/10/11 19:09:06, jam wrote: > > fyi this is based on translating the previous check in > > ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest, which did: > > xtension_info_map_->process_map().Contains(info->GetChildID()) > > > > The map entry is inserted in > > ChromeContentBrowserClientExtensionsPart::SiteInstanceGotProcess > > which calls: > > registry->enabled_extensions().GetExtensionOrAppByURL( > > site_instance->GetSiteURL()); > > to get the extension > > Ideally, I'd like to keep the process id check as well. If we have the > SiteInstance here, we can ensure that the process for the SiteInstance is in the > extensions process map, which I think is accessible on the UI thread too. The old code didn't check that it was in the map, it was checking if it's _not_ in it so I'm not sure what you mean? I tried adding it in patchset 2 and it gave a few browser_tests failures related to apps, i.e. for ChromeAppAPITest.InstallAndRunningState. I guess the process map won't have entries for chrome apps processes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, Nasko and I looked at this together, and I'm writing up the combined comments from both of us. The new approach looks nice, and these are mostly nits. https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:302: frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); On 2016/10/12 22:10:45, jam wrote: > On 2016/10/12 17:00:35, nasko (slow) wrote: > > This is not going to give you the correct RFH. It will pick the RFH in which > we > > must commit this the navigation, not the one that started it. > > > > Alex and I are looking through code to see what is the correct thing to use > here > > and will get back to you, as there isn't a convenient way right now that will > do > > the right thing. > > thanks, I added a test based on your description and got the site instance from > the frame tree node's current frame host. It's not clear to me if this is > sufficient, or if sometimes I need to use the pending/speculative frame host if > the former doesn't exist because it's a new frame? Thanks, the test looks nice and makes it very clear what this returns. We like the new approach, it should just work for all the cases we're concerned about. New frames should also be fine, as they always start out in the SiteInstance of their creator, so the current_frame_host() should give the right SiteInstance in that case. https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2411693003/diff/1/extensions/browser/extensio... extensions/browser/extension_navigation_throttle.cc:41: is_extension = !!registry->enabled_extensions().GetExtensionOrAppByURL( On 2016/10/12 22:10:45, jam wrote: > On 2016/10/12 17:00:35, nasko (slow) wrote: > > On 2016/10/11 19:09:06, jam wrote: > > > fyi this is based on translating the previous check in > > > ChromeExtensionsNetworkDelegateImpl::OnBeforeURLRequest, which did: > > > xtension_info_map_->process_map().Contains(info->GetChildID()) > > > > > > The map entry is inserted in > > > ChromeContentBrowserClientExtensionsPart::SiteInstanceGotProcess > > > which calls: > > > registry->enabled_extensions().GetExtensionOrAppByURL( > > > site_instance->GetSiteURL()); > > > to get the extension > > > > Ideally, I'd like to keep the process id check as well. If we have the > > SiteInstance here, we can ensure that the process for the SiteInstance is in > the > > extensions process map, which I think is accessible on the UI thread too. > > The old code didn't check that it was in the map, it was checking if it's _not_ > in it so I'm not sure what you mean? Yes, Nasko meant the reverse (what you tried). > > I tried adding it in patchset 2 and it gave a few browser_tests failures related > to apps, i.e. for ChromeAppAPITest.InstallAndRunningState. I guess the process > map won't have entries for chrome apps processes? Ah, let's not do the process check in this CL then. We will investigate and follow up on this separately from our side. Seems like the SiteInstance check should cover the cases we care about. https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:97: frame_tree_node_->render_manager()->current_frame_host(); nit: This can be shortened to frame_tree_node_->current_frame_host() https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8360: // Verifies that the site instance of a navigation handle correctly identifies nit: s/site instance/SiteInstance/, s/navigation handle/NavigationHandle/ https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8363: IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigationHandleSiteIntance) { nit: s/Intance/Instance/ https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... content/public/browser/navigation_handle.h:52: // Returns the SiteInstance that created the request. To eliminate any possible confusion with the initiator, would it be worth adding a concrete example to the comment, along the lines of: If a frame in SiteInstance A navigates a frame in SiteInstance B to a URL in SiteInstance C, then this returns B. https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... content/public/browser/navigation_handle.h:53: virtual SiteInstance* GetCreatorSiteInstance() = 0; We had some more discussions about the name, and we think it might be better to name this GetStartingSiteInstance(). WDYT?
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:97: frame_tree_node_->render_manager()->current_frame_host(); On 2016/10/12 23:13:28, alexmos wrote: > nit: This can be shortened to frame_tree_node_->current_frame_host() Done. https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8360: // Verifies that the site instance of a navigation handle correctly identifies On 2016/10/12 23:13:28, alexmos wrote: > nit: s/site instance/SiteInstance/, s/navigation handle/NavigationHandle/ Done. https://codereview.chromium.org/2411693003/diff/40001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8363: IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigationHandleSiteIntance) { On 2016/10/12 23:13:28, alexmos wrote: > nit: s/Intance/Instance/ Done. https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... content/public/browser/navigation_handle.h:52: // Returns the SiteInstance that created the request. On 2016/10/12 23:13:28, alexmos wrote: > To eliminate any possible confusion with the initiator, would it be worth adding > a concrete example to the comment, along the lines of: If a frame in > SiteInstance A navigates a frame in SiteInstance B to a URL in SiteInstance C, > then this returns B. Done. https://codereview.chromium.org/2411693003/diff/40001/content/public/browser/... content/public/browser/navigation_handle.h:53: virtual SiteInstance* GetCreatorSiteInstance() = 0; On 2016/10/12 23:13:28, alexmos wrote: > We had some more discussions about the name, and we think it might be better to > name this GetStartingSiteInstance(). WDYT? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Thanks, LGTM with nits. https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:17: #include "content/browser/frame_host/render_frame_host_manager.h" nit: is this still necessary? https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8350: class NavigatigationHandleWatcher : public WebContentsObserver { nit: s/Navigatigation/Navigation/ https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8356: navigation_handle->GetCreatorSiteInstance()->GetSiteURL().spec()); GetStartingSiteInstance (should fix compile)
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2411693003/#ps100001 (title: "review nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347,645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d Cr-Commit-Position: refs/heads/master@{#424937} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d Cr-Commit-Position: refs/heads/master@{#424937}
Message was sent while issue was closed.
https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:17: #include "content/browser/frame_host/render_frame_host_manager.h" On 2016/10/13 00:46:27, alexmos wrote: > nit: is this still necessary? Done. https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:8350: class NavigatigationHandleWatcher : public WebContentsObserver { On 2016/10/13 00:46:27, alexmos wrote: > nit: s/Navigatigation/Navigation/ Done. |