Closed Bug 1373387 Opened 7 years ago Closed 7 years ago

Places should not be initialized by the SanityTest window

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

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.
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.
https://hg.mozilla.org/mozilla-central/rev/4d3fe42a592d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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)
(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.
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)
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.
(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. :-)
(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.

Attachment

General

Created:
Updated:
Size: