549090 - gfxWindowsPlatform can be called before user prefs are loaded

Status: NEW

(Core :: Widget: Win32, defect, P3)

Description

[Rain [:rain1] (inactive)]

app-startup happens at [1], while user prefs are read at profile-do-change [2], which happens after [1], at [3]. This means that user prefs haven't been read during app-startup.

Now several addons register themselves to do stuff at app-startup, and if one of them causes e.g. gfxPlatform to be initialized (as Adblock Plus does), gfxPlatform doesn't get the user value. This probably wasn't a problem in the past, but is now one because of [4]. The net effect is that enabling Adblock Plus currently causes DirectWrite/Direct2D to be shut off.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3322
[2] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefService.cpp#167
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3356
[4] http://mxr.mozilla.org/comm-central/source/mozilla/gfx/thebes/src/gfxWindowsPlatform.cpp#137

Comment 1

[Rain [:rain1] (inactive)]

The offending line is <https://hg.adblockplus.org/adblockplus/file/99f42cf05b3c/chrome/content/elemhide.js#l30>, which causes the entire layout module to be initialized -- it's fixed in ABP trunk though.

Comment 2

[Hans Schmucker]

Stylish apparently has the same behaviour (at least it's triggering the same D2D glitch).

Comment 3

[Rain [:rain1] (inactive)]

Ugh, it's even worse for debug builds -- Adblock Plus causes gfxPlatform to be initialized during autoreg, at <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3286>.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Isn't that just a bug in the directwrite code?  It should be observing the pref, not just getting it once...

Note that you can't read prefs until profile-change, because until then you don't know what profile to read prefs from!

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> Isn't that just a bug in the directwrite code?  It should be observing the
> pref, not just getting it once...
> 
> Note that you can't read prefs until profile-change, because until then you
> don't know what profile to read prefs from!

We cannot, at the moment change font backend while running. (this will become possible in the relatively near future as fonts are refactored)

We should be able to change render mode while running, but that's simply not how the code in gfxWindowsPlatform is setup. Note this is not a DirectWrite thing, the code reading the render-mode value predates either DWrite or D2D.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, as I see it we have several options:

1)  Reset the render mode on profile change (if possible).
2)  Mark this bug invalid and tell extension to not muck with layout before
    profile change.

Frankly, I thought we were telling them #2 already...

Comment 7

[John Daggett (:jtd)]

gfxPlatform code is dependent on prefs being available, I don't think that's a simple thing to change (or necessarily desirable).

bz, is there a general mechanism for denoting/enforcing initialization order in cases like this?  And why/how are extensions loading before basic initialization occurs?

Comment 8

[Benjamin Smedberg]

There is at least one non-extension case where we show UI before loading profile preferences, which is profile migration. In this case we normally won't have a user-set value for this specific pref (because the profile is just being created).

Note that you can read from prefs at pretty much any time after XPCOM registration: it just won't give you user-set values until after we've loaded the profile.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> bz, is there a general mechanism for denoting/enforcing initialization order

Not really, no.  I suppose we could fail layout init before profile-before-change, but then profile migration wouldn't work per comment 8.

> And why/how are extensions loading before basic initialization occurs?

I have no idea.  Benjamin might.

Comment 10

[Benjamin Smedberg]

Extensions which do old-style profile roaming show UI before we load the profile, as does our own profile-migration UI and the safe-mode UI. You can't enforce that load order without breaking some important stuff!

Comment 11

[John Daggett (:jtd)]

This code is in the gfxWindowsPlatform constructor, it could easily live in the Init() method.  But if extensions need to lay out text before the platform object is initialized, lots of things aren't going to work correctly, not just DWrite.  The Init() process is relatively heavy (we initialize the font list there) so we don't want to do this twice if we want decent startup times.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think the startup time impact should be a big concern. This will only happen for users who have the pref set to a non-default value, which is probably going to be a relatively small number, *and* who do something with an extension or profile migration that triggers early gfxPlatform init.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should defer all deferable work (like the processing of fonts for CMAP construction) until user-prefs have loaded, certainly.

Comment 14

[Wladimir Palant]

Note that Adblock Plus behavior changed because of bug 542111. While I'm not going to change it back of course, I consider this a platform issue - the code should either be more robust or there should be a big warning somewhere telling you that getting stylesheet service too early is a no-no.

Comment 15

[Benjamin Smedberg]

What are the effects of delaying "processing of fonts for CMAP construction" until after user prefs have loaded? Wouldn't that break UI which needs to show before that point?

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to comment #15)
> What are the effects of delaying "processing of fonts for CMAP construction"
> until after user prefs have loaded? Wouldn't that break UI which needs to show
> before that point?

No, it shouldn't. We load all cmaps lazily (after startup) to improve font fallback performance, which is what Roc is referring to, but we also load cmap tables as needed for the individual fonts we're actually using.