browser/base/content/test/performance/browser_startup.js | resource://gre/modules/Sqlite.sys.mjs loaded is not allowed before first paint (due to cookie banner domain pref service)
Categories
(Toolkit :: Performance Monitoring, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox114 | --- | affected |
People
(Reporter: Gijs, Unassigned)
References
Details
(Keywords: intermittent-failure, leave-open)
Attachments
(1 file)
This showed up on pernosco when I was trying to repro something else.
|
Reporter | |
Comment 1•3 years ago
|
||
JS stack:
0 _getConnection() ["resource://gre/modules/ContentPrefService2.sys.mjs":1193:6]
1 get conn/this._connPromise<() ["resource://gre/modules/ContentPrefService2.sys.mjs":142:26]
2 get conn() ["resource://gre/modules/ContentPrefService2.sys.mjs":148:6]
3 CPS2__execStmts() ["resource://gre/modules/ContentPrefService2.sys.mjs":906:15]
4 CPS2_getByName() ["resource://gre/modules/ContentPrefService2.sys.mjs":186:9]
|
|
Reporter | |
Comment 2•3 years ago
•
|
||
Looks like this is from the cookie banner pref service runnable, with this stack:
::RunnableFunction<>::Run () at nsThreadUtils.h:548
::TaskController::TaskController()::$_0::operator() const () at TaskController.cpp:191
::TaskController::ProcessPendingMTTask () at TaskController.cpp:464
::TaskController::ExecuteNextTaskOnlyMainThreadInternal () at TaskController.cpp:742
::TaskController::DoExecuteNextTaskOnlyMainThreadInternal () at TaskController.cpp:869
::RunnableTask::Run () at TaskController.cpp:553
::RunnableFunction<>::Run () at nsThreadUtils.h:548
::nsCookieBannerService::Init()::$_0::operator() const () at nsCookieBannerService.cpp:197
::CookieBannerDomainPrefService::Init () at CookieBannerDomainPrefService.cpp:99
This runnable is set up here: https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/toolkit/components/cookiebanners/nsCookieBannerService.cpp#187-197
It's not clear to me why the dispatch to the thread queue with idle priority would be able to run before firstpaint. Florian, do you know?
Also, Jared, could we just not run this code if the cookie banner stuff is disabled (which I believe is still the default across channels) ?
|
|
Reporter | |
Comment 3•3 years ago
•
|
||
(if it's helpful, I have a pernosco recording so we can use that to dig into the idle thread queue question - I'm not sure how to, off-hand.)
|
||
Comment 4•3 years ago
|
||
(In reply to :Gijs (he/him) from comment #2)
It's not clear to me why the dispatch to the thread queue with idle priority would be able to run before firstpaint. Florian, do you know?
If there's no runnable with a priority higher than idle queued to run on the main thread, then I see no reason why idle runnables wouldn't be running. Being idle on the main thread during startup should not happen much, but I guess it could happen while the main thread is waiting for something happening asynchronously on another thread to finish.
(In reply to :Gijs (he/him) from comment #1)
JS stack:
This is the same stack as what I saw in bug 1798750 comment 18.
|
||
Comment 5•3 years ago
|
||
We've introduced the idle dispatch in Bug 1784868 to resolve startup test failures and avoid initializing the cookie banner service too early, when we don't really need it that early. Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
Init should only be called if the cookie banner service is enabled via pref, see https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/toolkit/components/cookiebanners/nsCookieBannerService.cpp#111,118
|
|
||
Comment 6•3 years ago
|
||
(In reply to Paul Zühlcke [:pbz] from comment #5)
Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
I see no simple solution in C++. If it's fine to use a bit of JS, then removing the profile-after-change observer and adding instead something around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/browser/components/BrowserGlue.sys.mjs#2874 that calls something on Services.cookieBanners could be a solution.
Do you know how late during startup you can init without causing trouble?
|
|
Reporter | |
Comment 7•3 years ago
|
||
(In reply to Paul Zühlcke [:pbz] from comment #5)
Init should only be called if the cookie banner service is enabled via pref, see https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/toolkit/components/cookiebanners/nsCookieBannerService.cpp#111,118
The cookie banner service is enabled for PB mode on nightly, which is presumably why we're hitting this. I guess in that case we could delay initialization until the first private context is opened, but that would just mask the problem here until we start enabling outside of PB.
(In reply to Florian Quèze [:florian] from comment #6)
(In reply to Paul Zühlcke [:pbz] from comment #5)
Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
I see no simple solution in C++. If it's fine to use a bit of JS, then removing the profile-after-change observer and adding instead something around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/browser/components/BrowserGlue.sys.mjs#2874 that calls something on Services.cookieBanners could be a solution.
Do you know how late during startup you can init without causing trouble?
--> Paul ? :-)
|
|
||
Comment 8•3 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #6)
(In reply to Paul Zühlcke [:pbz] from comment #5)
Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
I see no simple solution in C++. If it's fine to use a bit of JS, then removing the profile-after-change observer and adding instead something around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/browser/components/BrowserGlue.sys.mjs#2874 that calls something on Services.cookieBanners could be a solution.
BrowserGlue would work, however the feature is not limited to Firefox Desktop so we need to init on platform level somewhere.
Do you know how late during startup you can init without causing trouble?
Fairly late, we would want the service to init before the first top level HTTP request to a website for a browser tab. That's so we can inject a cookie to suppress / handle a banner if needed. Though blocking the request on the init call is probably a bad idea for performance reasons. So we either init a bit earlier or we accept that we can't handle banners for tabs loading very early.
(In reply to :Gijs (he/him) from comment #7)
(In reply to Paul Zühlcke [:pbz] from comment #5)
Init should only be called if the cookie banner service is enabled via pref, see https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/toolkit/components/cookiebanners/nsCookieBannerService.cpp#111,118
The cookie banner service is enabled for PB mode on nightly, which is presumably why we're hitting this. I guess in that case we could delay initialization until the first private context is opened, but that would just mask the problem here until we start enabling outside of PB.
(In reply to Florian Quèze [:florian] from comment #6)
(In reply to Paul Zühlcke [:pbz] from comment #5)
Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
I see no simple solution in C++. If it's fine to use a bit of JS, then removing the profile-after-change observer and adding instead something around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/browser/components/BrowserGlue.sys.mjs#2874 that calls something on Services.cookieBanners could be a solution.
Do you know how late during startup you can init without causing trouble?
--> Paul ? :-)
Agreed, waiting for the first PBM context doesn't really help much, especially since the feature is toggled on for both windows via the preferences checkbox.
|
||
Updated•3 years ago
|
|
|
||
Comment 9•3 years ago
|
||
(In reply to Paul Zühlcke [:pbz] from comment #8)
Do you know how late during startup you can init without causing trouble?
Fairly late, we would want the service to init before the first top level HTTP request to a website for a browser tab.
Maybe find a place in the initialization of networking code and trigger an idle runnable from here? Really not sure.
|
|
||
Comment 10•3 years ago
|
||
Hmm, that could be an option too. Alternatively we can also accept that if the user has this feature enabled it will have a small impact on performance. Maybe I'm understanding this wrong, but is it really that problematic if we only run the task on idle anyway?
|
|
||
Comment 11•2 years ago
|
||
(In reply to Paul Zühlcke [:pbz] from comment #10)
is it really that problematic if we only run the task on idle anyway?
One reason for being idle during startup could be that we are blocked on waiting for some async I/O to complete, so adding more (blocking) I/O during that time might have an actual impact. I don't know for sure.
|
|
||
Updated•2 years ago
|
|
|
||
Comment 12•2 years ago
|
||
|
||
Comment 13•2 years ago
|
||
|
|
||
Comment 14•2 years ago
|
||
I'll take a look, but I can't commit to a specific timeline. Thanks for adding the allowlist entry!
|
||
Comment 15•2 years ago
|
||
| bugherder | ||
|
|
||
Comment 16•2 years ago
|
||
(In reply to Florian Quèze [:florian] from comment #6)
(In reply to Paul Zühlcke [:pbz] from comment #5)
Do have suggestions for alternative approaches to deferring init that would not lead to these intermittent test failures?
I see no simple solution in C++. If it's fine to use a bit of JS, then removing the profile-after-change observer and adding instead something around https://searchfox.org/mozilla-central/rev/31f5847a4494b3646edabbdd7ea39cb88509afe2/browser/components/BrowserGlue.sys.mjs#2874 that calls something on Services.cookieBanners could be a solution.
Do you know how late during startup you can init without causing trouble?
Looks like moving the init to BrowserGlue still causes the test to fail: https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a32e4eb732c15939416e1375feccff976f896d95
Florian, do you have suggestions on how I could troubleshoot this? Would probably good to rule out that there is no other component triggering this call now that the check has been commented out for a while.
|
|
||
Comment 17•2 years ago
|
||
(In reply to Paul Zühlcke [:pbz] from comment #16)
Florian, do you have suggestions on how I could troubleshoot this? Would probably good to rule out that there is no other component triggering this call now that the check has been commented out for a while.
I tried to reproduce the failures with the profiler (and then with lower overhead settings), and couldn't. I then pushed to try your patches without enabling the profiler, and I still couldn't reproduce.
So either something has changed on mozilla-central since when you tried, or you were very unlucky in your try run, or... I have no idea what's happening.
The failure I did reproduce is the one that is reported in bug 1798750.
|
|
||
Comment 18•1 year ago
|
||
Unassigning myself because the project is currently on hold.
| Comment hidden (Intermittent Failures Robot) |
| Comment hidden (Intermittent Failures Robot) |
| Comment hidden (Intermittent Failures Robot) |
| Comment hidden (Intermittent Failures Robot) |
| Comment hidden (Intermittent Failures Robot) |
| Comment hidden (Intermittent Failures Robot) |
Description
•