1343677 - Consider adding asserts in nsCSSProps::IsEnabled that we've received the prefs from the parent process

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

We want to catch it if we try to parse some pref-dependent CSS before we know our pref values.

Is it possible to detect, in a debug build, whether we've received the initial pref data in the content process?

Comment 1

[Boris Zbarsky [:bzbarsky]]

To be clear, the idea here is to catch cases when prefs we did not add in dom/ipc/ContentPrefs.cpp are accessed too early from the style system.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, we can detect in DEBUG builds whether prefs have been received yet:
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/modules/libpref/prefapi.cpp#738

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK. So we could add a Preferences::GetInitPhase().

So I guess we can ContentPrefs::GetContentPrefs, check whether our pref is in the list, and if not assert that the phase is END_ALL_PREFS or so?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I think that should work.

I don't really understand the UA stylesheet issue though. Is there a way that we could work around this so that we don't have to send these prefs on the command line? Could we initialize the UA stylesheet stuff later?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Could we initialize the UA stylesheet stuff later?

It's really not clear to me that we don't already do that.  We initialize it the first time we create a document that needs styling.  That _ought_ to be pretty late!  I just don't know for sure.

Once we have asserts for this stuff, we should try removing it from the list and seeing whether they trigger.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I guess there's also the question of how to make this play with stylo and its pref setup.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, I guess stylo just calls nsCSSProps::IsEnabled.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I tried doing the obvious thing and asserting that we're at the END_ALL_PREFS phase when IsEnabled() is called.

This fails to start up, and it has nothing to do with content processes, even.  In the chrome process, the "addons-startup" notification is dispatched, WebCompatReporter.jsm runs, gets the stylesheet service, and does preloadSheet, passing "chrome://webcompat-reporter/skin/lightbulb.css" as the URI.  This is all happening quite early, before we've loaded prefs even in the parent process, I guess.

The good news is presumably that it doesn't use any pref-controlled properties in that sheet.  The bad news is that this makes it a bit more complicated to figure out what the right thing to assert is.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  I guess we never actually change the pref phase in the parent process.  So maybe I need to restrict the assert to the content process...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to get the current preferences init phase in debug builds

MozReview-Commit-ID: JtHhLuFDu2d

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Assert in nsCSSprops::IsEnabled that we have received all preferences

This ensures that we don't read incorrect values out of the gPropertyEnabled
array simply because we haven't gotten preference values from the parent process
yet.

MozReview-Commit-ID: 59AgN3ecXQl

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove all the CSS bits from the ContentPrefs whitelist

We're now asserting that we never check these before the END_ALL_PREFS phase,
which means they don't need to be sent to the content process synchronously.

MozReview-Commit-ID: 4BGbvVCjDWz

Comment 13

[Boris Zbarsky [:bzbarsky]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=050b5c458e389e926ae3b463fa13774a634bda74

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8847930 [details] [diff] [review]
part 2.  Assert in nsCSSprops::IsEnabled that we have received all preferences

Review of attachment 8847930 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me. r=me

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8847931 [details] [diff] [review]
part 3.  Remove all the CSS bits from the ContentPrefs whitelist

Review of attachment 8847931 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me, if billm's OK with it & if the assertion doesn't fire.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8847930 [details] [diff] [review]
part 2.  Assert in nsCSSprops::IsEnabled that we have received all preferences

Review of attachment 8847930 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSProps.h
@@ +635,5 @@
>                 "out of range");
> +    // We don't have useful pref init phases in the parent process.  But in the
> +    // child process, assert that we're not trying to parse stylesheets before
> +    // we've gotten all our prefs.
> +    MOZ_ASSERT(XRE_IsParentProcess() ||

MOZ_ASSERT_IF(XRE_IsContentProcess(), ...) is a little nicer IMO, but it looks like it doesn't allow the string to be passed in :-(. I guess it's fine as-is.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8847931 [details] [diff] [review]
part 3.  Remove all the CSS bits from the ContentPrefs whitelist

Review of attachment 8847931 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this Boris.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ee07a916b8
part 1.  Add a way to get the current preferences init phase in debug builds.  r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b773c15593
part 2.  Assert in nsCSSprops::IsEnabled that we have received all preferences.  r=dholbert,billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/008b180abd4a
part 3.  Remove all the CSS bits from the ContentPrefs whitelist.  r=billm,dholbert

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55ee07a916b8
https://hg.mozilla.org/mozilla-central/rev/d6b773c15593
https://hg.mozilla.org/mozilla-central/rev/008b180abd4a