WIP / not-at-all-ready-to-land CL is @ https://crrev.com/2394343002

Tests that are currently failing:

- content_unittests:
  - *ResourceDispatcherHostTest.Transfer*
- browser_tests
  - AppApiTest.ServerRedirectToAppFromExtension
  - PrerenderBrowserTest.PrerenderCrossProcessServerRedirectNoHang
  - PrerenderBrowserTest.PrerenderCrossProcessServerRedirect

The prerender browser tests are failing, because after the changes, there is no process swap / no call to ShouldTransferNavigation or OpenURLFromTab method of PrerenderContents::WebContentsDelegateImpl (and consequently the tests ends up waiting forever for FINAL_STATUS_OPEN_URL prerender status).  I think this is just a test artifact - I think I can preserve more or less the same test coverage by having the test opt into --site-per-process mode and trigger process swaps via urls handled by content::SetupCrossSiteRedirector.

I haven't yet looked into the other test failures.

Let's find a chance to discuss.  I haven't read all the details here yet, but DoesSiteRequireDedicatedProcess is not a sufficient replacement for ShouldSwapProcessesForRedirect.  (Extensions, for example, require a process swap but cannot use dedicated processes, because they can share with each other.)

Also, I don't think ShouldSwapProcessesForRedirect is called after each redirect.  It's called from MaybeTransferAndProceed after all redirects have occurred.  (Is that incorrect?)

That said, I would definitely believe that we have some bugs in how it's used, and it's worth looking closer.

RE: dedicated

Yes, that does seem weird.  But... currently we have ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess returning true for all extensions (I believe we had the same behavior even before the recent r423360, starting with the very first version of DoesSiteRequireDedicatedProcess from r354038).  I am not sure if this is a naming or a behavior problem (i.e. if it might be sufficient to just get rid of the "Dedicated" part in the method name).

At any rate, this (existing) problem seems orthogonal to the desire to remove ShouldSwapProcessesForRedirect.


RE: I don't think ShouldSwapProcessesForRedirect is called after each redirect.  It's called from MaybeTransferAndProceed after all redirects have occurred.  (Is that incorrect?)

No - this is correct.  ShouldSwapProcessesForRedirect is only called once per navigation (not once per redirect).

So - maybe ShouldSwapProcessesForRedirect just needs a name change (to remove the "ForRedirect" part).


RE: redundancy

Let me try to expand and clarify what I mean by "redundancy".  In some navigation paths ShouldSwapProcessesForRedirect is not used at all.  For example - when running the test (NavigatingExtensionPopupBrowserTest.Webpage) that navigates an extension popup to a webpage (using POST, so no OpenURL path in 2nd part of the test) we have:

- First ShouldSwapProcessesForRedirect is called and returns false (no need for a process swap) because old_url == new_url (there were no redirects);  both urls are equal to "http://foo.com:44920/title1.html".  Stack captured below [1].

- We detect a need for a transfer in DoesSiteRequireDedicatedProcess, which returns true, because effective_site_url (chrome-extension://jfehpflefiffgmegfjgojimaajiiaokc) is an extension (and not a hosted app, platform app or CWS).  Stack captured below [2].

- Finally (having decided that a transfer is needed), we ask the extensions popup host if a transfer is desirable.  It answers "no, don't transfer the main frame please", so the whole navigation fails (as expected).  Stack captured below [3].

Based on the above, I would hope that all navigation paths can figure out whether to do a transfer via DoesSiteRequireDedicatedProcess (and that we could remove ShouldSwapProcessesForRedirect because in the scenario above is was useless).  In other words - if there are navigation scenarios where ShouldSwapProcessesForRedirect cannot be relied upon, then we need to have another way of enforcing process transfers (and if so, ShouldSwapProcessesForRedirect seems redundant / replacable with this other way).

[1]
#2 0x000005bb5c34 extensions::ChromeContentBrowserClientExtensionsPart::ShouldSwapProcessesForRedirect()
#3 0x7f0489daf659 content::CrossSiteResourceHandler::OnNormalResponseStarted()
#4 0x7f0489db5f05 content::InterceptingResourceHandler::OnResponseStarted()
#5 0x7f0489e317f5 content::ThrottlingResourceHandler::OnResponseStarted()
#6 0x7f0489e32b13 content::ThrottlingResourceHandler::ResumeResponse()
#7 0x7f0489e31f71 content::ThrottlingResourceHandler::Resume()
#8 0x7f0489dc86c5 content::NavigationResourceThrottle::OnUIChecksPerformed()

[2]
#2 0x000005bb3f56 extensions::ChromeContentBrowserClientExtensionsPart::DoesSiteRequireDedicatedProcess()
#3 0x7f048a37c89d content::SiteInstanceImpl::DoesSiteRequireDedicatedProcess()
#4 0x7f0489b83f9a content::RenderFrameHostManager::IsRendererTransferNeededForNavigation()
#5 0x7f0489db2067 content::(anonymous namespace)::CheckNavigationPolicyOnUI()

[3] 
#2 0x000005e99dc4 extensions::ExtensionViewHost::ShouldTransferNavigation()
#3 0x7f784be4ff33 content::NavigatorImpl::RequestTransferURL()
#4 0x7f784beac7f1 content::RenderFrameHostManager::OnCrossSiteResponse()
#5 0x7f784be7e35c content::RenderFrameHostImpl::OnCrossSiteResponse()
#6 0x7f784c0e0e09 content::(anonymous namespace)::OnCrossSiteResponseHelper()


Now obviously, my argumentation above breaks down if there are navigation scenarios where ShouldSwapProcessesForRedirect is the only thing ensuring a process swap (e.g. because DoesSiteRequireDedicatedProcess is not called in these scenarios OR because DoesSiteRequireDedicatedProcess is called and returns a wrong thing;  I think both cases might be seen as bugs and we might still want to unify process swapping decisions into a single content embedders method).  QUESTION: Are there such navigation scenarios?

One other thing to note is that when running ChromeWebStoreProcessTest.NavigateWebTabToChromeWebStoreViaPost, ShouldSwapProcessesForRedirect is invoked with current_url == new_url == http://chrome.webstore.test.com:51687/title1.html which is misleading - this URL looks like a web/http/https URL, but in reality it will be handled by CWS app.  I am not sure exactly how apps "handle" URLs coming from the web, but it seems configurable by https://developer.chrome.com/apps/manifest/url_handlers (although I am not quite sure if CWS uses the same mechanism).  In contrast, DoesSiteRequireDedicatedProcess is getting called with effective_site_url = chrome-extension://ahfgeienlihckogmohjhadlkjgocpleb (belonging to CWS app in case of this browser test).

BTW: ShouldSwapProcessesForRedirect is only redundant in --isolate-extension mode (in this mode DoesSiteRequireDedicatedProcess is the ultimate decision maker for site isolation).  This means that we cannot really remove ShouldSwapProcessesForRedirect until --isolate-extension mode ships (i.e. can no longer be turned off by a trial).

Remaining issues I am aware of:
1. PrerenderBrowserTest.*ServerRedirect* - I think these tests can just be deleted once PlzNavigate ships
2. AppApiTest.ServerRedirectToAppFromExtension - I am not sure what is going on here, but I think this is my fault (i.e. I think I should have moved some of the ShouldSwapProcessesForRedirect logic elsewhere - maybe into  IsRendererTransferNeededForNavigation).

 Issue 723083  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa86d9b3d691f2aee04dce0527de1632f51c3a74

commit fa86d9b3d691f2aee04dce0527de1632f51c3a74
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Dec 05 23:56:24 2017

Removing ContentBrowserClient::ShouldSwapProcessesForRedirect.

Only the final URL (after all the redirect hops) should matter for
choosing the renderer process to host a frame after a navigation (this
is definitely true for PlzNavigate but should also be true without
PlzNavigate).  Therefore - this CL removes the
ShouldSwapProcessesForRedirect method - this method is not really
needed now.

Bug:  655214 
Change-Id: I18c03d0ebcbb7a0fd673c52bc451186ec909f78b
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Tbr: sky@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/792005
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521904}
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/devtools/devtools_sanity_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/api/web_request/web_request_apitest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/chrome_extension_web_contents_observer.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/extension_apitest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/site_details_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/common/BUILD.gn
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/renderer/BUILD.gn
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[rename] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/renderer/extensions/extension_process_policy.cc
[rename] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/renderer/extensions/extension_process_policy.h
[rename] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/renderer/extensions/extension_process_policy_unittest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/chrome/test/BUILD.gn
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/browser/cross_site_transfer_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/browser/loader/resource_dispatcher_host_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/public/browser/content_browser_client.h
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/fa86d9b3d691f2aee04dce0527de1632f51c3a74/content/shell/browser/shell_content_browser_client.h