Keep extension service workers out of EcoQoS on Windows [chromium/src : main]
Chris Davis (Gerrit)
Chris Davis voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Arthur Sonzogni
- Hiroki Nakagawa
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 4 comments![]( "Open in Gerrit")
Keep extension service workers out of EcoQoS on Windows
Extension service workers can be left at background process priority
(EcoQoS on Windows). Under heavy system load their render process then
starves and cannot finish top-level script evaluation before the service
worker start timeout (kStartInstalledWorkerTimeout); the worker is torn
down and retried indefinitely. For force-installed extensions with
webRequestBlocking (e.g. enterprise DLP), this can block navigation for
minutes.Q: You mention windows, but I don't see any windows specific code. Is this expected?
Both features are gated so they can be disabled via Finch if needed.The part in content adding "has_foreground_service_worker" checks isn't under the flags. Does this sentence still holds?
#if !BUILDFLAG(IS_ANDROID)Can I defer content/ review to @pmon...@chromium.org ?
(If yes, feel free to close once the review is done, and I will stamp the approved patch)
Related details
- Chris Davis
- Hiroki Nakagawa
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 3 comments![]( "Open in Gerrit")
Keep extension service workers out of EcoQoS on Windows
Extension service workers can be left at background process priority
(EcoQoS on Windows). Under heavy system load their render process then
starves and cannot finish top-level script evaluation before the service
worker start timeout (kStartInstalledWorkerTimeout); the worker is torn
down and retried indefinitely. For force-installed extensions with
webRequestBlocking (e.g. enterprise DLP), this can block navigation for
minutes.Q: You mention windows, but I don't see any windows specific code. Is this expected?
The changes are not locked down to just Windows but the issue with service worker extensions at lowest priority is an issue on all platforms.
Both features are gated so they can be disabled via Finch if needed.The part in content adding "has_foreground_service_worker" checks isn't under the flags. Does this sentence still holds?
This refers to kExtensionServiceWorkerVoter and kServiceWorkerForegroundOnExtensionStartup. The addition of the has_foreground_service_worker checks resolves a bug where this was not done in the override case.
#if !BUILDFLAG(IS_ANDROID)Can I defer content/ review to @pmon...@chromium.org ?
(If yes, feel free to close once the review is done, and I will stamp the approved patch)
- Arthur Sonzogni
- Hiroki Nakagawa
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
Keep extension service workers out of EcoQoS on Windows
Extension service workers can be left at background process priority
(EcoQoS on Windows). Under heavy system load their render process then
starves and cannot finish top-level script evaluation before the service
worker start timeout (kStartInstalledWorkerTimeout); the worker is torn
down and retried indefinitely. For force-installed extensions with
webRequestBlocking (e.g. enterprise DLP), this can block navigation for
minutes.Chris DavisQ: You mention windows, but I don't see any windows specific code. Is this expected?
The changes are not locked down to just Windows but the issue with service worker extensions at lowest priority is an issue on all platforms.
Thanks!
#if !BUILDFLAG(IS_ANDROID)Chris DavisCan I defer content/ review to @pmon...@chromium.org ?
(If yes, feel free to close once the review is done, and I will stamp the approved patch)
That's fine.
Okay. I am keeping it open until this get reviewed. Feel free to close once this is done to give visibility back to me.
Related details
- Chris Davis
- Hiroki Nakagawa
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
- Andrea Orru
- Chris Davis
- Hiroki Nakagawa
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette voted and added 3 comments![]( "Open in Gerrit")
Votes added by Patrick Monette
| Code-Review | +1 |
3 comments
lgtm
and only covers workers that already have a WorkerNode. During the
critical kStarting window there is no worker node or vote yet, so the
render process falls to background priority and starves.This is really a major flaw of the priority system in the performance manager. Unfortunately I don't have time to fix it right away.
I'd love to have worker nodes created before they start so we could apply a priority as soon as possible.
#if !BUILDFLAG(IS_ANDROID)Chris DavisCan I defer content/ review to @pmon...@chromium.org ?
(If yes, feel free to close once the review is done, and I will stamp the approved patch)
Arthur SonzogniThat's fine.
Okay. I am keeping it open until this get reviewed. Feel free to close once this is done to give visibility back to me.
It's adding a tiny bit more tech debt but there's already some there that I need to address anyways in the future. I think this is okay.
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);We shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);We shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
This is currently blocking a large number of companies in bug 484218883.
Related details
- Andrea Orru
- Arthur Sonzogni
- Hiroki Nakagawa
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
This is currently blocking a large number of companies in bug 484218883.
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Justin LulejianThis is currently blocking a large number of companies in bug 484218883.
@rdevlin...@chromium.org FYI.
Also something to consider is that not all enterprises enable Finch so reverting this will require merging back changes to address those users.
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Justin LulejianThis is currently blocking a large number of companies in bug 484218883.
Justin Lulejian@rdevlin...@chromium.org FYI.
Also something to consider is that not all enterprises enable Finch so reverting this will require merging back changes to address those users.
After speaking with Devlin I'll retract this request since we're still a couple week away from branch cut. We should have some metrics by then to decide whether to revert this default enable.
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
+yyanagisawa@: Can you review this CL on behalf of me?
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
1 comment
This is actually for the extension service worker, I would like to delegate the decision to the extension folks. lgtm if the are fine.
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
Andrea Orru (Gerrit)
Andrea Orru voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Chris Davis
Andrea Orru (Gerrit)
Andrea Orru voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Andrea Orru (Gerrit)
Andrea Orru voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
- Alex Moshchuk
- Arthur Sonzogni
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Alex Moshchuk
- Chris Davis
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
script_url_.SchemeIs("chrome-extension")) {Hmm, this seems like a layering violation. content/ shouldn't know about the chrome-extension scheme or be checking it directly like this.
Can this check go up to the content embedder through a new ContentBrowserClient API? Ideally, the feature flags would also be declared and checked in the chrome/ layer.
Related details
- Chris Davis
- Kinuko Yasuda
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
Hmm, this seems like a layering violation. content/ shouldn't know about the chrome-extension scheme or be checking it directly like this.
Can this check go up to the content embedder through a new ContentBrowserClient API? Ideally, the feature flags would also be declared and checked in the chrome/ layer.
Done — moved the check behind a new ContentBrowserClient::ShouldServiceWorkerRequireForegroundPriorityDuringStartup() . content/ keeps only the kStarting gate; ChromeContentBrowserClient does the extensions::kExtensionScheme check and the feature flag now lives in chrome/common/chrome_features.h .
Related details
- Alex Moshchuk
- Andrea Orru
- Arthur Sonzogni
- Kinuko Yasuda
- Patrick Monette
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
CONTENT_EXPORT BASE_DECLARE_FEATURE(kServiceWorkerForegroundOnExtensionStartup);Can this be removed now, since the feature has moved to chrome/?
Related details
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Kinuko Yasuda
- Patrick Monette
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
CONTENT_EXPORT BASE_DECLARE_FEATURE(kServiceWorkerForegroundOnExtensionStartup);Can this be removed now, since the feature has moved to chrome/?
Thanks. Looks like I reintroduced it while resolving a conflict during rebase.
Related details
- Alex Moshchuk
- Andrea Orru
- Arthur Sonzogni
- Kinuko Yasuda
- Patrick Monette
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk voted Code-Review+1![]( "Open in Gerrit")
- Andrea Orru
- Arthur Sonzogni
- Chris Davis
- Kinuko Yasuda
- Patrick Monette
- Yoshisato Yanagisawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Andrea Orru voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Chris Davis (Gerrit)
Chris Davis voted Commit-Queue+2![]( "Open in Gerrit")
- Arthur Sonzogni
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Keep extension service workers out of EcoQoS on Windows
Extension service workers can be left at background process priority
(EcoQoS on Windows). Under heavy system load their render process then
starves and cannot finish top-level script evaluation before the service
worker start timeout (kStartInstalledWorkerTimeout); the worker is torn
down and retried indefinitely. For force-installed extensions with
webRequestBlocking (e.g. enterprise DLP), this can block navigation for
minutes.
ExtensionServiceWorkerVoter already votes USER_BLOCKING for extension
(chrome-extension) service worker nodes, but it was disabled by default
and only covers workers that already have a WorkerNode. During the
critical kStarting window there is no worker node or vote yet, so the
render process falls to background priority and starves.
This CL keeps extension service workers at foreground priority for both
the starting and running states, then hands priority back to the normal
signals once startup completes:
- Enable ExtensionServiceWorkerVoter by default so running
chrome-extension service workers get a USER_BLOCKING vote and no
longer drop to background/EcoQoS.
- Add a ContentBrowserClient hook,
ShouldServiceWorkerRequireForegroundPriorityDuringStartup(script_url).
content/ calls it while a worker is kStarting but stays agnostic of
the chrome-extension scheme; ChromeContentBrowserClient implements the
policy (extension scheme + feature check). This covers the gap the
voter cannot (no worker node yet during startup).
- Declare and check the feature in the chrome/ layer:
features::kServiceWorkerForegroundOnExtensionStartup now lives in
chrome/common/chrome_features.{h,cc} instead of content/common.
- Honor has_foreground_service_worker under a Performance Manager
priority override. RenderProcessPriority::is_background() and
GetProcessPriority() previously ignored the flag whenever a
priority_override was present, which silently defeated the boost.
- Re-evaluate priority when startup finishes.
EmbeddedWorkerInstance::OnStarted() now calls UpdateForegroundPriority(),
dropping the temporary startup boost once the worker reaches kRunning
(it is likewise released when the worker stops).
Both features are gated so they can be disabled via Finch if needed.
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/chrome_content_browser_client.h
- M chrome/common/chrome_features.cc
- M chrome/common/chrome_features.h
- M components/performance_manager/features.cc
- M content/browser/child_process_launcher.cc
- M content/browser/service_worker/embedded_worker_instance.cc
- M content/browser/service_worker/service_worker_version.cc
- M content/public/browser/content_browser_client.cc
- M content/public/browser/content_browser_client.h
Code-Review: +1 by Alex Moshchuk, +1 by Andrea Orru
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Justin LulejianThis is currently blocking a large number of companies in bug 484218883.
Justin Lulejian@rdevlin...@chromium.org FYI.
Justin LulejianAlso something to consider is that not all enterprises enable Finch so reverting this will require merging back changes to address those users.
After speaking with Devlin I'll retract this request since we're still a couple week away from branch cut. We should have some metrics by then to decide whether to revert this default enable.
@Justin - can you disable the experiment you are running? I think it is setting kExtensionServiceWorkerVoter disabled for some people that are trying to verify the fix in Canary.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Justin LulejianThis is currently blocking a large number of companies in bug 484218883.
Justin Lulejian@rdevlin...@chromium.org FYI.
Justin LulejianAlso something to consider is that not all enterprises enable Finch so reverting this will require merging back changes to address those users.
Chris DavisAfter speaking with Devlin I'll retract this request since we're still a couple week away from branch cut. We should have some metrics by then to decide whether to revert this default enable.
@Justin - can you disable the experiment you are running? I think it is setting kExtensionServiceWorkerVoter disabled for some people that are trying to verify the fix in Canary.
That is possible but it would severely impact some experiment data. Before doing that could the people trying to verify the fix in canary try passing `--enable-features==ExtensionServiceWorkerVoter` to `chrome` forcibly enable it?
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kExtensionServiceWorkerVoter, base::FEATURE_ENABLED_BY_DEFAULT);Chris DavisWe shouldn't enable this until https://crbug.com/493556675 is resolved. We're currently at 50% of stable. Hopefully this will be enabled by default in the next couple weeks once the analysis completes.
Justin LulejianThis is currently blocking a large number of companies in bug 484218883.
Justin Lulejian@rdevlin...@chromium.org FYI.
Justin LulejianAlso something to consider is that not all enterprises enable Finch so reverting this will require merging back changes to address those users.
Chris DavisAfter speaking with Devlin I'll retract this request since we're still a couple week away from branch cut. We should have some metrics by then to decide whether to revert this default enable.
Justin Lulejian@Justin - can you disable the experiment you are running? I think it is setting kExtensionServiceWorkerVoter disabled for some people that are trying to verify the fix in Canary.
That is possible but it would severely impact some experiment data. Before doing that could the people trying to verify the fix in canary try passing `--enable-features==ExtensionServiceWorkerVoter` to `chrome` forcibly enable it?
That is what I had them do to verify the fix.