1361500 - UTCToLocalStandardOffsetSeconds blocks startup with main thread IO on Windows (calling _tzset)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Florian Quèze [:florian]]

See this profile: https://perf-html.io/public/d9b0b7ac9efe483342b830c073ee337170203b7b/calltree/?callTreeFilters=prefix-01DmEr&range=0.0000_6.8777&thread=0

Comment 1

[(no longer active)]

Bug 1330890 is adding a resist fingerprinting service which wants to call _tzset().  It seems like that would be a nice place to centralize the logic of calling _tzset off the main thread on Windows.

Comment 2

[Doug Thayer [:dthayer] (he/him)]

Should this even be happening in the chrome process if e10s is enabled? Is there a purpose to this other than masking a user's timezone for JS running in the content process?

Comment 3

[(no longer active)]

That is a great question!  Arthur?

Comment 4

[Arthur Edelstein [:arthur]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> That is a great question!  Arthur?

I think Doug is right -- for anti-fingerprinting purposes, we only need to mask the true timezone in the content process.

Comment 5

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1361500 - Don't call _tzset on startup

The reason we call _tzset inside DateTime.cpp is to allow the
privacy.resistFingerprinting pref to mask our timezone by setting
the TZ environment variable. Without _tzset, the changes to the
environment variable won't actually change anything. However, if
a process is started with the TZ environment variable set to
something (like "UTC"), then those changes will be active in
that process. Since we're only masking timezone to JS running in
the content process, and since those content processes will be
started by the parent process which has already set its TZ to UTC,
and will copy that variable to its children, we only need to call
_tzset() when the pref changes, and only in the content process,
provided we are on e10s.

Review commit: https://reviewboard.mozilla.org/r/152216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152216/

Comment 6

[(no longer active)]

Comment on attachment 8880888 [details]
Bug 1361500 - Don't call _tzset on startup

https://reviewboard.mozilla.org/r/152216/#review157676

Very nice!

Comment 7

[Arthur Edelstein [:arthur]]

Comment on attachment 8880888 [details]
Bug 1361500 - Don't call _tzset on startup

https://reviewboard.mozilla.org/r/152216/#review158454

Looks good to me too. Thanks!

Comment 8

[Doug Thayer [:dthayer] (he/him)]

Quick update on this: it's causing non-e10s tests to fail right now, because it checks if we're e10s before we've actually loaded the profile's prefs.js, which is where the e10s defaults are overridden. This causes us to cache the wrong e10s setting and seems to break Firefox in a bunch of random ways. In any case, Arthur, are there specific requirements for when nsRFPService::Init needs to be called? For my change to work, it needs to be called before the first content process is created, but are there other reasons why it's called specifically where it's called, or does it just need to happen some time during startup?

Comment 9

[Arthur Edelstein [:arthur]]

Tim, do you have any advice for Doug? Thanks.

Comment 10

[Tim Huang[:timhuang]]

The main purpose of nsRFPService::Init() is to add the observer for 'privacy.resistFingerprinting', cache its value and update TZ value accordingly. So, this initialization should happen before any fingerprinting resistance checks for making sure every check gets a correct value. 

Most of fingerprinting resistance checks are triggered by the content, so init it before the first content process will do for them. But, there are two exceptions, the timezone setting and the window size rounding. For timezone setting, it looks good to me for you to move the initialization. For the window size rounding, if the timing of the first content process is after the creation of chrome window, and I suppose it is, then I think it will also be good.

Comment 11

[Doug Thayer [:dthayer] (he/him)]

Attachment: Bug 1361500 - (2) Move e10s check after profile load

Since user-provided values for the e10s prefs won't be loaded
until the profile's prefs.js file is loaded, we need to wait for
this to occur. The simplest way to do this while keeping the
logic neatly inside nsRFPService seemed to be to add an observer,
but another option would be to just directly call _tzset at the
right time during start-up, or add a static method in nsRFPService
doing this that we could call. This would avoid the overhead of an
observer, but since I don't know how significant that overhead
is I just went with the observer for now.

Review commit: https://reviewboard.mozilla.org/r/155032/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155032/

Comment 12

[Doug Thayer [:dthayer] (he/him)]

Comment on attachment 8880888 [details]
Bug 1361500 - Don't call _tzset on startup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152216/diff/1-2/

Comment 13

[(no longer active)]

Comment on attachment 8884106 [details]
Bug 1361500 - (2) Move e10s check after profile load

https://reviewboard.mozilla.org/r/155032/#review160104

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dbe15749c1b
Don't call _tzset on startup r=arthuredelstein,Ehsan
https://hg.mozilla.org/integration/autoland/rev/dd8944d6956b
(2) Move e10s check after profile load r=Ehsan

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/2dbe15749c1b
https://hg.mozilla.org/mozilla-central/rev/dd8944d6956b