Change in cross-origin LoadURL renderer process creation with M78?

28 views
Skip to first unread message

Marshall Greenblatt

unread,
Sep 25, 2019, 6:10:41 AM9/25/19
to Chromium Embedders
Hi All,

I'm trying to identify the cause of a behavior change in one of my unit tests which uses WebContentsDelegate::OpenURLFromTab and NavigationController::LoadURL. The test clicks a link [1] with the left mouse button while holding down the CTRL key. This results in a call to the OpenURLFromTab method. My implementation of  OpenURLFromTab calls |source->GetController().LoadURL| with the values from OpenURLParams [2] and then returns the same |source| WebContents. With M77 and older (tested with 77.0.3865.56) this would create a new renderer process [3] due to the link URL being from a different origin. Now, with M78 (tested with 78.0.3904.0), the same renderer process is being reused for the cross-origin navigation. The OpenURLParams values themselves appear unchanged between M77 and M78.

Can anyone point me to the Chromium change that might be responsible for this new behavior?

Note that the below debug values are from Windows 10 but the same issue appears to exist on at least macOS as well.

Thanks,
Marshall

[1] Nothing fancy, just: <a href="http://tests-conav2.com/nav2.html">Click me</a>

[2]  OpenURLParams values:

NameValueType
url0x28860c18 "http://tests-conav2.com/nav2.html"GURL
referrer{url=0x28860a98 "http://tests-conav1.com/nav1.html" policy=kNoReferrerWhenDowngrade (2) }content::Referrer
initiator_origin{tuple_={scheme_=0x00afd948 "http" host_=0x287a2488 "tests-conav1.com" port_=80 } nonce_=(null) }base::Optional<url::Origin>
source_site_instance[4] 0x285f4b60 RefCount: 4scoped_refptr<content::SiteInstance>
redirect_chain{ size=0 }std::__1::vector<GURL,std::__1::allocator<GURL>>
uses_postfalsebool
post_datanullscoped_refptr<network::ResourceRequestBody>
extra_headers0x287a21b8 "Upgrade-Insecure-Requests: 1"std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>>
frame_tree_node_id-1int
source_render_frame_id3int
source_render_process_id4int
dispositionNEW_BACKGROUND_TAB (4)WindowOpenDisposition
transitionPAGE_TRANSITION_FIRST (0)ui::PageTransition
is_renderer_initiatedtruebool
should_replace_current_entryfalsebool
user_gesturetruebool
triggering_event_infokFromTrustedEvent (2)blink::WebTriggeringEventInfo
started_from_context_menufalsebool
blob_url_loader_factorynullscoped_refptr<network::SharedURLLoaderFactory>
open_app_window_if_possiblefalsebool
href_translate0x00afd9cc ""std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>>
reload_typeNONE (0)content::ReloadType

[3] Renderer process command lines:

With M77: --type=renderer --js-flags=--expose-gc --log-file="<path>" --field-trial-handle=1536,3793632140430602784,5062479849261668425,131072 --disable-features=OutOfBlinkCors --lang=en-US --uncaught-exception-stack-size=10 --device-scale-factor=1 --num-raster-threads=4 --enable-main-frame-before-activation --service-request-channel-token=10380488957069589981 --renderer-client-id=6 --mojo-platform-channel-handle=2360 /prefetch:1  

With M78: --type=renderer --js-flags=--expose-gc --log-file="<path>" --field-trial-handle=1564,9847037342025122997,7650305657514794350,131072 --disable-features=MimeHandlerViewInCrossProcessFrame,OutOfBlinkCors --lang=en-US --uncaught-exception-stack-size=10 --device-scale-factor=1 --num-raster-threads=4 --enable-main-frame-before-activation --service-request-channel-token=3152359196949456461 --renderer-client-id=4 --mojo-platform-channel-handle=2280 /prefetch:1

John Abd-El-Malek

unread,
Sep 25, 2019, 4:01:10 PM9/25/19
to Marshall Greenblatt, Chromium Embedders
Have you tried bisecting this?

--
You received this message because you are subscribed to the Google Groups "Chromium Embedders" group.
To unsubscribe from this group and stop receiving emails from it, send an email to embedder-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/CAFW9UJ-02QsDqhL7goX2J4E-UKwk0n%3D34XVjEjDoKmzomS0vRw%40mail.gmail.com.

Marshall Greenblatt

unread,
Sep 26, 2019, 7:17:55 AM9/26/19
to John Abd-El-Malek, Chromium Embedders
Thanks for the suggestion :) The behavior change is due to ProcessSharingWithDefaultSiteInstances (added here and enabled by default here after M77 branched).

Alex Moshchuk

unread,
Sep 26, 2019, 1:44:03 PM9/26/19
to Marshall Greenblatt, acol...@chromium.org, John Abd-El-Malek, Chromium Embedders, Site Isolation Development
+acolwell@, who worked on the default SiteInstance change, and +site-isolation-dev@ as FYI.  I'm surprised this change affects you on Windows, since we should be using full site isolation there by default, which should force all cross-site navigations to swap processes.  Are you returning true from your implementation of ContentBrowserClient::ShouldEnableStrictSiteIsolation(), if any (it's true by default)?

Aaron Colwell

unread,
Sep 26, 2019, 2:25:06 PM9/26/19
to Alex Moshchuk, Marshall Greenblatt, John Abd-El-Malek, Chromium Embedders, Site Isolation Development
Hi Marshall,
I'm sorry this is causing problems for you. I agree with Alex, that I'd be surprised that this was happening if you are returning true for  ContentBrowserClient::ShouldEnableStrictSiteIsolation() which is the default for Windows and Mac. If you happen to have a custom implementation of that method which returns false, then you'll need to update your client to override ContentBrowserClient::DoesSiteRequireDedicatedProcess() to return true for sites that you want to be separated. The logic that controls all this is in SiteInstanceImpl::DoesSiteURLRequireDedicatedProcess() and SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(). The ProcessSharingWithDefaultSiteInstances feature allows a process to be shared by multiple sites if SiteIsolation is disabled and the ContentBrowserClient indicates that the site does not require a dedicated process. This feature was added to help lower the resource requirements for sites that did not need isolation on platforms, like Android, where the isolation vs resources tradeoff is different than desktop.

I hope this makes sense and helps you figure out a path forward. Please let me know if you run into more problems or have other questions.

Aaron

Marshall Greenblatt

unread,
Sep 26, 2019, 2:33:12 PM9/26/19
to Aaron Colwell, Alex Moshchuk, John Abd-El-Malek, Chromium Embedders, Site Isolation Development
On Thu, Sep 26, 2019 at 9:25 PM Aaron Colwell <acol...@chromium.org> wrote:
Hi Marshall,
I'm sorry this is causing problems for you. I agree with Alex, that I'd be surprised that this was happening if you are returning true for  ContentBrowserClient::ShouldEnableStrictSiteIsolation() which is the default for Windows and Mac. If you happen to have a custom implementation of that method which returns false, then you'll need to update your client to override ContentBrowserClient::DoesSiteRequireDedicatedProcess() to return true for sites that you want to be separated. The logic that controls all this is in SiteInstanceImpl::DoesSiteURLRequireDedicatedProcess() and SiteInstanceImpl::CanBePlacedInDefaultSiteInstance(). The ProcessSharingWithDefaultSiteInstances feature allows a process to be shared by multiple sites if SiteIsolation is disabled and the ContentBrowserClient indicates that the site does not require a dedicated process. This feature was added to help lower the resource requirements for sites that did not need isolation on platforms, like Android, where the isolation vs resources tradeoff is different than desktop.

I hope this makes sense and helps you figure out a path forward. Please let me know if you run into more problems or have other questions.

Thanks for the explanation, that makes a lot of sense. We're currently return false from ShouldEnableStrictSiteIsolation because of some strange behaviors that we observed back when that method was introduced (there are details in [1] if you're curious). I'll test whether those problems are now resolved, in which case we can start returning the platform default value.

Marshall Greenblatt

unread,
Oct 3, 2019, 6:15:54 AM10/3/19
to Aaron Colwell, Alex Moshchuk, John Abd-El-Malek, Chromium Embedders, Site Isolation Development
Hi Aaron,

I'm testing ShouldEnableStrictSiteIsolation returning true in M78 (78.0.3904.0) and I'm seeing some strange renderer callback behavior that perhaps you can help me with.

In my testing I'm creating two WebContents that load different origins. When I create and navigate the 1st WebContents I get the  ContentRendererClient::RenderThreadStarted and ContentRendererClient::RenderFrameCreated callbacks in the 1st renderer process as expected. However, when I create and navigate the 2nd WebContents (which loads a different origin) I get the RenderFrameCreated callback in the 1st renderer process. The 2nd renderer process is then created and I get the RenderThreadStarted callback in the 2nd renderer process. There is no additional call to RenderFrameCreated in the 2nd renderer process.

I use these callbacks to set up state in the renderer process, so the lack of both callbacks in the 2nd renderer process is problematic for me.

Is this expected behavior? If not, are there any additional flags or settings that I should check which might be causing this behavior?

Thanks,
Marshall

Marshall Greenblatt

unread,
Oct 3, 2019, 7:42:47 AM10/3/19
to Aaron Colwell, Alex Moshchuk, John Abd-El-Malek, Chromium Embedders, Site Isolation Development
On Thu, Oct 3, 2019 at 1:15 PM Marshall Greenblatt <magree...@gmail.com> wrote:
Hi Aaron,

I'm testing ShouldEnableStrictSiteIsolation returning true in M78 (78.0.3904.0) and I'm seeing some strange renderer callback behavior that perhaps you can help me with.

In my testing I'm creating two WebContents that load different origins. When I create and navigate the 1st WebContents I get the  ContentRendererClient::RenderThreadStarted and ContentRendererClient::RenderFrameCreated callbacks in the 1st renderer process as expected. However, when I create and navigate the 2nd WebContents (which loads a different origin) I get the RenderFrameCreated callback in the 1st renderer process. The 2nd renderer process is then created and I get the RenderThreadStarted callback in the 2nd renderer process. There is no additional call to RenderFrameCreated in the 2nd renderer process.

OK, I think I understand what's happening. It looks like 3 renderer processes are being created ([1], [2] and [3]) . [1] and [2] are created at the same time as the 1st WebContents. [3] is created at the same time as the 2nd WebContents. Navigation and callbacks for the 2nd WebContents occur in the already existing renderer process created via [2], and [3] is now spare (presumably to be used for the next WebContents).

So, for me, the problem is that I'm sending state to [3] in response to the RenderThreadStarted callback (using sync IPC) when I actually want that state in [2]. I'll need to find more appropriate callback for this use case.

[1] > content.dll!content::RenderProcessHostImpl::RenderProcessHostImpl(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, bool is_for_guests_only) Line 1472 C++
  content.dll!content::RenderProcessHostImpl::CreateRenderProcessHost(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, content::SiteInstance * site_instance, bool is_for_guests_only) Line 1413 C++
  content.dll!content::RenderProcessHostImpl::GetProcessHostForSiteInstance(content::SiteInstanceImpl * site_instance) Line 3999 C++
  content.dll!content::SiteInstanceImpl::GetProcess() Line 267 C++
  content.dll!content::WebContentsImpl::Init(const content::WebContents::CreateParams & params) Line 2124 C++
  content.dll!content::WebContentsImpl::CreateWithOpener(const content::WebContents::CreateParams & params, content::RenderFrameHostImpl * opener_rfh) Line 799 C++
  content.dll!content::WebContents::Create(const content::WebContents::CreateParams & params) Line 308 C++
  libcef.dll!CefBrowserHostImpl::Create(CefBrowserHostImpl::CreateParams & create_params) Line 365 C++

[2] > content.dll!content::RenderProcessHostImpl::RenderProcessHostImpl(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, bool is_for_guests_only) Line 1472 C++
  content.dll!content::RenderProcessHostImpl::CreateRenderProcessHost(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, content::SiteInstance * site_instance, bool is_for_guests_only) Line 1413 C++
  content.dll!content::`anonymous namespace'::SpareRenderProcessHostManager::WarmupSpareRenderProcessHost(content::BrowserContext * browser_context) Line 514 C++
  content.dll!content::`anonymous namespace'::SpareRenderProcessHostManager::PrepareForFutureRequests(content::BrowserContext * browser_context) Line 603 C++
  content.dll!content::RenderProcessHostImpl::NotifySpareManagerAboutRecentlyUsedBrowserContext(content::BrowserContext * browser_context) Line 2728 C++
  content.dll!content::RenderFrameHostManager::GetSiteInstanceForNavigation(const GURL & dest_url, content::SiteInstanceImpl * source_instance, content::SiteInstanceImpl * dest_instance, content::SiteInstanceImpl * candidate_instance, ui::PageTransition transition, bool is_failure, bool dest_is_restore, bool dest_is_view_source_mode, bool was_server_redirect) Line 1309 C++
  content.dll!content::RenderFrameHostManager::GetSiteInstanceForNavigationRequest(const content::NavigationRequest & request) Line 2295 C++
  content.dll!content::RenderFrameHostManager::GetFrameHostForNavigation(const content::NavigationRequest & request) Line 664 C++
  content.dll!content::RenderFrameHostManager::DidCreateNavigationRequest(content::NavigationRequest * request) Line 631 C++
  content.dll!content::FrameTreeNode::CreatedNavigationRequest(std::__1::unique_ptr<content::NavigationRequest,std::__1::default_delete<content::NavigationRequest>> navigation_request) Line 445 C++
  content.dll!content::NavigatorImpl::Navigate(std::__1::unique_ptr<content::NavigationRequest,std::__1::default_delete<content::NavigationRequest>> request, content::ReloadType reload_type, content::RestoreType restore_type) Line 374 C++
  content.dll!content::NavigationControllerImpl::NavigateWithoutEntry(const content::NavigationController::LoadURLParams & params) Line 2876 C++
  content.dll!content::NavigationControllerImpl::LoadURLWithParams(const content::NavigationController::LoadURLParams & params) Line 956 C++
  content.dll!content::NavigationControllerImpl::LoadURL(const GURL & url, const content::Referrer & referrer, ui::PageTransition transition, const std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>> & extra_headers) Line 919 C++

[3] > content.dll!content::RenderProcessHostImpl::RenderProcessHostImpl(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, bool is_for_guests_only) Line 1472 C++
  content.dll!content::RenderProcessHostImpl::CreateRenderProcessHost(content::BrowserContext * browser_context, content::StoragePartitionImpl * storage_partition_impl, content::SiteInstance * site_instance, bool is_for_guests_only) Line 1413 C++
  content.dll!content::`anonymous namespace'::SpareRenderProcessHostManager::WarmupSpareRenderProcessHost(content::BrowserContext * browser_context) Line 514 C++
  content.dll!content::`anonymous namespace'::SpareRenderProcessHostManager::PrepareForFutureRequests(content::BrowserContext * browser_context) Line 603 C++
  content.dll!content::RenderProcessHostImpl::GetProcessHostForSiteInstance(content::SiteInstanceImpl * site_instance) Line 4013 C++
  content.dll!content::SiteInstanceImpl::GetProcess() Line 267 C++
  content.dll!content::WebContentsImpl::Init(const content::WebContents::CreateParams & params) Line 2124 C++
  content.dll!content::WebContentsImpl::CreateWithOpener(const content::WebContents::CreateParams & params, content::RenderFrameHostImpl * opener_rfh) Line 799 C++
  content.dll!content::WebContents::Create(const content::WebContents::CreateParams & params) Line 308 C++
Reply all
Reply to author
Forward
0 new messages