Improve third-party NTPs creation performance by using spare renderer [chromium/src : main]
Wangsong Jin (Gerrit)
Wangsong Jin added 5 comments![]( "Open in Gerrit")
a new interface, SetConfigurationOnProcessLockUpdate(), in the existingWangsong Jinit is a new method instead of interface
Done
RendererConfiguration mojom. This method is called inWangsong Jin`RendererConfiguration mojom interface`?
Done
bool AreStaticParamsEqual(const chrome::mojom::StaticParamsPtr& a,
const chrome::mojom::StaticParamsPtr& b) {
if (!a || !b) {
return !a && !b;
}
return a->is_instant_process == b->is_instant_process;
}
Wangsong Jin`a.Equals(b)` should work, no need for this helper function.
Done
CHECK(!static_params_ || AreStaticParamsEqual(static_params_, params));Wangsong JinWould `static_params_ == params` not work?
Done
// Returns true if this renderer process is incognito.
bool IsIncognitoProcess();
// Sets whether this renderer process is an incognito process.
void SetIsIncognitoProcess(bool is_incognito_process);
// Returns true if this renderer process is an instant process.
bool IsInstantProcess();
// Sets whether this renderer process is an instant process.
void SetIsInstantProcess(bool is_instant_process);Wangsong JinDo we want to put these into some name space?
Yep, we should. IsInstantProcess() is widely used. Thanks!
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Wangsong JinIs there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Wangsong JinThat's the current behavior. If we do this for an instant process, it prevents 3rd party NTPs from displaying chrome-search:: urls in some frames the in ntp page. We have an existing test `ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe` to cover this. So I assume we still want to maintain this behavior?
Could you provide additional context if you believe we should apply this to all renders? I can create a separate bug based on that. Thx!
GetRendererInterface()->SetIsInstantProcess();Wangsong JinPer my other comment about layering, it seems that you might need to instead call out to the embedder (e.g., via ContentBrowserClient) to do whatever work is needed on a LockedStateUpdate, which could include sending over the instant process status (over some similar chrome-layer interface)
Liang ZhaoThank you for pointing out the layering issue. I wasn't aware of it before. This brings to mind the initial proposal: Move instant process flag into WebPreferences. Add a flag like bool enable_instant_api in WebPreferencse, and use that flag in ChromeContentRendererClient::RenderFrameCreated() to call RegisterURLSchemeAsDisplayIsolated()/SearchBox(render_frame). The flag is consumed only in chrome/ code. Here's the earlier revision https://chromium-review.googlesource.com/c/chromium/src/+/6373046/3
Do you think that's something we want to move forward with?
Wangsong JinMy thoughts.
There seems to be 3 aspects of this.
The first is what we label the thing so that it feels useful for other content embedders. With current code or put it into Web/RendererPreferences, it is still a flag that sounds chrome only, even if the code under /content does not access the flag.
Would use something like `mojo_base.mojom.DictionaryValue embedder_preferences` be more generic?The second is how/when to pass the flag and apply update according to the flag. As Web/RenderPreferences are passed down for every WebView creation and when some preference changes, if we use Web/RendererPreferences, we would end up passing the info multiple times and then have to ensure that it does not change when apply it. That feels not optimal to me.
As this is something that used to be command line switch, we don't expect it to change after the process is launched. So, calling it from NotifyRendererOfLockedStateUpdate() seems to be better to me. If we do it there, then it would be natural to add a new SetEmbedderPreference() method on Renderer mojo interface. We would want to call it when the process is locked or not though.The third is when to apply the preference. We could use existing places like ChromeContentRendererClient::WebViewCreated() to retrieve and apply the preference with a static flag to avoid applying it multiple times. Alternatively, we could add a new method like ContentRendererClient::UpdateEmbedderPreference() to ContentRendererClient and call it with GetContentClient()->renderer()->ApplyEmbedderPreference() from RenderThreadImpl::SetEmbedderPreference().
For checking, we could expose methods from ContentRendererClient to retrieve the preferences and switch the code to check that instead of command line switch. Those checking places are under /chrome, so no layer violation.
Wangsong JinThanks for you all inputs!
WebPreferences/RendererPreferences ... Since these are in Blink and also should be just about the web platform,
I retained the current approach NotifyRendererOfLockedStateUpdate() and refactored the code to isolate instant-process/chrome-search schema within the content layer.
Update the GetRendererInterface()->SetIsInstantProcess() to more general calls:
- GetContentClient()->browser()->GetClientPreferences(): We checked the instant flag and instant URL logic and encapsulate the instant flag into the dictionary.
- GetRendererInterface()->SetClientPreferences(): We store the ClientPreferences for the renderer, and call the ChromeContentRendererClient::SetClientPreferences() to set up RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme).
but we'd need to ask Blink owners if they would be ok with.
Should we go through a design review, or would it be sufficient to add them to review the CL
and I also share Liang's concern of this sending more updates than necessary... pass it in a separate IPC sent from NotifyRendererOfLockedStateUpdate() - maybe something like SetProcessLockedEmbedderState().
In RenderThreadImpl::SetClientPreferences, there're checks to ensure we don't
a. Update client preferences when the process is locked.
b. Apply the client preferences if there's no change.This ensures that RegisterURLSchemeAsDisplayIsolated() won't be called unnecessarily. I should do the same thing for blink_platform_impl_->SetClientPreferences(client_preferences); to avoid setting unnecessary values. Do you think that's acceptable?
I'm wondering if NotifyRendererOfLockedStateUpdate() could just have a ContentBrowserClient callout to the embedder, and then the chrome layer sends an IPC to its renderer counterpart through a different interface
It sounds for me what we refactored. Let me know if I don't get you.
would it be possible to rename the whole thing to not be "instant"? I don't think this feature has anything to do with instant processes anymore,
I updated the CL title and description. Are you suggesting that all the places (code) "instant" should be changed to something like "3rd party NTPs"?
Alex MoshchukAcknowledged
Wangsong Jin>> but we'd need to ask Blink owners if they would be ok with.
Should we go through a design review, or would it be sufficient to add them to review the CL
Let's just do it in this CL review; you'll need a blink owner anyway. @dch...@chromium.org, would you be able to help take a look at this, and whether there's a simpler way of doing things? tldr is that this is trying to replace a command-line switch for instant/third-party NTP processes with an IPC that is sent when we lock a process, with the goal of setting a renderer-global "I'm an instant process" bit. And the challenge being that we don't really want content/blink to know about instant.
>> I'm wondering if NotifyRendererOfLockedStateUpdate() could just have a ContentBrowserClient callout to the embedder, and then the chrome layer sends an IPC to its renderer counterpart through a different interface
It sounds for me what we refactored. Let me know if I don't get you.
Not quite. I meant instead of collecting prefs from the content embedder, sending them at the content layer (content/common/renderer.mojom), and then again calling out to the embedder on the renderer side to interpret them - find a way to send the actual IPC at the chrome layer (through some other process-wide interface). Daniel might be able to suggest something that might work better.
Daniel ChengGot it, thanks! Here's what I'm thinking: In the content embedder, we call into the chrome layer to set the client preferences directly:
- call the content layer to store the preference.
- call the content embedder to invoke RegisterURLSchemeAsDisplayIsolated(). Might need one more call from content embedder to the embedder on the renderer side to RegisterURL...
Would it be acceptable to expose GetRendererInterface() in chrome/browser? I haven’t seen us do that before, so I’m wondering if that’s why you mentioned going “through a different interface.
Or let me know if you have a better idea. Thx!
Wangsong JinI think we can simplify this quite a bit.
We can just reuse (or add a new) .mojom in `//chrome/common` to pass the bit down directly. [renderer_configuration.mojom](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/renderer_configuration.mojom;l=1?q=renderer_configuration.mojom&ss=chromium) would be promising... but it looks like it's called a bit too early. I really hate to add another Mojo interface "just" for this, that's probably the easiest. We could even just say that binding the Mojo interface itself is sufficient to activate instant, though it's probably clearer to have an explicit call for that.
Making that call would just set a global somewhere, and the other bits of the code that check instant preferences (or the command line today) would just check this flag.
Wangsong JinThank you for your suggestion! We have two options:
1. Add a new method into mojom::RendererConfiguration, something like SetConfigurationOnProcessLockUpdate, then we can set/get IsInstantProcess similarly to how it's done for chrome\renderer\process_state.cc.
2. Add a new mojo interface for setting renderer client preferences:
````
struct ClientPreferences {
bool is_instant_process;
};
interface RendererClientPreferences {
SetClientPreferences(ClientPreferences client_preferences);
};
```Which option do you prefer?
Wangsong JinI refactored using option 1, which allows for the reuse of the existing mojom. The `SetConfiguration(DynamicParams params)` method is used for renderer configurations with settings that can change. It seems reasonable for me to introduce a separate interface for settings like is_instant_process, which should remain unchanged once set. Therefore, I added SetConfigurationOnProcessLockUpdate(StaticParams params).
PTAL when you have the chance, thanks a lot!
Hi @dch...@chromium.org, when you get the chance, could you help to look at this? We need your input on this refactor. Thank you!
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
ChromeContentBrowserClient::GetRendererConfiguration(This helper function doesn't seem to access any member of ChromeContentBrowserClient. Should we make it a helper function in anonymous namespace in this file instead of a member function of the class?
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
This helper function doesn't seem to access any member of ChromeContentBrowserClient. Should we make it a helper function in anonymous namespace in this file instead of a member function of the class?
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Wangsong JinIs there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Wangsong JinThat's the current behavior. If we do this for an instant process, it prevents 3rd party NTPs from displaying chrome-search:: urls in some frames the in ntp page. We have an existing test `ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe` to cover this. So I assume we still want to maintain this behavior?
Could you provide additional context if you believe we should apply this to all renders? I can create a separate bug based on that. Thx!
I guess I'm a bit concerned about this, because we have to weaken the DCHECK somewhat significantly. In particular, just because a process isn't locked to a site doesn't mean it can't have loaded pages.
What exactly is the chrome-search scheme for? Why is it important for its security properties to differ in an instant process? That is honestly a bit odd–definitely unfortunate that we're running into it now, but if we want to move away from the command-line flag, this is an issue we're going to have to solve.
GetRendererInterface()->SetIsInstantProcess();- Alex Moshchuk
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Wangsong JinIs there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Wangsong JinThat's the current behavior. If we do this for an instant process, it prevents 3rd party NTPs from displaying chrome-search:: urls in some frames the in ntp page. We have an existing test `ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe` to cover this. So I assume we still want to maintain this behavior?
Daniel ChengCould you provide additional context if you believe we should apply this to all renders? I can create a separate bug based on that. Thx!
I guess I'm a bit concerned about this, because we have to weaken the DCHECK somewhat significantly. In particular, just because a process isn't locked to a site doesn't mean it can't have loaded pages.
What exactly is the chrome-search scheme for? Why is it important for its security properties to differ in an instant process? That is honestly a bit odd–definitely unfortunate that we're running into it now, but if we want to move away from the command-line flag, this is an issue we're going to have to solve.
Thanks!
chrome-search is designed for 3rd party default search provider to be hosted and have access to some native APIs that other web pages do not have access to.
Why is it important for its security properties to differ in an instant process?
This is the existing behavior for all third party NTPs. I'm not entirely sure about the impact. We'd better to have separate task to clear if we want to this for all renderers.
Just because a process isn't locked to a site doesn't mean it can't have loaded pages.
Do you have any thoughts on the where to configure the settings?
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Wangsong JinIs there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Wangsong JinThat's the current behavior. If we do this for an instant process, it prevents 3rd party NTPs from displaying chrome-search:: urls in some frames the in ntp page. We have an existing test `ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe` to cover this. So I assume we still want to maintain this behavior?
Daniel ChengCould you provide additional context if you believe we should apply this to all renders? I can create a separate bug based on that. Thx!
Wangsong JinI guess I'm a bit concerned about this, because we have to weaken the DCHECK somewhat significantly. In particular, just because a process isn't locked to a site doesn't mean it can't have loaded pages.
What exactly is the chrome-search scheme for? Why is it important for its security properties to differ in an instant process? That is honestly a bit odd–definitely unfortunate that we're running into it now, but if we want to move away from the command-line flag, this is an issue we're going to have to solve.
Thanks!
chrome-search is designed for 3rd party default search provider to be hosted and have access to some native APIs that other web pages do not have access to.
Why is it important for its security properties to differ in an instant process?
This is the existing behavior for all third party NTPs. I'm not entirely sure about the impact. We'd better to have separate task to clear if we want to this for all renderers.
Just because a process isn't locked to a site doesn't mean it can't have loaded pages.Do you have any thoughts on the where to configure the settings?
I think there are probably two paths here:
1. Don't add chrome-search as a display-isolated scheme at all. That would require understanding why we did this historically.
2. Figure out what's triggering thread creation... and don't do it. I don't honestly know if that's possible though.
The DCHECK exists to prevent race conditions, and we're relaxing the DCHECK to potentially allow those race conditions. I think (1) might be easier, but that requires some code archaeology to understand the historical context.
Related details
- Alex Moshchuk
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Wangsong JinIs there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Wangsong JinThat's the current behavior. If we do this for an instant process, it prevents 3rd party NTPs from displaying chrome-search:: urls in some frames the in ntp page. We have an existing test `ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe` to cover this. So I assume we still want to maintain this behavior?
Daniel ChengCould you provide additional context if you believe we should apply this to all renders? I can create a separate bug based on that. Thx!
Wangsong JinI guess I'm a bit concerned about this, because we have to weaken the DCHECK somewhat significantly. In particular, just because a process isn't locked to a site doesn't mean it can't have loaded pages.
What exactly is the chrome-search scheme for? Why is it important for its security properties to differ in an instant process? That is honestly a bit odd–definitely unfortunate that we're running into it now, but if we want to move away from the command-line flag, this is an issue we're going to have to solve.
Daniel ChengThanks!
chrome-search is designed for 3rd party default search provider to be hosted and have access to some native APIs that other web pages do not have access to.
Why is it important for its security properties to differ in an instant process?
This is the existing behavior for all third party NTPs. I'm not entirely sure about the impact. We'd better to have separate task to clear if we want to this for all renderers.
Just because a process isn't locked to a site doesn't mean it can't have loaded pages.Do you have any thoughts on the where to configure the settings?
I think there are probably two paths here:
1. Don't add chrome-search as a display-isolated scheme at all. That would require understanding why we did this historically.
2. Figure out what's triggering thread creation... and don't do it. I don't honestly know if that's possible though.The DCHECK exists to prevent race conditions, and we're relaxing the DCHECK to potentially allow those race conditions. I think (1) might be easier, but that requires some code archaeology to understand the historical context.
I found some historical context for chrome-search and display-isolated scheme in https://issues.chromium.org/issues/40309067 and https://chromiumcodereview.appspot.com/12621008/.
IIUC, all this is due to the fact that a third-party NTP needs to be able to show most visited tiles hosted at chrome-search://. E.g., if I set Bing as my default search engine, the NTP is loaded from https://bing.com/chrome/newtab in a process locked to chrome-search://remote-ntp; the origin seen in Blink is https://bing.com. That document needs to be able to display most-visited tiles, which are embedded via iframes to chrome-search://most-visited/title.html?rid=2. Other sites in non-NTP processes shouldn't be able to embed them.
Prior to https://chromiumcodereview.appspot.com/12621008, chrome-search was actually registered as a display-isolated scheme unconditionally, and then special exceptions were made for allowing tile URLs to be iframed via WebSecurityPolicy::AddOriginAccessAllowListEntry(). But https://issues.chromium.org/issues/40309067 showed that this was problematic because it allowed bing.com to XHR and read the tile data directly, which was undesirable. So instead, the origin access exceptions were removed, and chrome-search was made non-display-isolated in the instant process, which allowed the instant process to embed and display the most visited tiles, but not read them.
This was a long time ago (e.g., WebSecurityPolicy evolved a lot, we've got site isolation and CORB, etc). I also think that we have some NTP tile exceptions configured in the browser process, and that even if we completely removed the display-isolated scheme thing in the renderer process, the browser process still wouldn't allow a web process other than 3P NTP to embed chrome-search: iframes (though you might get a renderer kill instead of a console error) - maybe this is worth verifying.
A couple of other random thoughts here:
- could there be a better way with WebSecurityPolicy (which evolved a lot) to configure most visited tile exception for 3P NTP, without running into the XHR bug?
- I didn't look much at the race question, but presumably the process should be locked before any pages get to execute any code in that process, so at that time it should still be theoretically ok to be configuring the scheme registry?
- Does it help if we start with chrome-search: being display-isolated unconditionally on renderer process startup, and then we relax that condition for instant processes when they're locked (still before they get to run any code)?
Related details
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Thanks for you all the valuable insights!
We also found that DuckDuckGo embeds a chrome-search iframe in their NTP page in real-world scenarios. Therefore, we cannot block the test ThirdPartyNTPBrowserTest.EmbeddedMostVisitedIframe.
Attempted to remove the registration of the chrome-search schema for all renderers. However, this does not prevent non-instant processes from embedding the chrome-search schema. Therefore, we still need to use RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme).
if we start with chrome-search: being display-isolated unconditionally on renderer process startup, and then we relax that condition for instant processes when they're locked.
Updated with this approach. It turns out we don't need to modify the DCHECK(). This also resolves the 'Relaxing the DCHECK()' issue.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 3 comments![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kInstantUsesSpareRenderer) &&Since we now only call SetConfigurationOnProcessLockUpdate when is_instant_process is true and we rely on SetConfigurationOnProcessLockUpdate to make process_state::IsInstantProcess() true, we should ensure that params->is_instant_process is true whether the feature is enabled or not.
bool is_instant_process;should we explicitly set `= false`?
// The Instant process can only display the content but not read it. Other
// processes can't display it or read it.Should we update the comments? With the new code/feature, we treat the process as non instant process at the process start time and update the registration later if it is decided that it is an instant process.
Do we even need to check for command line switch?
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Attempted to remove the registration of the chrome-search schema for all renderers. However, this does not prevent non-instant processes from embedding the chrome-search schema. Therefore, we still need to use RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme).
Why is it a problem for non-instance processes to embed it? It shouldn't do anything in a non-instant process, since it wouldn't have the corresponding URLLoaderFactory for the scheme, right?
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
That seems to be the intent, but the current code does not really achieve it.
In [InstantService::ShouldServiceRequest](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/search/instant_service.cc;l=296?q=InstantService::ShouldServiceRequest&ss=chromium%2Fchromium%2Fsrc#:~:text=bool%20InstantService%3A%3AShouldServiceRequest,%7D), we return true if `render_process_id == -1` and the comments states: "The process_id for the navigation request will be -1. If so, allow this request since it's not going to another renderer.".
And WebUIURLLoaderFactory [always uses -1](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_url_loader_factory.cc;l=357?q=CreateWebUIURLLoaderFactory&ss=chromium%2Fchromium%2Fsrc#:~:text=if%20(!source%2D%3E,%2D1) when checking ShouldServiceRequest.
And because chrome-search is supported WebUI scheme, [CreateWebUIURLLoaderFactory is always called](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/loader/navigation_url_loader_impl.cc#:~:text=CreateWebUIURLLoaderFactory(,mojom%3A%3AkBrowserProcessId)%2C) in `NavigationURLLoaderImpl::Start` when trying to add an chrome-search: iframe.
If we want to really block it in browser code, we might want to do it in a separate CL.
Anyway, not removing renderer process restriction is better.
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kInstantUsesSpareRenderer) &&Since we now only call SetConfigurationOnProcessLockUpdate when is_instant_process is true and we rely on SetConfigurationOnProcessLockUpdate to make process_state::IsInstantProcess() true, we should ensure that params->is_instant_process is true whether the feature is enabled or not.
Thanks!
In process_state::IsInstantProcess() we still check the command line switch. Thus we don't need to set params->is_instant_process to true if it's from command line.
But yes, it's better to set this params->is_instant_process to true for all instant process cases, it can simplify the logic.
// The Instant process can only display the content but not read it. Other
// processes can't display it or read it.Should we update the comments? With the new code/feature, we treat the process as non instant process at the process start time and update the registration later if it is decided that it is an instant process.
Do we even need to check for command line switch?
Updated. Previous we need it as we only call SetConfigurationOnProcessLockUpdate() with the feature enabled case. As we update to set params->is_instant_process for both command switches and feature flag, we can remove it now.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
should we explicitly set `= false`?
Done
// The Instant process can only display the content but not read it. Other
// processes can't display it or read it.Wangsong JinShould we update the comments? With the new code/feature, we treat the process as non instant process at the process start time and update the registration later if it is decided that it is an instant process.
Do we even need to check for command line switch?
Updated. Previous we need it as we only call SetConfigurationOnProcessLockUpdate() with the feature enabled case. As we update to set params->is_instant_process for both command switches and feature flag, we can remove it now.
Ah. I missed that IsInstantProcess() also checks for command line switch. Both the current change and previous version work. While the current code is simpler, previous version (patchset 36) code should be safer against possible regression when the new feature is disabled. To de-risk while we experiment with the new feature, I feel that it might be better to keep the previous version of the code.
Don't know what Daniel and Alex think.
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kInstantUsesSpareRenderer) &&If the feature is not enabled, we would never call SetConfigurationOnProcessLockUpdate. Should we move the feature check to the top of the function and return early if the feature is not enabled?
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(features::kInstantUsesSpareRenderer) &&If the feature is not enabled, we would never call SetConfigurationOnProcessLockUpdate. Should we move the feature check to the top of the function and return early if the feature is not enabled?
Yes, it's better to return earlier. I've updated the function accordingly.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
The CSA team chatted about this today. I would really like to avoid adding more ways to modify the URL scheme registry, even if they don't happen to hit the DCHECKs anymore. While there are other `Remove*` functions, they appear to be primarily (only?) used in tests, and should probably be marked as such. In general, we should treat the scheme registry as one-off registration, and not something we can go back and patch after the fact.
We also think it's fragile to lean so heavily on the renderer to enforce that chrome-display: is only loaded "where it should be". Enforcing this should be two parts on the browser side:
1. Make sure we don't provide a URLLoaderFactory for chrome-search subresources (and make sure the fallback URL loader factory doesn't inadvertently end up serving chrome-search).
2. Somehow fixing navigations to block navigation requests unless this an NTP process is requesting to load chrome-search:// as a subframe. (This means that attempting to commit chrome-search:// as a main frame **also** should not work). I'm not quite sure what's involved; as you noted, it looks like passing -1 here: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_url_loader_factory.cc;l=141;drc=743a82d08e59d803c94ee1b8564b8b11dd7b462f;bpv=0;bpt=1 is allowing checks to pass when it should be denied, i.e. I think this comment:
```
// We pass the FrameTreeNode ID to get to the WebContents because requests
// from frames can happen while the RFH is changed for a cross-process
// navigation. The URLDataSources just need the WebContents; the specific
// frame doesn't matter.
```
is not quite correct–the specific frame making the request does matter.
How does this work for "normal" WebUI schemes? I'd expect it to get blocked by ChildProcessSecurityPolicy or the like... in our discussion, we had mentioned using a navigation throttle, but I think this was actually removed in https://chromium-review.googlesource.com/c/chromium/src/+/2007813.
Related details
- Alex Moshchuk
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Looks like normal WebUI schemes are called with RegisterURLSchemeAsDisplayIsolated() in renderer process and not called with RegisterWebSafeScheme() in browser process.
If we use the renderer process id associated with the frame_tree_node_id, ShouldServiceRequest will work as expected and block the navigation.
However, we might not want to block main frame navigation though. They are currently allowed and third party NTP is using chrome-search://local-ntp and remote-ntp.
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Thanks!
Removed all the chrome-search as a display-isolated scheme.
Fixed the logic to return the actual render_process_id instead of -1 in `WebUIURLLoaderFactory`. And among all ShouldServiceRequest() implementations, only InstantService::ShouldServiceRequest() uses render_process_id, so this change looks safe. Also added a test: `ThirdPartyNTPBrowserTest.NonInstantProcessCannotEmbedChromeSearch` to ensure that non-Instant processes cannot embed a chrome-search:// iframe.
This means that attempting to commit chrome-search:// as a main frame also should not work
Currently on Edge, it's possible to navigate from a normal process to a chrome-search:// URL in the main frame. So we'd like to keep main frame behavior unchanged in this update.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
GURL non_instant_url = https_test_server().GetURL("accounts.google.com", "/");Can we use same url as in EmbeddedMostVisitedIframe test, or example.com or a.com if we want to avoid using ntp.com to avoid potential confusion?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);We should also mention this change and the fix in WebUIURLLoaderFactory in CL description, and update comments in [chrome/common/url_constants.h above kChromeSearchScheme](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/url_constants.h;l=192?q=kChromeSearchScheme&sq=&ss=chromium%2Fchromium%2Fsrc#:~:text=//%20%201.%20Renderer%3A%20The%20display%2Disolated%20check%20in%20WebKit%20will%20deny%20the%20request%2C)
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
GURL non_instant_url = https_test_server().GetURL("accounts.google.com", "/");Can we use same url as in EmbeddedMostVisitedIframe test, or example.com or a.com if we want to avoid using ntp.com to avoid potential confusion?
Done
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);We should also mention this change and the fix in WebUIURLLoaderFactory in CL description, and update comments in [chrome/common/url_constants.h above kChromeSearchScheme](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/url_constants.h;l=192?q=kChromeSearchScheme&sq=&ss=chromium%2Fchromium%2Fsrc#:~:text=//%20%201.%20Renderer%3A%20The%20display%2Disolated%20check%20in%20WebKit%20will%20deny%20the%20request%2C)
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
if (!request.is_outermost_main_frame) {Is there a chance this will be racy? Is it possible for one RFH to start the navigation, and then get it swapped out in a way that would cause subsequent checks to pass when they should have failed?
I think this would be a pretty rare edge case (since non-WebUI shouldn't be able to navigate to WebUI... but I don't know if there are weird edge cases here, e.g. if we use chrome-untrusted: to implement something), and maybe the BrowsingInstance swap we force on transitions between WebUI and non-WebUI pages prevents this... but it'd probably be good to have a comment here that describes why this isn't problematic if so.
Related details
- Alex Moshchuk
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (!request.is_outermost_main_frame) {Is there a chance this will be racy? Is it possible for one RFH to start the navigation, and then get it swapped out in a way that would cause subsequent checks to pass when they should have failed?
I think this would be a pretty rare edge case (since non-WebUI shouldn't be able to navigate to WebUI... but I don't know if there are weird edge cases here, e.g. if we use chrome-untrusted: to implement something), and maybe the BrowsingInstance swap we force on transitions between WebUI and non-WebUI pages prevents this... but it'd probably be good to have a comment here that describes why this isn't problematic if so.
The process id is only used for checking whether it is in instant process. For the purpose of that check, the potential race condition risk should not matter. There should not be a case where navigation in non instant process get assigned to an instant process.
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
Are these changes still needed?
Ah, deleted that one.
I will reset this blank space in next patchset
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
if (!request.is_outermost_main_frame) {Liang ZhaoIs there a chance this will be racy? Is it possible for one RFH to start the navigation, and then get it swapped out in a way that would cause subsequent checks to pass when they should have failed?
I think this would be a pretty rare edge case (since non-WebUI shouldn't be able to navigate to WebUI... but I don't know if there are weird edge cases here, e.g. if we use chrome-untrusted: to implement something), and maybe the BrowsingInstance swap we force on transitions between WebUI and non-WebUI pages prevents this... but it'd probably be good to have a comment here that describes why this isn't problematic if so.
The process id is only used for checking whether it is in instant process. For the purpose of that check, the potential race condition risk should not matter. There should not be a case where navigation in non instant process get assigned to an instant process.
Would it better if we capture the render process id when WebUIURLLoaderFactory is created? It should address the race condition concern and also avoid calculating the render process id for every sub resource loading.
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
if (!request.is_outermost_main_frame) {Liang ZhaoIs there a chance this will be racy? Is it possible for one RFH to start the navigation, and then get it swapped out in a way that would cause subsequent checks to pass when they should have failed?
I think this would be a pretty rare edge case (since non-WebUI shouldn't be able to navigate to WebUI... but I don't know if there are weird edge cases here, e.g. if we use chrome-untrusted: to implement something), and maybe the BrowsingInstance swap we force on transitions between WebUI and non-WebUI pages prevents this... but it'd probably be good to have a comment here that describes why this isn't problematic if so.
Liang ZhaoThe process id is only used for checking whether it is in instant process. For the purpose of that check, the potential race condition risk should not matter. There should not be a case where navigation in non instant process get assigned to an instant process.
Would it better if we capture the render process id when WebUIURLLoaderFactory is created? It should address the race condition concern and also avoid calculating the render process id for every sub resource loading.
Thanks, refined.
I will reset this blank space in next patchset
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
// instant process. If the request is from the outer most main frame, we
// set the render process id to -1 to preserve the existing behavior,We might be able to move this logic also to object creation time. FWIW, we are also using -1 for service worker.
ftn->current_frame_host()->GetBrowserContext(),This code implies that ftn->current_frame_host() should not be null and therefore we don't have to check ftn and rfh not null.
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
// instant process. If the request is from the outer most main frame, we
// set the render process id to -1 to preserve the existing behavior,We might be able to move this logic also to object creation time. FWIW, we are also using -1 for service worker.
Updated!
This code implies that ftn->current_frame_host() should not be null and therefore we don't have to check ftn and rfh not null.
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 6 comments![]( "Open in Gerrit")
I didn't review the tests nor the new `render_process_id` in the web UI url loader factory in a lot of detail, but they look approximately correct to me.
return mojo::AssociatedRemote<chrome::mojom::RendererConfiguration>();You could `return mojo::NullAssociatedRemote()` here if you wanted.
That being said, is there any reasonable world where we'd actually have no channel here? It seems undesirable to silently fail in this case, because then the first-time init would just not happen...
renderer_configuration->SetConfigurationOnProcessLockUpdate(i.e. we should be able to assume we have a ChannelProxy here probably?
https_test_server().GetURL("ntp.com", "/instant_extended.html");Nit: I think it would be safe to use something like ntp.example.com
There's no real guarantee where ntp.com will point.
// once and do not change.When would we call this again?
process_state::SetIsInstantProcess(params ? params->is_instant_processWe don't need to check that this is not null; it's not marked nullable in method declaration in the mojom, so Mojo will enforce this is always set.
Related details
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 5 comments![]( "Open in Gerrit")
Hi, I really appreciate all the valuable feedback on this change.
I think now is a fair time to bring in additional reviews. Since this CL involves considerable amount of contexts, could you help loop in any key OWNERS you think would be a good fit to review? I’ll make sure to add the remaining reviewers as needed. cc: alexmos@, dcheng@ Thanks a lot!
return mojo::AssociatedRemote<chrome::mojom::RendererConfiguration>();You could `return mojo::NullAssociatedRemote()` here if you wanted.
That being said, is there any reasonable world where we'd actually have no channel here? It seems undesirable to silently fail in this case, because then the first-time init would just not happen...
I followed existing pattern to setup [GetRendererConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/renderer_updater.cc;l=174?q=GetRendererConfiguration()&ss=chromium%2Fchromium%2Fsrc:chrome%2F). If the renderer process died unexpectedly, the channel would be null and we don't want to crash browser process if that happened. So I don't add CHECK for renderer_configuration.
https_test_server().GetURL("ntp.com", "/instant_extended.html");Nit: I think it would be safe to use something like ntp.example.com
There's no real guarantee where ntp.com will point.
Done
// once and do not change.When would we call this again?
Both RenderProcessHostImpl::OnProcessLaunched() and RenderProcessHostImpl::SetProcessLock() call NotifyRendererOfLockedStateUpdate(), which in turn triggers OnRendererProcessLockedStateUpdated() → SetConfigurationOnProcessLockUpdate(). As a result, if params->is_instant_process == true, SetConfigurationOnProcessLockUpdate() may be invoked twice
More specifically:
1. Instant process with existing command-line switch:
SetConfigurationOnProcessLockUpdate() is never called. The configuration setting is skipped, and the command-line switch is used directly in IsInstantProcess() check.
2. Instant process set with process_state and a spare renderer available:
SetConfigurationOnProcessLockUpdate() is called only once for third party NTPs. This is because OnProcessLaunched() is triggered during spare renderer creation, but is_instant_process is false at that point, so the update is skipped.
3. Instant process set with process_state and no spare renderer available:
A new process is created, and SetConfigurationOnProcessLockUpdate() is called twice for third party NTPs.
This guard is just to prevent potential issue from future changes. Personally, I think we'd better add some guards for other fields that are also meant to be set only once in NotifyRendererOfLockedStateUpdate() to prevent from unintentionally modifying renderer settings.
process_state::SetIsInstantProcess(params ? params->is_instant_processWe don't need to check that this is not null; it's not marked nullable in method declaration in the mojom, so Mojo will enforce this is always set.
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
https_test_server().GetURL("ntp.com", "/instant_extended.html");Wangsong JinNit: I think it would be safe to use something like ntp.example.com
There's no real guarantee where ntp.com will point.
Done
There actually is no need to change to ntp.example.com. This test does ` host_resolver()->AddRule("*", "127.0.0.1");` in its SetUpOnMainThread() to ensure that all hosts are resolved to the test server.
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
return mojo::AssociatedRemote<chrome::mojom::RendererConfiguration>();Wangsong JinYou could `return mojo::NullAssociatedRemote()` here if you wanted.
That being said, is there any reasonable world where we'd actually have no channel here? It seems undesirable to silently fail in this case, because then the first-time init would just not happen...
I followed existing pattern to setup [GetRendererConfiguration](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/renderer_updater.cc;l=174?q=GetRendererConfiguration()&ss=chromium%2Fchromium%2Fsrc:chrome%2F). If the renderer process died unexpectedly, the channel would be null and we don't want to crash browser process if that happened. So I don't add CHECK for renderer_configuration.
Acknowledged
https_test_server().GetURL("ntp.com", "/instant_extended.html");Wangsong JinNit: I think it would be safe to use something like ntp.example.com
There's no real guarantee where ntp.com will point.
Liang ZhaoDone
There actually is no need to change to ntp.example.com. This test does ` host_resolver()->AddRule("*", "127.0.0.1");` in its SetUpOnMainThread() to ensure that all hosts are resolved to the test server.
I understand that, but I don't really want random hostnames that aren't controlled in our code. ntp.com doesn't really point to anything right now, but it could point to something unfortunate in the future.
// once and do not change.Wangsong JinWhen would we call this again?
Both RenderProcessHostImpl::OnProcessLaunched() and RenderProcessHostImpl::SetProcessLock() call NotifyRendererOfLockedStateUpdate(), which in turn triggers OnRendererProcessLockedStateUpdated() → SetConfigurationOnProcessLockUpdate(). As a result, if params->is_instant_process == true, SetConfigurationOnProcessLockUpdate() may be invoked twice
More specifically:
1. Instant process with existing command-line switch:
SetConfigurationOnProcessLockUpdate() is never called. The configuration setting is skipped, and the command-line switch is used directly in IsInstantProcess() check.2. Instant process set with process_state and a spare renderer available:
SetConfigurationOnProcessLockUpdate() is called only once for third party NTPs. This is because OnProcessLaunched() is triggered during spare renderer creation, but is_instant_process is false at that point, so the update is skipped.3. Instant process set with process_state and no spare renderer available:
A new process is created, and SetConfigurationOnProcessLockUpdate() is called twice for third party NTPs.This guard is just to prevent potential issue from future changes. Personally, I think we'd better add some guards for other fields that are also meant to be set only once in NotifyRendererOfLockedStateUpdate() to prevent from unintentionally modifying renderer settings.
It feels kind of messy that the static configuration would be sent more than once :)
Can the browser check if the static config has changed and skip sending the update the second time?
(And if it has changed, that seems like a browser bug...)
Related details
- Alex Moshchuk
- Liang Zhao
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
https_test_server().GetURL("ntp.com", "/instant_extended.html");Wangsong JinNit: I think it would be safe to use something like ntp.example.com
There's no real guarantee where ntp.com will point.
Liang ZhaoDone
There actually is no need to change to ntp.example.com. This test does ` host_resolver()->AddRule("*", "127.0.0.1");` in its SetUpOnMainThread() to ensure that all hosts are resolved to the test server.
Updated all domain to ntp.example.com
// once and do not change.Wangsong JinWhen would we call this again?
Daniel ChengBoth RenderProcessHostImpl::OnProcessLaunched() and RenderProcessHostImpl::SetProcessLock() call NotifyRendererOfLockedStateUpdate(), which in turn triggers OnRendererProcessLockedStateUpdated() → SetConfigurationOnProcessLockUpdate(). As a result, if params->is_instant_process == true, SetConfigurationOnProcessLockUpdate() may be invoked twice
More specifically:
1. Instant process with existing command-line switch:
SetConfigurationOnProcessLockUpdate() is never called. The configuration setting is skipped, and the command-line switch is used directly in IsInstantProcess() check.2. Instant process set with process_state and a spare renderer available:
SetConfigurationOnProcessLockUpdate() is called only once for third party NTPs. This is because OnProcessLaunched() is triggered during spare renderer creation, but is_instant_process is false at that point, so the update is skipped.3. Instant process set with process_state and no spare renderer available:
A new process is created, and SetConfigurationOnProcessLockUpdate() is called twice for third party NTPs.This guard is just to prevent potential issue from future changes. Personally, I think we'd better add some guards for other fields that are also meant to be set only once in NotifyRendererOfLockedStateUpdate() to prevent from unintentionally modifying renderer settings.
It feels kind of messy that the static configuration would be sent more than once :)
Can the browser check if the static config has changed and skip sending the update the second time?
(And if it has changed, that seems like a browser bug...)
Thanks!
Added GetUserData(kRendererConfigurationStaticParamsKey) to save the stage per render process host. This avoids calling the SetConfigurationOnProcessLockUpdate() mojo method a second time. Also added a CHECK on browser side to ensure static params won't change.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 3 comments![]( "Open in Gerrit")
host->GetUserData(kRendererConfigurationStaticParamsKey));Could we avoid calling GetUserData() twice?
CHECK(stored_params->Equals(params->Clone()));Do we have to call Clone() here and when calling SetConfigurationOnProcessLockUpdate?
if (!static_params_) {With the CHECK above, don't need this check anymore
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
CHECK(stored_params->Equals(params->Clone()));Do we have to call Clone() here and when calling SetConfigurationOnProcessLockUpdate?
Done
// once and do not change.Wangsong JinWhen would we call this again?
Daniel ChengBoth RenderProcessHostImpl::OnProcessLaunched() and RenderProcessHostImpl::SetProcessLock() call NotifyRendererOfLockedStateUpdate(), which in turn triggers OnRendererProcessLockedStateUpdated() → SetConfigurationOnProcessLockUpdate(). As a result, if params->is_instant_process == true, SetConfigurationOnProcessLockUpdate() may be invoked twice
More specifically:
1. Instant process with existing command-line switch:
SetConfigurationOnProcessLockUpdate() is never called. The configuration setting is skipped, and the command-line switch is used directly in IsInstantProcess() check.2. Instant process set with process_state and a spare renderer available:
SetConfigurationOnProcessLockUpdate() is called only once for third party NTPs. This is because OnProcessLaunched() is triggered during spare renderer creation, but is_instant_process is false at that point, so the update is skipped.3. Instant process set with process_state and no spare renderer available:
A new process is created, and SetConfigurationOnProcessLockUpdate() is called twice for third party NTPs.This guard is just to prevent potential issue from future changes. Personally, I think we'd better add some guards for other fields that are also meant to be set only once in NotifyRendererOfLockedStateUpdate() to prevent from unintentionally modifying renderer settings.
Wangsong JinIt feels kind of messy that the static configuration would be sent more than once :)
Can the browser check if the static config has changed and skip sending the update the second time?
(And if it has changed, that seems like a browser bug...)
Thanks!
Added GetUserData(kRendererConfigurationStaticParamsKey) to save the stage per render process host. This avoids calling the SetConfigurationOnProcessLockUpdate() mojo method a second time. Also added a CHECK on browser side to ensure static params won't change.
Well, this change could affect a case where an instant renderer process crashes. When the crashed page is reloaded, the same RenderProcessHost is reused, which preserves its UserData state. As a result, SetConfigurationOnProcessLockUpdate() is skipped for the new renderer process.
if (!static_params_) {With the CHECK above, don't need this check anymore
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
host->GetUserData(kRendererConfigurationStaticParamsKey));Could we avoid calling GetUserData() twice?
Done
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
// once and do not change.Wangsong JinWhen would we call this again?
Daniel ChengBoth RenderProcessHostImpl::OnProcessLaunched() and RenderProcessHostImpl::SetProcessLock() call NotifyRendererOfLockedStateUpdate(), which in turn triggers OnRendererProcessLockedStateUpdated() → SetConfigurationOnProcessLockUpdate(). As a result, if params->is_instant_process == true, SetConfigurationOnProcessLockUpdate() may be invoked twice
More specifically:
1. Instant process with existing command-line switch:
SetConfigurationOnProcessLockUpdate() is never called. The configuration setting is skipped, and the command-line switch is used directly in IsInstantProcess() check.2. Instant process set with process_state and a spare renderer available:
SetConfigurationOnProcessLockUpdate() is called only once for third party NTPs. This is because OnProcessLaunched() is triggered during spare renderer creation, but is_instant_process is false at that point, so the update is skipped.3. Instant process set with process_state and no spare renderer available:
A new process is created, and SetConfigurationOnProcessLockUpdate() is called twice for third party NTPs.This guard is just to prevent potential issue from future changes. Personally, I think we'd better add some guards for other fields that are also meant to be set only once in NotifyRendererOfLockedStateUpdate() to prevent from unintentionally modifying renderer settings.
Wangsong JinIt feels kind of messy that the static configuration would be sent more than once :)
Can the browser check if the static config has changed and skip sending the update the second time?
(And if it has changed, that seems like a browser bug...)
Wangsong JinThanks!
Added GetUserData(kRendererConfigurationStaticParamsKey) to save the stage per render process host. This avoids calling the SetConfigurationOnProcessLockUpdate() mojo method a second time. Also added a CHECK on browser side to ensure static params won't change.
Well, this change could affect a case where an instant renderer process crashes. When the crashed page is reloaded, the same RenderProcessHost is reused, which preserves its UserData state. As a result, SetConfigurationOnProcessLockUpdate() is skipped for the new renderer process.
That might explain why other renderer settings in NotifyRendererOfLockedStateUpdate() aren’t applied just once. Do you have any additional thoughts? Otherwise, I'll revert the change.
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Updated the code to bypass process spawn after crashing case by checking if host->GetLastInitTime() has changed. Does that approach looks good for you?
Other options:
1. We could track the render process creation and deletion directly, though that might be a bit complex.
2. Do not care the second call.
3. Find a way to skip the second call for "Instant process set with process_state and no spare renderer available".
Let me know which option you prefer—or if you have another idea in mind. Thanks!
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Missing required owners added.
Hi all, please take a look when you get a chance, thanks a lot!
Related details
- Adam Rice
- Alex Moshchuk
- Daniel Cheng
- David Trainor
- Frank Liberato
- Liang Zhao
- Paul Adedeji
- Reilly Grant
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
- Adam Rice
- Alex Moshchuk
- Daniel Cheng
- David Trainor
- Frank Liberato
- Liang Zhao
- Paul Adedeji
- Wangsong Jin
Alex Moshchuk (Gerrit)
Alex Moshchuk added 8 comments![]( "Open in Gerrit")
Thanks, caught up on the latest changes - the mojo plumbing looks much better to me. A couple of questions below on the outstanding threads.
// If the input |url| should be assigned to the Instant renderer.site_url?
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));The origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Is there any reason we can't just unconditionally do this always? I thought there was already an extension point in ContentClient or ContentBrowserClient for configuring schemes, and it doesn't seem like it'd hurt to do this in all renderers.
Hmm, if I try to do something like window.location="chrome-search://most-visited/title.html?rid=4&fs=0", Chrome disallows that in normal web pages with a "Not allowed to load local resource" error. Doing this in the instant process (i.e., from the remote NTP) works, though. Would the -1 process_id fix mean that we regress this behavior and allow web pages to potentially navigate to instant processes in main frames? I'm not sure that's desirable.
Hmm, it's surprising to me that NotifyRendererOfLockedStateUpdate() would be called twice in the case you described, as the lock shouldn't really change once it's set. The original intention of calling it from OnProcessLaunched() was to ensure that it's done after crash recovery - see https://chromium-review.googlesource.com/c/chromium/src/+/1083494. If it's called twice with the same info, it might be a bug that we might want to fix separately at the content layer.
For the "Instant process set with process_state and no spare renderer available" case - Is the NotifyRendererOfLockedStateUpdate() happening twice for normal non-NTP URLs that don't use the spare too? Are both of those NotifyRendererOfLockedStateUpdate() calls done with the same non-empty chrome-search:// site_url, or is one of them with an empty site_url? Asking because I expect the only case where we might lock a process twice to be when the first lock is an "allow_any_site" lock (basically meaning the process is not really locked) that's created for about:blank (for example), which can later be upgraded to a site-specific lock (see [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/child_process_security_policy_impl.cc;l=658-662;drc=8e9c2dff53856916bbbc066f04b7ab0495d57a2f) and [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/process_lock.h;l=29-38;drc=8e9c2dff53856916bbbc066f04b7ab0495d57a2f)). NotifyRendererOfLockedStateUpdate() is also [skipped](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_process_host_impl.cc;l=3269;drc=8e9c2dff53856916bbbc066f04b7ab0495d57a2f) for invalid/unset process locks, which should be the case for spare processes.
// The render process id here only used innit: \`render_process_id\`
// The render process id here only used innit: "is only"
// InstantService::ShouldServiceRequest() to block any child frame in
// a non-instant process from loading chrome-search URLs.Not for this CL, but we've got https://crbug.com/40447789 filed to eventually make chrome-search tiles into OOPIFs. (Today, we keep them in process with a pretty ugly site isolation workaround.) I'm curious if that existing code would get in the way of that?
Related details
- Adam Rice
- Daniel Cheng
- David Trainor
- Frank Liberato
- Liang Zhao
- Tibor Goldschwendt
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted and added 1 comment![]( "Open in Gerrit")
Votes added by Frank Liberato
| Code-Review | +1 |
1 comment
- Adam Rice
- Daniel Cheng
- David Trainor
- Liang Zhao
- Tibor Goldschwendt
- Wangsong Jin
Code-Coverage
Code-Review
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt added 2 comments![]( "Open in Gerrit")
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));The origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
+1
I don't think the spare renderer variant is necessary. The NTPs tested here are never instant NTPs.
Related details
- Adam Rice
- Daniel Cheng
- David Trainor
- Liang Zhao
- Wangsong Jin
David Trainor (Gerrit)
David Trainor added 2 comments![]( "Open in Gerrit")
// If the input |url| should be assigned to the Instant renderer.site_url?
The comment seems redundant with the method call below - can we elaborate more or remove it?
// receive the renderer settings even RenderProcessHost is reused.even -> even if?
Related details
- Adam Rice
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 7 comments![]( "Open in Gerrit")
// If the input |url| should be assigned to the Instant renderer.David Trainorsite_url?
The comment seems redundant with the method call below - can we elaborate more or remove it?
Done
// receive the renderer settings even RenderProcessHost is reused.Wangsong Jineven -> even if?
Done
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));Tibor GoldschwendtThe origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
+1
Thanks! It doesn't seem to return an error page. Found how other tests do it, I will try this first.
```
console_observer.SetPattern("Not allowed to load local resource: " +
view_source_url.spec());
```
Hi Alex, thanks for your response! I was mostly out of office on June 5, and it took me some time to re-consider - apologies for the delayed reply.
You're right about the behavior change: to avoid blocking browser-initiated navigations (e.g., from the address bar) while still blocking window.location = "chrome-search://xxx", we still need to call RegisterURLSchemeAsDisplayIsolated for renderer. However, that brings us back to the earlier issue—invoking RegisterURLSchemeAsDisplayIsolated() at that point could trigger the DCHECK(WTF::IsBeforeThreadCreated()).
BTW, Since registering the URL scheme is quite fragile, I’ve updated the code: if the feature is disabled, we’ll retain the scheme registration at its [original location](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/chrome_content_renderer_client.cc;l=541?q=WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated&ss=chromium%2Fchromium%2Fsrc:chrome%2F) during process startup, after checking the command-line switch. And in below [thread](https://chromium-review.googlesource.com/c/chromium/src/+/6373046/comment/5ec03902_73a538eb/) We've addressed the duplicate call issue, which is something we also want to avoid when registering the URL scheme.
Previously, we tried to either relax the DCHECK or register it first then remove for the instant process. Also tried to move OnRendererProcessLockedStateUpdated() before process_lock.IsValid() check to see if that block some cases, but still, we hit the DCHECK for certain tests. e.g. All/UserScriptTrackerBrowserTest.UserScriptViaUserScriptsApi/1 (run with InstantUsesSpareRenderer enabled) I haven't get chance to get all failed tests. Do you have some ideas?
I got below stack:
(Start the browser with empty page and open a new tab, disabled spare render)
First, SetProcessLock() is called.
```
content.dll!content::RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate() Line 3259 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:3259)
content.dll!content::RenderProcessHostImpl::SetProcessLock(const content::IsolationContext & isolation_context, const content::ProcessLock & process_lock) Line 3251 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:3251)
content.dll!content::SiteInstanceImpl::LockProcessIfNeeded() Line 1541 (q:\cr\src\content\browser\site_instance_impl.cc:1541)
content.dll!content::SiteInstanceImpl::SetProcessInternal(content::RenderProcessHost * process) Line 554 (q:\cr\src\content\browser\site_instance_impl.cc:554)
content.dll!content::SiteInstanceImpl::GetOrCreateProcess(const content::ProcessAllocationContext & context) Line 485 (q:\cr\src\content\browser\site_instance_impl.cc:485)
content.dll!content::RenderFrameHostManager::CreateRenderFrameHost(content::RenderFrameHostManager::CreateFrameCase create_frame_case, content::SiteInstanceImpl * site_instance, int frame_routing_id, mojo::PendingAssociatedRemote<content::mojom::Frame> frame_remote, const base::TokenType<blink::LocalFrameTokenTypeMarker> & frame_token, const base::TokenType<blink::DocumentTokenTypeMarker> & document_token, base::UnguessableToken devtools_frame_token, bool renderer_initiated_creation, scoped_refptr<content::BrowsingContextState> browsing_context_state, const content::ProcessAllocationContext & process_allocation_context) Line 4105 (q:\cr\src\content\browser\renderer_host\render_frame_host_manager.cc:4105)
content.dll!content::RenderFrameHostManager::InitRoot(content::SiteInstanceImpl * site_instance, bool renderer_initiated_creation, blink::FramePolicy initial_main_frame_policy, const std::__Cr::basic_string<char,std::__Cr::char_traits<char>,std::__Cr::allocator<char>> & name, const base::UnguessableToken & devtools_frame_token) Line 705 (q:\cr\src\content\browser\renderer_host\render_frame_host_manager.cc:705)
content.dll!content::FrameTree::Init(content::SiteInstanceImpl * main_frame_site_instance, bool renderer_initiated_creation, const std::__Cr::basic_string<char,std::__Cr::char_traits<char>,std::__Cr::allocator<char>> & main_frame_name, content::RenderFrameHostImpl * opener_for_origin, const blink::FramePolicy & frame_policy, const base::UnguessableToken & devtools_frame_token) Line 950 (q:\cr\src\content\browser\renderer_host\frame_tree.cc:950)
content.dll!content::WebContentsImpl::Init(const content::WebContents::CreateParams & params, blink::FramePolicy primary_main_frame_policy) Line 4099 (q:\cr\src\content\browser\web_contents\web_contents_impl.cc:4099)
content.dll!content::WebContentsImpl::CreateWithOpener(const content::WebContents::CreateParams & params, content::RenderFrameHostImpl * opener_rfh) Line 1672 (q:\cr\src\content\browser\web_contents\web_contents_impl.cc:1672)
content.dll!content::WebContentsImpl::Create(const content::WebContents::CreateParams & params) Line 779 (q:\cr\src\content\browser\web_contents\web_contents_impl.cc:779)
content.dll!content::WebContents::Create(const content::WebContents::CreateParams & params) Line 774 (q:\cr\src\content\browser\web_contents\web_contents_impl.cc:774)
chrome.dll!`anonymous namespace'::CreateTargetContents(const NavigateParams & params, const GURL & url) Line 547 (q:\cr\src\chrome\browser\ui\browser_navigator.cc:547)
chrome.dll!Navigate(NavigateParams * params) Line 859 (q:\cr\src\chrome\browser\ui\browser_navigator.cc:859)
chrome.dll!chrome::AddAndReturnTabAt(Browser * browser, const GURL & url, int idx, bool foreground, std::__Cr::optional<tab_groups::TabGroupId> group) Line 44 (q:\cr\src\chrome\browser\ui\browser_tabstrip.cc:44)
chrome.dll!chrome::AddTabAt(Browser * browser, const GURL & url, int idx, bool foreground, std::__Cr::optional<tab_groups::TabGroupId> group) Line 62 (q:\cr\src\chrome\browser\ui\browser_tabstrip.cc:62)
chrome.dll!chrome::BrowserTabStripModelDelegate::AddTabAt(const GURL & url, int index, bool foreground, std::__Cr::optional<tab_groups::TabGroupId> group) Line 76 (q:\cr\src\chrome\browser\ui\browser_tab_strip_model_delegate.cc:76)
chrome.dll!BrowserTabStripController::CreateNewTab() Line 624
```
Then RenderProcessHostImpl::OnProcessLaunched()
```
content.dll!content::RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate() Line 3259 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:3259)
content.dll!content::RenderProcessHostImpl::OnProcessLaunched() Line 5601 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:5601)
content.dll!content::ChildProcessLauncher::Notify(content::internal::ChildProcessLauncherHelper::Process process, unsigned long last_error, int error_code) Line 202 (q:\cr\src\content\browser\child_process_launcher.cc:202)
content.dll!content::internal::ChildProcessLauncherHelper::PostLaunchOnClientThread(content::internal::ChildProcessLauncherHelper::Process process, unsigned long last_error, int error_code) Line 497 (q:\cr\src\content\browser\child_process_launcher_helper.cc:497)
content.dll!base::internal::DecayedFunctorTraits<void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),content::internal::ChildProcessLauncherHelper *&&,content::internal::ChildProcessLauncherHelper::Process &&,unsigned long &&,int &&>::Invoke<void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int>(void(content::internal::ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int) method, scoped_refptr<content::internal::ChildProcessLauncherHelper> && receiver_ptr, content::internal::ChildProcessLauncherHelper::Process && args, unsigned long && args, int && args) Line 731 (q:\cr\src\base\functional\bind_internal.h:731)
content.dll!base::internal::InvokeHelper<0,base::internal::FunctorTraits<void (ChildProcessLauncherHelper::*&&)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),content::internal::ChildProcessLauncherHelper *&&,content::internal::ChildProcessLauncherHelper::Process &&,unsigned long &&,int &&>,void,0,1,2,3>::MakeItSo<void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),std::__Cr::tuple<scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int>>(void(content::internal::ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int) && functor, std::__Cr::tuple<scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int> && bound) Line 923 (q:\cr\src\base\functional\bind_internal.h:923)
content.dll!base::internal::Invoker<base::internal::FunctorTraits<void (ChildProcessLauncherHelper::*&&)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),content::internal::ChildProcessLauncherHelper *&&,content::internal::ChildProcessLauncherHelper::Process &&,unsigned long &&,int &&>,base::internal::BindState<1,1,0,void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int>,void ()>::RunImpl<void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),std::__Cr::tuple<scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int>,0,1,2,3>(void(content::internal::ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int) && functor, std::__Cr::tuple<scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int> && bound, std::__Cr::integer_sequence<unsigned long long,0,1,2,3>) Line 1060 (q:\cr\src\base\functional\bind_internal.h:1060)
content.dll!base::internal::Invoker<base::internal::FunctorTraits<void (ChildProcessLauncherHelper::*&&)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),content::internal::ChildProcessLauncherHelper *&&,content::internal::ChildProcessLauncherHelper::Process &&,unsigned long &&,int &&>,base::internal::BindState<1,1,0,void (ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, unsigned long, int),scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,unsigned long,int>,void ()>::RunOnce(base::internal::BindStateBase * base) Line 973
```
That's also what I observe when navigating a tab to a different site.
And I don't see they get [skipped](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_process_host_impl.cc;l=3269;drc=8e9c2dff53856916bbbc066f04b7ab0495d57a2f) And to avoid the second call, we added new flag `renderer_locked_state_updated_` in render_process_host_impl.cc. And reset it on RenderProcessHostImpl::ProcessDied() to cover process spawn after crashing case. We keep CHECK() in render to ensure there's no second call.
// The render process id here only used inWangsong Jinnit: \`render_process_id\`
Done
// The render process id here only used inWangsong Jinnit: "is only"
Done
Related details
- Alex Moshchuk
- Daniel Cheng
- David Trainor
- Liang Zhao
- Tibor Goldschwendt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 3 comments![]( "Open in Gerrit")
class StaticParamsUserData : public base::SupportsUserData::Data {We probably don't need this any more.
I don't think the spare renderer variant is necessary. The NTPs tested here are never instant NTPs.
We should revert changes in this file.
// InstantService::ShouldServiceRequest() to block any child frame in
// a non-instant process from loading chrome-search URLs.Not for this CL, but we've got https://crbug.com/40447789 filed to eventually make chrome-search tiles into OOPIFs. (Today, we keep them in process with a pretty ugly site isolation workaround.) I'm curious if that existing code would get in the way of that?
I think that the current URLLoaderFactory restriction is tied to requests from instant process. If we are going to move chrome-search tiles out of instant process, we could update the code to use renderer process id of the main frame if we want to keep ShouldServiceRequest concept to ensure that we only serves iframes related to instant process. Or we could just remove the ShouldServiceRequest related code to not have instant process concept in URLLoaderFactory, which makes it even more important to have the renderer process restriction on the scheme.
Related details
- Alex Moshchuk
- Daniel Cheng
- David Trainor
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
class StaticParamsUserData : public base::SupportsUserData::Data {We probably don't need this any more.
Done
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));Tibor GoldschwendtThe origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
Wangsong Jin+1
Thanks! It doesn't seem to return an error page. Found how other tests do it, I will try this first.
```
console_observer.SetPattern("Not allowed to load local resource: " +
view_source_url.spec());
```
Fixed.
Liang ZhaoI don't think the spare renderer variant is necessary. The NTPs tested here are never instant NTPs.
We should revert changes in this file.
- Alex Moshchuk
- Daniel Cheng
- David Trainor
- Liang Zhao
- Tibor Goldschwendt
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Trainor (Gerrit)
David Trainor added 1 comment![]( "Open in Gerrit")
Deferring to most of the other folks on this CL as the right people to be reviewing this. If you need me to review or stamp specific files after everyone aligns let me know. Moving myself to CC now, but please feel free to add me back if/when it's necessary. Thanks!
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
- Tibor Goldschwendt
- Wangsong Jin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tibor Goldschwendt
| Code-Review | +1 |
1 comment
//chrome/browser/ui/search/third_party_ntp_browsertest.cc LGTM
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));Tibor GoldschwendtThe origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
Wangsong Jin+1
Wangsong JinThanks! It doesn't seem to return an error page. Found how other tests do it, I will try this first.
```
console_observer.SetPattern("Not allowed to load local resource: " +
view_source_url.spec());
```
Fixed.
Done
Hi @alexmos@ and @dcheng, we need some inputs to move forward. Here're options we have:
1. Remove scheme registration and check ShouldServiceRequest for main and sub frames with the renderer process ID.
2. Register the scheme by default and remove it when we determine it is an instant process.
For option 1, currently, renderer initiated navigation to chrome-search urls (most visited, local and remote ntp urls) are blocked in renderer process for main frame or sub frame. When we remove the scheme registration, renderer initiated navigations will be allowed for those navigations. If it is an iframe navigation from normal web site, the navigation will fail due to urlloader restriction. However, if it is a main frame navigation, once the navigation is allowed, the frame will be assigned to instant process and the navigation will succeed. So, this will be a behavior change. Is this regression acceptable?
If not, we will need to keep RegisterURLSchemeAsDisplayIsolated(). We've tried different places to change where to register the scheme, but each attempt resulted in hitting DCHECK(WTF::IsBeforeThreadCreated()) in various tests. Our latest attempt was in ChromeContentRendererClient::WebViewCreated on patchset [56-57](https://chromium-review.googlesource.com/c/chromium/src/+/6373046/55..57) but still hit the DCHECK. Couldn't think about any other places. The only feasible one is option 2 to register it first and remove it for the instant process, which won't hit any DCHECK and maintains the renderer restriction without the above behavior change.
With above context, are you okay with the option 2?
// once and do not change.Included logs below from a test that navigates from example.com to the instant process URL. Two NotifyRendererOfLockedStateUpdate() called for the same process. There is no involvement of an empty site_url.
```
[43716:44840:0605/165434.839:INFO:content\browser\renderer_host\render_process_host_impl.cc:3238] SetProcessLock: { https://example.com/ } GetDeprecatedID: 7, site_url: chrome-search://remote-ntp/, process_lock.is_invalid(): 0, process_lock.allows_any_site(): 0, process_lock.IsASiteOrOrigin(): 1
[43716:44840:0605/165434.839:INFO:content\browser\renderer_host\render_process_host_impl.cc:3263] NotifyRendererOfLockedStateUpdate: { https://example.com/ } for process 7, site_url: chrome-search://remote-ntp/, process_lock.is_invalid(): 0, process_lock.allows_any_site(): 0, process_lock.IsASiteOrOrigin(): 1
[43716:44840:0605/165434.871:INFO:content\browser\renderer_host\render_process_host_impl.cc:5539] RenderProcessHostImpl::OnProcessLaunched() for 41456 (ID: 7)
[43716:44840:0605/165434.872:INFO:content\browser\renderer_host\render_process_host_impl.cc:3263] NotifyRendererOfLockedStateUpdate: { https://example.com/ } for process 7, site_url: chrome-search://remote-ntp/, process_lock.is_invalid(): 0, process_lock.allows_any_site(): 0, process_lock.IsASiteOrOrigin(): 1
```
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 2 comments![]( "Open in Gerrit")
EXPECT_EQ("null", content::EvalJs(subframe, "window.origin"));Tibor GoldschwendtThe origin is opaque presumably because there's an error page loaded in `subframe`? Can we check that window.location or the last committed URL is kUnreachableWebDataURL, just in case we ever change chrome-search: to use opaque origins too?
Wangsong Jin+1
Wangsong JinThanks! It doesn't seem to return an error page. Found how other tests do it, I will try this first.
```
console_observer.SetPattern("Not allowed to load local resource: " +
view_source_url.spec());
```
Fixed.
Done
it is a main frame navigation, once the navigation is allowed, the frame will be assigned to instant process and the navigation will succeed. So, this will be a behavior change. Is this regression acceptable?
Sigh, I'm personally not fond of giving web pages a new ability to navigate to chrome-search: URLs, so this would be nice to avoid.
I'd defer to dcheng@ on option 2 and whether it's workable, though it did sound like it would be problematic. I do want to mention an additional thing to consider here after we discussed this earlier in the week. The browser process also block attempts to request or commit WebUI pages from non-WebUI processes in ChildProcessSecurityPolicy::CanRequestURL(), [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/child_process_security_policy_impl.cc;l=1393-1407;drc=9ba7fc14c994ebb3a083726360612d50ec1de26f). This check is invoked in FilterURL ([here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_process_host_impl.cc;l=4479;drc=9ba7fc14c994ebb3a083726360612d50ec1de26f)) on renderer-initiated navigations, so normally, if a web renderer bypasses the blink checks and attempts a navigation to a WebUI scheme, FilterURL would block it by rewriting the target URL to about:blank#blocked. AFAICT, this should apply to chrome-search: as well (with an exception for most visited tiles [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_content_browser_client.cc;l=2058;drc=9ba7fc14c994ebb3a083726360612d50ec1de26f) - sigh).
So, I wonder if another option could be to use (or expand) this mechanism to still block navigations to chrome-search:, if the dynamic display-isolated scheme registration turns out to be impractical per @dch...@chromium.org's earlier comments? We'd probably lose the error messages on the blink side, but maybe we can live with that (or generate additional ones from the browser side)?
Related details
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Thanks for your inputs! Based on the suggestion from our sync meeting on 06/17:
We investigate more about the the register scheme hit DCHECK(WTF::IsBeforeThreadCreated()) issue. It seems to be related to the order of mojo calls not guaranteed on non associated interfaces. In this case, service worker thread is created via blink::mojom::EmbeddedWorkerInstanceClient interface after we lock the process and calls SetConfigurationOnProcessLockUpdate() in browser. However, in the renderer process, the service worker thread is created before SetConfigurationOnProcessLockUpdate() is received.
We confirmed this by disabling the ServiceWorkerAvoidMainThreadForInitialization feature in PatchSet 69 (InstantUsesSpareRenderer is also overridden to expose the DCHECK issue), and all tests passed.
Do you have any suggestions on what we should do? Thank you!
Related details
- Alex Moshchuk
- Daniel Cheng
- Liang Zhao
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
// InstantService::ShouldServiceRequest() to block any child frame in
// a non-instant process from loading chrome-search URLs.Liang ZhaoNot for this CL, but we've got https://crbug.com/40447789 filed to eventually make chrome-search tiles into OOPIFs. (Today, we keep them in process with a pretty ugly site isolation workaround.) I'm curious if that existing code would get in the way of that?
I think that the current URLLoaderFactory restriction is tied to requests from instant process. If we are going to move chrome-search tiles out of instant process, we could update the code to use renderer process id of the main frame if we want to keep ShouldServiceRequest concept to ensure that we only serves iframes related to instant process. Or we could just remove the ShouldServiceRequest related code to not have instant process concept in URLLoaderFactory, which makes it even more important to have the renderer process restriction on the scheme.
Close this since we've isolated the WebUIURLLoaderFactory change in this CL.
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
base::FEATURE_ENABLED_BY_DEFAULT); // [!!NEED RESET] Temporary overridePlaceholder to reset the feature flag back
Copied conversion from slack blink channel:
Wangsong:
> One option to solve this for instant process might be to also add renderer config to EmbeddedWorkerStartParams and from EmbeddedWorkerInstanceClientImpl::StartWorker() calls up a new method like GetContentClient()->renderer()->OnRenderConfigUpdate(config) to register scheme from if we have not done so before. We could also put this config into CreateViewParams so that we have the same pattern. This kind of goes back to the original approach of adding a browser client method to collect the config and and a renderer client method to apply the config and modify the mojo interfaces to carry the config. More changes to interfaces and complicated, but could work.
Another option would be have some "ProcessReady" or "RenderThreadReady" event and let the service worker creation thread wait for that event before it tries to create the worker, and we set that event when renderer config (instant flag) and other security related things are set.
Daniel:
> I personally lean towards this ("ProcessReady"), but I think we'll need to check with the worker team to see what they think, since I think the entire point of this was to make sure worker creation isn't blocked on the main thread.
Shunya:
> Thanks for looping me in. Could you help me to understand:
Why RegisterURLSchemeAsDisplayIsolated() should be called before the worker thread creation()?
Can the first-party NTP (google?) see the instant process flag, and use a spare renderer? Why it is difficult for other third-parties?
ServiceWorkerAvoidMainThreadForInitialization is the feature that our team recently shipped, we observed positive impacts on the various SW metrics (SW bootstrap time, FCP/LCP in the SW controlled page etc). It’s a bit difficult for us to just revert this flag. It would be nice to come up with some better options, but at least we’d ask you to monitor if there’s no regressions on the SW metrics.
Wangsong:
> The service worker thread creation calls NonMainThread::CreateThread -> WTF::WillCreateThread(); There's a DCHECK(WTF::IsBeforeThreadCreated()) avoids calling RegisterURLSchemeAsDisplayIsolated() after that. There're some tests failed for that.
1st party NTP does not use instant-process flag, so there's no issue involved. However, 3rd party currently relies on the instant-process command line switch. To enable the use of a spare renderer, we'll need to replace this switch, send the flag afterward, and call RegisterURLSchemeAsDisplayIsolated() at that point. However, this leads to the DCHECK issue because it's invoked after thread creation.
I saw this one https://chromium-review.googlesource.com/c/chromium/src/+/5568543 The general question is should we set the security param first before running the code, Currently, it seems we only need the CORS header for Service Workers, but might be a problem for other security related setup?
Daniel:
> Currently this (instant process) is set on the command-line, so we can make this decision at process startup. But because it is a command-line flag, we cannot reuse the spare process, because it won't have the right flag.
So the approach is to send the 'isolation info' down to the renderer when we lock it to a process.
Strictly speaking, it's (RegisterURLSchemeAsDisplayIsolated() should be called before the worker thread creation) not "required". But in general, it's a bit dangerous to mutate the URL scheme registry "too late"; once things have started in the process, the mutation might race with things reading the state. We could just exempt this one particular scheme type from these DCHECK()s, but conceptually it is a bit odd to relax it just for this one thing.
Shunya:
> This is not CORS header, but the CORS exempt header list. The SW need the CORS exempt header list for the startup, previously it was passed from the renderer main thread, but now it’s passed from the browser process.
I’m not sure what your question mean, could you elaborate?
ServiceWorkerAvoidMainThreadForInitialization is not just a nice-to-heve performance improvement actually, it was a bug fix for the regression introduced a couple of years ago (crbug.com/40753993 for more details). I believe the current behavior (SW doesn’t wait for the main thread) is the original intended behavior, and obviously it’s not welcomed to regress it again.
I want to hear what “ProcessReady” or “RenderThreadReady” exactly mean, too. Does that mean “updated the URL scheme registry and now the renderer main thread is ready”?
Liang:
> I can try to explain a little. There are security related settings that the browser process wants to set into renderer process before it tries to use it, like whether it is cross origin isolated, in RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate(). The "ProcessReady" would be an event set when these settings have processed in renderer process. We are adding a new settings that would register a scheme as display isolated, which would DCHECK if the process has already started parsing a page or created a worker. In the normal case where a process is created for a site, even if the page is under the control of a service worker, the DCHECK was not triggered, suggesting that adding a wait in service worker creation for the ProcessReady would not impact page navigation perf, probably due to enough time between the time we start navigation and create the renderer process for the site and the time we fetch service worker registration data and starts to create service worker. During the test, the DCHECK was only hit for some extension tests and IWA test.
Shunya:
> That’s good to know, but both extensions and IWA could affect navigations if service workers intercept requests. So not sure the navigation perf wouldn’t be impacted.
That’s said, I feel it’s worth trying making the process ready event and let the service worker bootstrap runs after the event.
Please don’t involve the renderer main thread to init the service worker, and monitoring service worker related metrics is much appreciated.
>
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Based on the above discussion, we've updated to wait for the ProcessReady signal before starting the service worker thread. The latest changes are somewhat messy due to some rebase modifications. It's more clear to review the change [PatchSet(69->72)](https://chromium-review.googlesource.com/c/chromium/src/+/6373046/69..72)
@dcheng and @sisidovski, could you help to take a look? Thanks!
Related details
- Daniel Cheng
- Liang Zhao
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();Why is this outside of IS_ANDROID? With the current code, renderer_configuration->SetConfigurationOnProcessLockUpdate() is not called for android, should we exclude that mojo method from android builds with [EnableIfNot=is_android]? Not sure whether we want to go further and exclude more functions like this one as a whole from android build instead of making the functions no op on android.
// Notifies the renderer has processed critical security settings from
// the browser. These settings (such as cross-origin isolation) are
// sent via `RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate()`
// and must be in place before the renderer can safely process web content.
virtual void OnProcessReady();Does this has to be declared on ContentRendererClient? It is only called directly on ChromeContentRendererClient.
Related details
- Daniel Cheng
- Shunya Shishido
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();Why is this outside of IS_ANDROID? With the current code, renderer_configuration->SetConfigurationOnProcessLockUpdate() is not called for android, should we exclude that mojo method from android builds with [EnableIfNot=is_android]? Not sure whether we want to go further and exclude more functions like this one as a whole from android build instead of making the functions no op on android.
Thanks! I missed moving it inside the build flag earlier. I've now updated the change to exclude the method from the Android build for better readability. Otherwise, it becomes difficult to understand the flow for Android.
// Notifies the renderer has processed critical security settings from
// the browser. These settings (such as cross-origin isolation) are
// sent via `RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate()`
// and must be in place before the renderer can safely process web content.
virtual void OnProcessReady();Does this has to be declared on ContentRendererClient? It is only called directly on ChromeContentRendererClient.
- Daniel Cheng
- Liang Zhao
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();Wangsong JinWhy is this outside of IS_ANDROID? With the current code, renderer_configuration->SetConfigurationOnProcessLockUpdate() is not called for android, should we exclude that mojo method from android builds with [EnableIfNot=is_android]? Not sure whether we want to go further and exclude more functions like this one as a whole from android build instead of making the functions no op on android.
Thanks! I missed moving it inside the build flag earlier. I've now updated the change to exclude the method from the Android build for better readability. Otherwise, it becomes difficult to understand the flow for Android.
Done
base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_wait;
process_ready_event_.Wait();We probably want to not wait if kInstantUsesSpareRenderer is disabled. When the feature is disabled, OnProcessReady() would not be called.
Related details
- Daniel Cheng
- Shunya Shishido
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();Wangsong JinWhy is this outside of IS_ANDROID? With the current code, renderer_configuration->SetConfigurationOnProcessLockUpdate() is not called for android, should we exclude that mojo method from android builds with [EnableIfNot=is_android]? Not sure whether we want to go further and exclude more functions like this one as a whole from android build instead of making the functions no op on android.
Thanks! I missed moving it inside the build flag earlier. I've now updated the change to exclude the method from the Android build for better readability. Otherwise, it becomes difficult to understand the flow for Android.
Done
base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_wait;
process_ready_event_.Wait();We probably want to not wait if kInstantUsesSpareRenderer is disabled. When the feature is disabled, OnProcessReady() would not be called.
- Daniel Cheng
- Liang Zhao
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 4 comments![]( "Open in Gerrit")
adding yyanagisawa as another SW owner.
BASE_DECLARE_FEATURE(kInstantUsesSpareRenderer);What is the plan to rollout this feature? We basically use [Finch](https://developer.chrome.com/docs/web-platform/chrome-finch) and run a/b testing. Wondering how this is going to be tested in the field.
void ChromeContentRendererClient::WaitProcessReady() {Do you mind adding the metrics to measure the taken time of this method?
if (!renderer_locked_state_updated_) {Can you elaborate on why `renderer_locked_state_updated_` is newly introduced? `NotifyRendererOfLockedStateUpdate()` is called multiple times but `OnRendererProcessLockedStateUpdated()` should be called only once unlike other parts in this method?
Related details
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
BASE_DECLARE_FEATURE(kInstantUsesSpareRenderer);What is the plan to rollout this feature? We basically use [Finch](https://developer.chrome.com/docs/web-platform/chrome-finch) and run a/b testing. Wondering how this is going to be tested in the field.
Since this is a third-party NTP feature, we will initially keep it disabled in Chromium and then roll it out downstream. We plan to collect performance data on the creation of third-party NTPs and the time token for WaitProcessReady(). We will then return to the upstream team to discuss whether we want to roll out it in Chromium.
void ChromeContentRendererClient::WaitProcessReady() {Do you mind adding the metrics to measure the taken time of this method?
Sure, added. Previous testing results revealed that mostly extension processes require this wait time. Therefore, we separated extension and non-extension processes for the metrics. From local testing, we found that in most cases, the wait time is around 20ms. We log instances where the wait time exceeds 20ms to identify potential performance issues.
if (!renderer_locked_state_updated_) {Can you elaborate on why `renderer_locked_state_updated_` is newly introduced? `NotifyRendererOfLockedStateUpdate()` is called multiple times but `OnRendererProcessLockedStateUpdated()` should be called only once unlike other parts in this method?
I'm aware that these settings can be applied multiple times, which is unnecessary. Specifically, OnRendererProcessLockedStateUpdated() also triggers display as isolated scheme registration in the renderer, and we only need to register it once. I believe it's better for the other static settings to be set only once as well. However, to mitigate any risks in case of changes, I want to isolate this.
There's a previous thread we discuss why we add `renderer_locked_state_updated_ ` https://chromium-review.googlesource.com/c/chromium/src/+/6373046/comment/5ec03902_73a538eb/
Related details
- Daniel Cheng
- Liang Zhao
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
BASE_DECLARE_FEATURE(kInstantUsesSpareRenderer);Wangsong JinWhat is the plan to rollout this feature? We basically use [Finch](https://developer.chrome.com/docs/web-platform/chrome-finch) and run a/b testing. Wondering how this is going to be tested in the field.
Since this is a third-party NTP feature, we will initially keep it disabled in Chromium and then roll it out downstream. We plan to collect performance data on the creation of third-party NTPs and the time token for WaitProcessReady(). We will then return to the upstream team to discuss whether we want to roll out it in Chromium.
Done
void ChromeContentRendererClient::WaitProcessReady() {Wangsong JinDo you mind adding the metrics to measure the taken time of this method?
Sure, added. Previous testing results revealed that mostly extension processes require this wait time. Therefore, we separated extension and non-extension processes for the metrics. From local testing, we found that in most cases, the wait time is around 20ms. We log instances where the wait time exceeds 20ms to identify potential performance issues.
Done
if (!renderer_locked_state_updated_) {Wangsong JinCan you elaborate on why `renderer_locked_state_updated_` is newly introduced? `NotifyRendererOfLockedStateUpdate()` is called multiple times but `OnRendererProcessLockedStateUpdated()` should be called only once unlike other parts in this method?
I'm aware that these settings can be applied multiple times, which is unnecessary. Specifically, OnRendererProcessLockedStateUpdated() also triggers display as isolated scheme registration in the renderer, and we only need to register it once. I believe it's better for the other static settings to be set only once as well. However, to mitigate any risks in case of changes, I want to isolate this.
There's a previous thread we discuss why we add `renderer_locked_state_updated_ ` https://chromium-review.googlesource.com/c/chromium/src/+/6373046/comment/5ec03902_73a538eb/
Done
Shunya Shishido (Gerrit)
Shunya Shishido added 4 comments![]( "Open in Gerrit")
void ChromeContentRendererClient::WaitProcessReady() {Wangsong JinDo you mind adding the metrics to measure the taken time of this method?
Wangsong JinSure, added. Previous testing results revealed that mostly extension processes require this wait time. Therefore, we separated extension and non-extension processes for the metrics. From local testing, we found that in most cases, the wait time is around 20ms. We log instances where the wait time exceeds 20ms to identify potential performance issues.
Done
Thanks. It doesn't make sense to me that we log the time only when it exceeds 20ms. Can we record this every time?
if (wait_duration >= base::Milliseconds(20)) {I want to understand 1) the frequency of waiting for the process here, and 2) the taken time even under 20ms.
? "ServiceWorker.InitiatorThread.WaitTime.Over20ms.ExtensionProcess"WaitTimeForProcessReady?
? "ServiceWorker.InitiatorThread.WaitTime.Over20ms.ExtensionProcess"Maybe we don't need this?
Related details
- Daniel Cheng
- Liang Zhao
- Wangsong Jin
- Yoshisato Yanagisawa
Wangsong Jin (Gerrit)
Wangsong Jin added 4 comments![]( "Open in Gerrit")
void ChromeContentRendererClient::WaitProcessReady() {Wangsong JinDo you mind adding the metrics to measure the taken time of this method?
Wangsong JinSure, added. Previous testing results revealed that mostly extension processes require this wait time. Therefore, we separated extension and non-extension processes for the metrics. From local testing, we found that in most cases, the wait time is around 20ms. We log instances where the wait time exceeds 20ms to identify potential performance issues.
Shunya ShishidoDone
Thanks. It doesn't make sense to me that we log the time only when it exceeds 20ms. Can we record this every time?
Thank you! I've updated to record every time.
I want to understand 1) the frequency of waiting for the process here, and 2) the taken time even under 20ms.
Thanks for clarifying! I've added the metric to record whether the wait time has been taken for Frequency. Also, I've removed the > 20ms threshold.
? "ServiceWorker.InitiatorThread.WaitTime.Over20ms.ExtensionProcess"Wangsong JinWaitTimeForProcessReady?
Updated.
? "ServiceWorker.InitiatorThread.WaitTime.Over20ms.ExtensionProcess"Maybe we don't need this?
- Daniel Cheng
- Liang Zhao
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi sisidovski@, thank you for reviewing the worker-related changes! Let me know if you have any further feedback. If not, I’ll go ahead and ask the owner of the other area to take a look. Thanks again!
Related details
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 5 comments![]( "Open in Gerrit")
Hi sisidovski@, thank you for reviewing the worker-related changes! Let me know if you have any further feedback. If not, I’ll go ahead and ask the owner of the other area to take a look. Thanks again!
Thanks, added metrics looks good, I still have a couple of questions though. Please don't try to get +1 one by one, just ask more broader reviews.
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();`params` should be created when it's actually used. Could you move this after the feature flag check?
if (search::ShouldAssignURLToInstantRenderer(site_url, profile)) {Does this mean some other pages which are not third-party NTPs may dynamically update the renderer process with the instant process flag after this CL? Also what about 1st party NTP (chrome://new-tab-page)? The 1st party NTP will also run this new code path?
struct StaticParams {I feel `StaticParams` is not appropriate since `is_instant_process` is dynamically updated.
process_ready_event_.Wait();I'm not familiar with `base::WaitableEvent`, but what happens if `OnProcessReady()` is not called? `EmbeddedWorkerInstanceClientImpl::StartWorker()` is called regardless of whether the third-party NTP is involved or not. Just wondering if it will work without `OnProcessReady()`, or calling `OnProcessReady()` is guaranteed?
Related details
- Wangsong Jin
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
Wangsong Jin (Gerrit)
Wangsong Jin added 5 comments![]( "Open in Gerrit")
@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
We call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
chrome::mojom::StaticParamsPtr params = chrome::mojom::StaticParams::New();`params` should be created when it's actually used. Could you move this after the feature flag check?
Fixed.
if (search::ShouldAssignURLToInstantRenderer(site_url, profile)) {Does this mean some other pages which are not third-party NTPs may dynamically update the renderer process with the instant process flag after this CL? Also what about 1st party NTP (chrome://new-tab-page)? The 1st party NTP will also run this new code path?
Yes — since we late bind the is-instant process flag. and every renderer needs to check that flag in its logic. This also applies to the first-party NTPs
I feel `StaticParams` is not appropriate since `is_instant_process` is dynamically updated.
This `is_instant_process` flag is set only once per renderer process. In ChromeRenderThreadObserver::SetConfigurationOnProcessLockUpdate(), we have CHECK(!static_params_) to enforce that this value once set once.
process_ready_event_.Wait();I'm not familiar with `base::WaitableEvent`, but what happens if `OnProcessReady()` is not called? `EmbeddedWorkerInstanceClientImpl::StartWorker()` is called regardless of whether the third-party NTP is involved or not. Just wondering if it will work without `OnProcessReady()`, or calling `OnProcessReady()` is guaranteed?
Wait won’t end until `OnProcessReady()` is called. But the design is based on `OnProcessReady()` is always happen after the renderer process is created and its security is set up. The caller `NotifyRendererOfLockedStateUpdate()->SetConfigurationOnProcessLockUpdate()` also has other settings which should be always called for renderers.
If `OnProcessReady()` isn’t called, it indicates a security issue - missing a call to `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` for renderer processes. To make this more robust, maybe we can add a timeout and call DwC to avoid hanging. What do you think? And What value do you think would be a good threshold? Thanks!
Related details
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
Shunya ShishidoHi sisidovski@, thank you for reviewing the worker-related changes! Let me know if you have any further feedback. If not, I’ll go ahead and ask the owner of the other area to take a look. Thanks again!
Thanks, added metrics looks good, I still have a couple of questions though. Please don't try to get +1 one by one, just ask more broader reviews.
I missed this message earlier and have re-added Daniel to review the other part of the change
Wangsong Jin@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
We call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
Done
process_ready_event_.Wait();Wangsong JinI'm not familiar with `base::WaitableEvent`, but what happens if `OnProcessReady()` is not called? `EmbeddedWorkerInstanceClientImpl::StartWorker()` is called regardless of whether the third-party NTP is involved or not. Just wondering if it will work without `OnProcessReady()`, or calling `OnProcessReady()` is guaranteed?
Wait won’t end until `OnProcessReady()` is called. But the design is based on `OnProcessReady()` is always happen after the renderer process is created and its security is set up. The caller `NotifyRendererOfLockedStateUpdate()->SetConfigurationOnProcessLockUpdate()` also has other settings which should be always called for renderers.
If `OnProcessReady()` isn’t called, it indicates a security issue - missing a call to `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` for renderer processes. To make this more robust, maybe we can add a timeout and call DwC to avoid hanging. What do you think? And What value do you think would be a good threshold? Thanks!
Added a 5-second timeout followed by a DwC action. Let me know if you have any alternative suggestions.
Related details
- Daniel Cheng
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Updated to WaitProcessReady() before starting the service worker thread solve the issue. Close this thread now.
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
Wangsong Jin@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
Wangsong JinWe call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
Done
Does RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme) have to be called only when the instant process flag is not enabled? We can't just call it unconditionally?
Related details
- Daniel Cheng
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
Wangsong Jin@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
Wangsong JinWe call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
Shunya ShishidoDone
Does RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme) have to be called only when the instant process flag is not enabled? We can't just call it unconditionally?
Yep.
Wangsong Jin@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
Wangsong JinWe call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
Shunya ShishidoDone
Does RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme) have to be called only when the instant process flag is not enabled? We can't just call it unconditionally?
Unfortunately, we can’t. This was also discussed [here](https://chromium-review.googlesource.com/c/chromium/src/+/6373046/comment/ca0b133a_d9863f35/). There’s historical context behind the special handling of chrome-search and display-isolated schemes (https://issues.chromium.org/issues/40309067 and https://chromiumcodereview.appspot.com/12621008/). We can’t simply register this for the instant process, as that would introduce behavior change.
Related details
- Daniel Cheng
- Shunya Shishido
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 5 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
Wangsong Jin@wangs...@microsoft.com Overall I feel that it would be great if `SetConfigurationOnProcessLockUpdate()` is called only when it's needed. Since it introduces additional mojo call in the service worker startup dependency, in the worst case a deadlock could happen in the initiator thread. If `SetConfigurationOnProcessLockUpdate()` and the lock mechanism are used only from third-party NTPs, this change is less concerning. WDYT?
Wangsong JinWe call SetConfigurationOnProcessLockUpdate() for all renderer processes, not just third-party NTPs, because we need to ensure that: `RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme)` is called for non-instant processes. And `SetConfigurationOnProcessLockUpdate()` is the earliest earliest safe point to do so. Theoretically, it's possible to set a flag only for third-party NTPs and find a later place to check the flag and register the URL scheme. But as we have to ensure that RegisterURLSchemeAsDisplayIsolated() is called before `WTF::IsBeforeThreadCreated()`, we would require more delays in `WaitForProcessReady()` before initiating the thread.
Shunya ShishidoDone
Wangsong JinDoes RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme) have to be called only when the instant process flag is not enabled? We can't just call it unconditionally?
Shunya ShishidoYep.
Thanks for explaining. I understood chrome-search shouldn't be registered as display isolated for third-party NTP.
LGTM w/ nits.
As the Chrome loading perspective, we'd like to ask the Chrome side POC to run the Finch experiment with monitoring ServiceWorker metrics to ensure the regression is negligible. I would appreciate if someone from the CSA team take this.
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Based on the discussion in this CL, could you add a comment why chrome-search scheme should not be registered as display isolated when 1) the process doesn't have instant flag, or 2) kInstantUsesSpareRenderer is enabled?
void RegisterDisplayIsolatedURLScheme() {Could you make this more concrete? e.g. `ResisterChromeSearchSchemeDisplayIsolated()`.
WebString chrome_search_scheme(ditto. Adding a comment would be helpful.
Related details
- Daniel Cheng
- Wangsong Jin
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
LGTM w/ nits.
As the Chrome loading perspective, we'd like to ask the Chrome side POC to run the Finch experiment with monitoring ServiceWorker metrics to ensure the regression is negligible. I would appreciate if someone from the CSA team take this.
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 3 comments![]( "Open in Gerrit")
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated(chrome_search_scheme);Based on the discussion in this CL, could you add a comment why chrome-search scheme should not be registered as display isolated when 1) the process doesn't have instant flag, or 2) kInstantUsesSpareRenderer is enabled?
Sure, added!
Could you make this more concrete? e.g. `ResisterChromeSearchSchemeDisplayIsolated()`.
Yeah, updated.
ditto. Adding a comment would be helpful.
- Alex Moshchuk
- Daniel Cheng
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 4 comments![]( "Open in Gerrit")
renderer_client_->OnProcessReady();Layering-wise, I think it would make more sense for the WaitableEvent to be here, and for ChromeContentRendererClient to delegate the WaitForProcessReady() call to the render thread observer.
Then we don't need to set a back pointer to the content renderer client implementation.
bool renderer_locked_state_updated_ = false;Our convention here isn't very strong, but I personally like it when bools start with "is_" or "did_" or "has_", so this would be something like `did_update_renderer_locked_state_`. But I guess that doesn't pair quite as well with the method name... so I'll leave this up to you.
if (!renderer_locked_state_updated_) {In earlier patchsets, I believe we mentioned that this can be called more than once?
We should maybe document why in a comment... but ideally we just would not call this more than once.
(e.g. it's not clear why we need to guard this but not `SetIsLockedToSite()`; it seems like some of these methods are idempotent while others aren't or might be dynamically updated, and it becomes a bit hard to understand what's going on here)
virtual void WaitProcessReady();Nit: `WaitForProcessReady()`
Related details
- Alex Moshchuk
- Wangsong Jin
Wangsong Jin (Gerrit)
Wangsong Jin added 4 comments![]( "Open in Gerrit")
Layering-wise, I think it would make more sense for the WaitableEvent to be here, and for ChromeContentRendererClient to delegate the WaitForProcessReady() call to the render thread observer.
Then we don't need to set a back pointer to the content renderer client implementation.
Yeah, that's a good idea. I’ve refactored the code. Thanks!
Our convention here isn't very strong, but I personally like it when bools start with "is_" or "did_" or "has_", so this would be something like `did_update_renderer_locked_state_`. But I guess that doesn't pair quite as well with the method name... so I'll leave this up to you.
Updated to `did_update_renderer_locked_state_`.
In earlier patchsets, I believe we mentioned that this can be called more than once?
We should maybe document why in a comment... but ideally we just would not call this more than once.
(e.g. it's not clear why we need to guard this but not `SetIsLockedToSite()`; it seems like some of these methods are idempotent while others aren't or might be dynamically updated, and it becomes a bit hard to understand what's going on here)
I’ve added a comment. I think it's better for other settings that are intended to be configured only once should also be gated, or we should find a place where they are invoked just once per renderer. I’ve created a separate task to for this: http://crbug.com/434735272.
virtual void WaitProcessReady();Wangsong JinNit: `WaitForProcessReady()`
Done
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
Shunya ShishidoLGTM w/ nits.
As the Chrome loading perspective, we'd like to ask the Chrome side POC to run the Finch experiment with monitoring ServiceWorker metrics to ensure the regression is negligible. I would appreciate if someone from the CSA team take this.
Thanks for highlighting this. Discussed with CSA team today (07/29/), the take away is that the feature will firstly be rolled out downstream. We'll monitor 3rd NTP and SW creation time metrics and share the result to the CSA team. If the metrics look fine, the CSA team proceed with a rollout with monitoring the default set of guardrails. cc: @dch...@chromium.org @ale...@chromium.org
base::FEATURE_ENABLED_BY_DEFAULT); // [!!NEED RESET] Temporary overridePlaceholder to reset the feature flag back
- Alex Moshchuk
- Daniel Cheng
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
Shunya ShishidoLGTM w/ nits.
As the Chrome loading perspective, we'd like to ask the Chrome side POC to run the Finch experiment with monitoring ServiceWorker metrics to ensure the regression is negligible. I would appreciate if someone from the CSA team take this.
Wangsong Jin
Thanks for highlighting this. Discussed with CSA team today (07/29/), the take away is that the feature will firstly be rolled out downstream. We'll monitor 3rd NTP and SW creation time metrics and share the result to the CSA team. If the metrics look fine, the CSA team proceed with a rollout with monitoring the default set of guardrails. cc: @dch...@chromium.org @ale...@chromium.org
SGTM. Here is our key metrics. Please monitor these metrics when you rollout:
- `ServiceWorker.LoadTiming.MainFrame.MainResource.ForwardServiceWorkerToWorkerReady2`
- `Extensions.ServiceWorkerBackground.StartWorkerTime`
- `PageLoad.Clients.ServiceWorker2.ParseTiming.NavigationToParseStart`
- `PageLoad.Clients.ServiceWorker2.PaintTiming.NavigationToLargestContentfulPaint2`
- `PageLoad.ParseTiming.NavigationToParseStart`
- `PageLoad.PaintTiming.NavigationToLargestContentfulPaint2`
Related details
- Alex Moshchuk
- Daniel Cheng
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
base::FEATURE_ENABLED_BY_DEFAULT); // [!!NEED RESET] Temporary overrideWangsong JinPlaceholder to reset the feature flag back
Will revert this after passing the DRY CQ Run.
PatchSet 87 passed all the tests. https://chromium-review.googlesource.com/c/chromium/src/+/6373046/87?tab=checks
Reverted to disable the feature flag by default.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi @ale...@chromium.org and @dch...@chromium.org, could you help to take a look when you get a chance? Thanks a lot!
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 10 comments![]( "Open in Gerrit")
Thanks, this is looking close; a few more comments from my side and we should be good.
if (!channel) {Do we need to handle this case? We effectively return a `nullptr` here, but we don't handle `nullptr` in the caller, which implies that this isn't reached; having no channel usually means the renderer crashed (I think?), and we shouldn't be committing things in that state.
if (profile) {Do we really need a null check here? It looks like this can only happen in incorrectly-configured tests.
const char* kChromeSearchIframeUrl =```suggestion
const char kChromeSearchIframeUrl[] =
```
(As written, this is a **mutable** pointer to a string literal, so `kChromeSearchIframeUrl` can be assigned)
std::string script = std::string(
"const frame = document.createElement('iframe');"
"frame.src = '") +
kChromeSearchIframeUrl +
"';"
"document.body.appendChild(frame);";Nit: use `base::StrCat()` instead of `std::string::operator+`, which has bad code generation and algorithmic complexity)
std::string("window.location= '") + kChromeSearchIframeUrl + "';";Same comment: use `base::StrCat()` here please.
bool should_register_chrome_search_scheme =Naming suggestion:
```suggestion
bool should_restrict_chrome_search_scheme =
```
LOG(ERROR) << "ServiceWorker.InitiatorThread: Process ready wait time took "Do you expect anyone to consume this log message? Otherwise let's use `DLOG`
content::BrowserContext* browser_context,I might suggest only passing the `RenderProcessHost` here; the caller can grab the `BrowserContext` from it if it needs it.
// Called when the locked state of a renderer process is updated.Nit: it would be nice to be a bit more specific here. When is the locked state updated? Can this be called more than once? et cetera
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Wangsong Jin (Gerrit)
Wangsong Jin added 9 comments![]( "Open in Gerrit")
Do we need to handle this case? We effectively return a `nullptr` here, but we don't handle `nullptr` in the caller, which implies that this isn't reached; having no channel usually means the renderer crashed (I think?), and we shouldn't be committing things in that state.
Thanks! We don't need to call if there's no channel - renderer crashed. I've added the non nullptr check in the caller to handle this case.
Do we really need a null check here? It looks like this can only happen in incorrectly-configured tests.
Removed.
```suggestion
const char kChromeSearchIframeUrl[] =
```(As written, this is a **mutable** pointer to a string literal, so `kChromeSearchIframeUrl` can be assigned)
Done
std::string script = std::string(
"const frame = document.createElement('iframe');"
"frame.src = '") +
kChromeSearchIframeUrl +
"';"
"document.body.appendChild(frame);";Nit: use `base::StrCat()` instead of `std::string::operator+`, which has bad code generation and algorithmic complexity)
Thanks! Refined.
std::string("window.location= '") + kChromeSearchIframeUrl + "';";Same comment: use `base::StrCat()` here please.
Done
Naming suggestion:
```suggestion
bool should_restrict_chrome_search_scheme =
```
Done
LOG(ERROR) << "ServiceWorker.InitiatorThread: Process ready wait time took "Do you expect anyone to consume this log message? Otherwise let's use `DLOG`
Done
I might suggest only passing the `RenderProcessHost` here; the caller can grab the `BrowserContext` from it if it needs it.
Thanks! I’ve removed `BrowserContext`. I kept site_url because including content/browser/process_lock.h in chrome_content_browser_client.cc didn’t feel appropriate.
// Called when the locked state of a renderer process is updated.Nit: it would be nice to be a bit more specific here. When is the locked state updated? Can this be called more than once? et cetera
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
if (renderer_configuration) {To be very clear: did you test this? My expectation is that we would **not** call `OnRendererProcessLockedStateUpdated()` at all if the process is crashed. I **would** expect it to be called again after the renderer crashes–but only after the renderer is re-created, so the channel would exist. So none of the null checks should be needed, unless i'm missing something.
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
if (renderer_configuration) {To be very clear: did you test this? My expectation is that we would **not** call `OnRendererProcessLockedStateUpdated()` at all if the process is crashed. I **would** expect it to be called again after the renderer crashes–but only after the renderer is re-created, so the channel would exist. So none of the null checks should be needed, unless i'm missing something.
Yeah, I've tested the renderer crash case. As we reset the `did_update_renderer_locked_state_` flag in `RenderProcessHostImpl::ProcessDied()`. Once after the process is re-created, we will call it again (channel recreated)
```
chrome.dll!`anonymous namespace'::GetRendererConfiguration(content::RenderProcessHost * render_process_host) Line 1007 (q:\cr\src\chrome\browser\chrome_content_browser_client.cc:1007)
chrome.dll!ChromeContentBrowserClient::OnRendererProcessLockedStateUpdated(content::RenderProcessHost * host, const GURL & site_url) Line 1908 (q:\cr\src\chrome\browser\chrome_content_browser_client.cc:1908)
content.dll!content::RenderProcessHostImpl::NotifyRendererOfLockedStateUpdate() Line 3290 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:3290)
content.dll!content::RenderProcessHostImpl::OnProcessLaunched() Line 5590 (q:\cr\src\content\browser\renderer_host\render_process_host_impl.cc:5590)
content.dll!content::ChildProcessLauncher::Notify(content::internal::ChildProcessLauncherHelper::Process process, unsigned long last_error, int error_code) Line 204 (q:\cr\src\content\browser\child_process_launcher.cc:204)
```
So yes, we don't expect the channel/configuration to be nullptr. I removed those checks.
Related details
- Alex Moshchuk
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
Related details
- Alex Moshchuk
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi folks, could you help to take a look when you get a chance? Thanks!
Related details
- Alex Moshchuk
- Marc Treib
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
2 comments
LGTM w/ nit
// also meant to be aplied once but may currently be updated dynamically.Please fix this WARNING reported by Spellchecker: "aplied" is a possible misspelling of "applied".
To bypass Spellchecker, add a ...
"aplied" is a possible misspelling of "applied".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Related details
- Alex Moshchuk
- Marc Treib
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
Hi folks, could you help to take a look when you get a chance? Thanks!
Can you please specify what you want me to look at? (I'm in chrome/OWNERS so I own most of the files here, but I'm not the ideal reviewer for many of them.)
Related details
- Alex Moshchuk
- Wangsong Jin
- Yoshisato Yanagisawa
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
Marc TreibHi folks, could you help to take a look when you get a chance? Thanks!
Can you please specify what you want me to look at? (I'm in chrome/OWNERS so I own most of the files here, but I'm not the ideal reviewer for many of them.)
I see. Could you help look at the following files or areas that might need your input?
- chrome/renderer/ chrome_render_frame_observer.cc
- chrome/renderer/ chrome_render_thread_observer.h
- chrome/renderer/ chrome_render_thread_observer.cc
- chrome/renderer/ process_state.h
- chrome/renderer/ process_state.cc
// also meant to be aplied once but may currently be updated dynamically.Please fix this WARNING reported by Spellchecker: "aplied" is a possible misspelling of "applied".
To bypass Spellchecker, add a ...
"aplied" is a possible misspelling of "applied".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
- Alex Moshchuk
- Marc Treib
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib voted and added 5 comments![]( "Open in Gerrit")
Votes added by Marc Treib
| Code-Review | +1 |
Marc TreibHi folks, could you help to take a look when you get a chance? Thanks!
Wangsong JinCan you please specify what you want me to look at? (I'm in chrome/OWNERS so I own most of the files here, but I'm not the ideal reviewer for many of them.)
I see. Could you help look at the following files or areas that might need your input?
- chrome/renderer/ chrome_render_frame_observer.cc
- chrome/renderer/ chrome_render_thread_observer.h
- chrome/renderer/ chrome_render_thread_observer.cc
- chrome/renderer/ process_state.h
- chrome/renderer/ process_state.cc
LGTM % nits for these files. I have *not* reviewed the overall change - seems like this has plenty of coverage from the other reviewers.
chrome_observer_->WaitForProcessReady(base::Seconds(5));optional: If there's any question about the timeout duration, you might want to make this a FeatureParam, so it can be configured remotely. See [example](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/chrome_features.h;drc=9f18cc08f11f0d844a9f80af9eb900cd28f76224;l=474) for how to define such a FeatureParam.
void RegisterChromeSearchSchemeDisplayIsolated() {`MaybeRegisterChromeSearchSchemeDisplayIsolated`? Since it might or might not actually do it.
chrome::mojom::StaticParamsPtr params) {
// Ensure static renderer configuration parameters are set
// once.nit: fits on one line
static_params_ = std::move(params);`static_params_` is unused, except for the `CHECK` above - do we really need to store it?
If there are no plans to use it, I'd replace it with something like `bool set_configuration_on_process_lock_update_called_` or `bool static_renderer_params_set_`.
Related details
- Alex Moshchuk
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Wangsong Jin (Gerrit)
Wangsong Jin added 5 comments![]( "Open in Gerrit")
Hi folks, when you get a chance, could you please take a look at the remaining files? Thank you!
Add ricea@ for c/r/n:
- chrome/renderer/net/net_error_helper.cc
- (This file is auto-covered by Treib@, but it would be great if the area owner could review it. The update is minor and only involves the method namespace).
Add yyanagisawa@ for service worker histograms:
- tools/metrics/histograms/metadata/service/histograms.xml
Add alexmos@ for content/:
- content/browser/renderer_host/render_process_host_impl.h
- content/browser/renderer_host/render_process_host_impl.cc
- content/public/browser/content_browser_client.h
- content/public/browser/content_browser_client.cc
- content/public/renderer/content_renderer_client.h
- content/public/renderer/content_renderer_client.cc
chrome_observer_->WaitForProcessReady(base::Seconds(5));optional: If there's any question about the timeout duration, you might want to make this a FeatureParam, so it can be configured remotely. See [example](https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/chrome_features.h;drc=9f18cc08f11f0d844a9f80af9eb900cd28f76224;l=474) for how to define such a FeatureParam.
Done
`MaybeRegisterChromeSearchSchemeDisplayIsolated`? Since it might or might not actually do it.
Done
chrome::mojom::StaticParamsPtr params) {
// Ensure static renderer configuration parameters are set
// once.nit: fits on one line
Done
`static_params_` is unused, except for the `CHECK` above - do we really need to store it?
If there are no plans to use it, I'd replace it with something like `bool set_configuration_on_process_lock_update_called_` or `bool static_renderer_params_set_`.
- Adam Rice
- Alex Moshchuk
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 2 comments![]( "Open in Gerrit")
Thanks, happy to see that this is getting close! One question below, otherwise content/ looks good.
#if !BUILDFLAG(IS_ANDROID)Sorry if this was already discussed, but why do we want to gate all the content bits behind !IS_ANDROID? Seems that at least the content/ parts of this are fairly platform-agnostic and something that we'd want on Android too, and if the chrome content client doesn't ever use the instant process on Android, the OnRendererProcessLockedStateUpdated(), etc. can just be a no-op at that layer?
Related details
- Adam Rice
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
#else // BUILDFLAG(IS_ANDROID)
return;probably don't need these. It will return naturally.
Related details
- Adam Rice
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wangsong Jin (Gerrit)
Wangsong Jin added 2 comments![]( "Open in Gerrit")
probably don't need these. It will return naturally.
Done
Sorry if this was already discussed, but why do we want to gate all the content bits behind !IS_ANDROID? Seems that at least the content/ parts of this are fairly platform-agnostic and something that we'd want on Android too, and if the chrome content client doesn't ever use the instant process on Android, the OnRendererProcessLockedStateUpdated(), etc. can just be a no-op at that layer?
Thanks! There wasn’t a strong preference around this. We made the change primarily for code readability—excluding Android made the flow easier to understand, as Android doesn’t involve the instant process. But considering that the content/ parts might be useful for Android in the future, it makes sense to include Android. I’ve updated both OnRendererProcessLockedStateUpdated() and WaitForProcessReady() to be defined across all platforms, with no-op implementations for Android.
Related details
- Adam Rice
- Alex Moshchuk
- Liang Zhao
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
Related details
- Adam Rice
- Wangsong Jin
- Yoshisato Yanagisawa
Wangsong Jin (Gerrit)
Wangsong Jin added 1 comment![]( "Open in Gerrit")
Hi toyoshim@, Could you help to take a look below service worker histogram change when you have time? Thank you!
- tools/metrics/histograms/metadata/service/histograms.xml
Related details
- Adam Rice
- Takashi Toyoshima
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Liao (Gerrit)
Robert Liao added 1 comment![]( "Open in Gerrit")
Adding kerenzhu as a starting point for the spare renderer.
Related details
- Keren Zhu
- Takashi Toyoshima
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 10 comments![]( "Open in Gerrit")
friend class ::ChromeContentRendererClient;nit: Should this be after the `::BrowserProcessImpl` at the line 835?
void ChromeContentRendererClient::WaitForProcessReady() {This method is defined as a content public API, and there is no guarantee that this should be called by ServiceWorker renderers.
So, having this assumption here seems strange, and can be wrong easily?
bool ready_within_timeout =
chrome_observer_->WaitForProcessReady(base::Seconds(5));Should we call this always? (regardless of the `process_was_ready` check result?
DLOG(ERROR)
<< "ServiceWorker.InitiatorThread: Process ready wait time took "
"more than 5 seconds for "
<< (is_extension ? "extension" : "regular") << " process";This is one part that has an assumption for the caller.
I'm not sure this log is useful for debugging or investigation.
Instead, how about using the SCOPED_CRASH_KEY_BOOL macro to ensure this check result is included in the dump report?
Otherwise, you may remove the "ServiceWorker.InitiatorThreaad" prefix in the log.
base::TimeDelta wait_duration = base::TimeTicks::Now() - start_time;
base::UmaHistogramBoolean(is_extension
? "ServiceWorker.InitiatorThread."
"ProcessReadyWaitRequired.ExtensionProcess"
: "ServiceWorker.InitiatorThread."
"ProcessReadyWaitRequired.RegularProcess",
!process_was_ready);
base::UmaHistogramTimes(is_extension
? "ServiceWorker.InitiatorThread."
"WaitTimeForProcessReady.ExtensionProcess"
: "ServiceWorker.InitiatorThread."
"WaitTimeForProcessReady.RegularProcess",
wait_duration);This is the second part, and I think we want to report this in the call site, content/renderer/service_worker/embedded_worker_instance_client_impl.cc
We may return `!process_was_ready` if we want the ProcessReadyWaitRequired.
bool g_is_instant_process = false;Is it possible to have this as optional<bool>, and have CHECK(g_is_instance_process.has_value()) in IsInstantProcess?
// TODO: http://crbug.com/434735272 — Handle other settings that are Whether a service worker thread start required waiting for the correspondingCan we also state when this is recorded?
The sentence contains `when ...`, but it's more like a condition, but we want timing information, i.e. when a new renderer process starts for SW.
<variant name="ExtensionProcess" summary="extension renderer processes"/>summary is defined, but not used?
fyi, If you embed {ProcessType} in the summary tag's document, this attribute value is expanded there. So, combined with my previous comment, you can use this like, e.g.
```
...
This metric is emitted when a {ProcessType} is created for a Service Worker.
```
Related details
- Keren Zhu
- Wangsong Jin
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
ready. Currently, service worker thread start required waiting for the
corresponding renderer process to become ready. Records true when securityShould we remove these statements. The statements below already explains that it is currently only useful for service worker startup.
Related details
- Alex Moshchuk
- Keren Zhu
- Wangsong Jin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |