Closed
Bug 1373387
Opened 7 years ago
Closed 7 years ago
Places should not be initialized by the SanityTest window
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file)
3.40 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
In bug 1371710 I landed my patch without the test because I couldn't figure out how to make it pass on Windows. Today I captured this profile of the first startup after updating nightly on the quantum reference hardware: https://perfht.ml/2swUyeR It gives relatively solid evidence that on Windows places is initialized before first paint due to a Msg_VisitURI message sent by a content process displaying the SanityTest window.
Assignee | ||
Comment 1•7 years ago
|
||
Try is green :-) https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ba727c248dce1d55064699c81aa5005bc86c87
Attachment #8878194 -
Flags: review?(mak77)
Comment 2•7 years ago
|
||
Comment on attachment 8878194 [details] [diff] [review] Patch Review of attachment 8878194 [details] [diff] [review]: ----------------------------------------------------------------- thanks!
Attachment #8878194 -
Flags: review?(mak77) → review+
Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3fe42a592d Places should not be initialized by the SanityTest window, r=mak.
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d3fe42a592d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 5•7 years ago
|
||
Marco, why do we need to blacklist nsAsyncShutdown.js? It should be quite cheap to load, and there are services other than places that need to initialize early and register shutdown blockers.
Flags: needinfo?(mak77)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5) > Marco, why do we need to blacklist nsAsyncShutdown.js? It should be quite > cheap to load, and there are services other than places that need to > initialize early and register shutdown blockers. We blacklisted whatever was consistently not used anymore during early startup. If you have a use case that definitely needs it before final-ui-startup, we can remove the blacklist entry.
Comment 7•7 years ago
|
||
Whatever we can blacklist is code we won't run, even if cheap. From my understanding the long term plan would be to move to a whitelist. For these "low-level" libraries, like asyncshutdown, OSFile and so on, we may likely limit to blacklisting their consumers, rather than the libraries themselves. If your fear is that people may start using the wrong shutdown methods just because AsyncShutdown is blacklisted, that may be a valid concern that could be worth undoing that. It's good you pointed out this possible change of direction, may be worth its own bug. In general I think we should not consider these blacklists carved in stone, necessities can change and we must be able to adapt.
Flags: needinfo?(mak77)
Comment 8•7 years ago
|
||
I think a whitelist probably makes sense, but I don't think that a blacklist with a small number of arbitrary entries does make sense. That tends to have the effect of discouraging people to use those modules, which is something we definitely don't want. In the particular case of nsAsyncShutdown, we already use AsyncShutdown.jsm extensively before first paint, so this seems to only discourage use from C++ consumers. I moved the ServiceWorkerRegistrar to use nsAsyncShutdown rather than its own hand-rolled event loop spinning in bug 1399646, which triggered this test failure.
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8) > I think a whitelist probably makes sense, but I don't think that a blacklist > with a small number of arbitrary entries does make sense. A whitelist for anything that gets loaded before first paint is the goal, but there's a lot of work to do before we get there. The intent with the blacklist was to prevent regressions for things we worked hard to delay. So we blacklisted anything we managed to delay, mostly so that we would know if something changes. Also, we already couldn't blacklist nsAsyncShutdown "before first paint", so blacklisting "before opening first browser window" isn't very important. We could blacklist "before profile selection"... but we have already managed to switch to a whitelist for that phase, so... I guess you can just remove the line. > I moved the ServiceWorkerRegistrar to use nsAsyncShutdown rather than its > own hand-rolled event loop spinning in bug 1399646, which triggered this > test failure. Sounds reasonable. :-)
Comment 10•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #9) > Also, we already couldn't blacklist nsAsyncShutdown "before first paint", so > blacklisting "before opening first browser window" isn't very important. We > could blacklist "before profile selection"... but we have already managed to > switch to a whitelist for that phase, so... I guess you can just remove the > line. Great. Thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•