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

[Marshall Greenblatt]

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:

	Name	Value	Type
▶	url	0x28860c18 "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: 4	scoped_refptr<content::SiteInstance>
▶	redirect_chain	{ size=0 }	std::__1::vector<GURL,std::__1::allocator<GURL>>
	uses_post	false	bool
▶	post_data	null	scoped_refptr<network::ResourceRequestBody>
▶	extra_headers	0x287a21b8 "Upgrade-Insecure-Requests: 1"	std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>>
	frame_tree_node_id	-1	int
	source_render_frame_id	3	int
	source_render_process_id	4	int
	disposition	NEW_BACKGROUND_TAB (4)	WindowOpenDisposition
	transition	PAGE_TRANSITION_FIRST (0)	ui::PageTransition
	is_renderer_initiated	true	bool
	should_replace_current_entry	false	bool
	user_gesture	true	bool
	triggering_event_info	kFromTrustedEvent (2)	blink::WebTriggeringEventInfo
	started_from_context_menu	false	bool
▶	blob_url_loader_factory	null	scoped_refptr<network::SharedURLLoaderFactory>
	open_app_window_if_possible	false	bool
▶	href_translate	0x00afd9cc ""	std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>>
	reload_type	NONE (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]

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]

Thanks for the suggestion :) The behavior change is due to ProcessSharingWithDefaultSiteInstances (added here and enabled by default here after M77 branched).

[Alex Moshchuk]

+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)?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/CAFW9UJ_wTMuJftrrxQwct%3DswHAKw9rrLBdM0VCG%3DtQ1HipDvaw%40mail.gmail.com.

[Aaron Colwell]

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]

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.

[1] https://bitbucket.org/chromiumembedded/cef/issues/2498/add-support-for-site-per-process

[Marshall Greenblatt]

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]

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++