1078163 - WorkerNavigator strings should honor general.*.override prefs in e10s mode

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[Georg Koppen]

general.*.override prefs are not honored in e10s mode. Steps to reproduce:

1) Take the latest nightly on a Linux system.
2) Start it with a new profile.
3) If getting asked whether to enable e10s mode, choose "Yes".
4) After restarting open about:config and create a new pref "general.platform.override" with a value of "Win32"
5) Visit https://securehomes.esat.kuleuven.be/~gacar/dev/worker_test.html.
6) Result: The test fails as the script detects the Linux platform.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: e10s.patch

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm going to need some more explanation of what the problem was and how this fixes it.

Comment 3

[Andrea Marchesini [:baku]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> I'm going to need some more explanation of what the problem was and how this
> fixes it.

It turned out that in RuntimeService::GetOrCreateService we set gRuntimeService when:

    nsRefPtr<RuntimeService> service = new RuntimeService();
    if (NS_FAILED(service->Init())) {

Init() finishes its task. But, in the Init we call |Preferences::RegisterCallbackAndCall()| for several preferences and many of these work with static methods such as PlatformOverrideChanged, LoadRuntimeOptions, etc etc.

These methods are immediately called and many of them do:

  RuntimeService* rts = RuntimeService::GetService();

that returns null, because we are still into the Init().

The second issue is that Navigator::GetAcceptLanguages() assumes that the input array is empty.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

If you're going to change this please clean out the |gRuntimeServiceDuringInit| stuff then?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: e10s.patch

https://tbpl.mozilla.org/?tree=Try&rev=f892660d6fc2

pushed to try before landing

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c00d59b1c00

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/8c00d59b1c00

Comment 8

[Kamil Jozwiak [:kjozwiak]]

Reproduced the issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-08-03-02-02-mozilla-central/

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-08-03-02-02-mozilla-central/

Test Cases:

- ensured that "general.platform.override" is correctly being displayed when e10s is enabled
- ensured that "general.platform.override" is correctly being displayed when e10s is disabled (normal/private/e10s)
- ensured that "general.platform.override" is still being correctly displayed under about:config
- ensured that "general.platform.override" is still correctly being displayed after closing/re-opening fx 
- ensured that updating from an older fx to the latest version fixes the original problem

Went through the following OS's:

- Win 8.1 x64 - PASSED
- Ubuntu 14.0.4.1 x64 (VM) - PASSED
- OSX 10.9.5 x64 (VM) - PASSED

Should this also be uplifted into fx34 and fx33 to fix Bug #1062920? This is still happening under fx34 and fx33 (used today's builds). Once you restart the browser, you'll get:

- FAIL - navigator.platform: Win32 != IntelMac

Is the problem in fx34 and fx33 related to this? Or is this strictly for e10s?

Comment 9

[Andrea Marchesini [:baku]]

> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-08-03-02-02-
> mozilla-central/

I think this patch was not in that build yet.
What I see is that the patch landed in nightly at this at this time: Wed Oct 08 23:37:42 2014 +0000

Can I ask you to try again with the current nightly? Thanks!

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8501615 [details] [diff] [review]
e10s.patch

Approval Request Comment
[Feature/regressing bug #]: workers.
[User impact if declined]: the overridden platform, appName, etc values from the preferences are not read at startup of firefox.
[Describe test coverage new/current, TBPL]: TBPL is fully green.
[Risks and why]: none. the patch is very simple.
[String/UUID change made/needed]: none

This bug is deeply connected to bug 1062920. I would like to have it uplift to aurora and beta.

Comment 11

[Kamil Jozwiak [:kjozwiak]]

> I think this patch was not in that build yet.
> What I see is that the patch landed in nightly at this at this time: Wed Oct
> 08 23:37:42 2014 +0000
> 
> Can I ask you to try again with the current nightly? Thanks!

I accidentally pasted the same link twice, this passed and worked fine under m-c. I'll finish up testing once the uplifts are completed for fx34 and fx33

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8501615 [details] [diff] [review]
e10s.patch

Too late for beta as the release is next tuesday.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Verified as per comment #8

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> Comment on attachment 8501615 [details] [diff] [review]
> e10s.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]: workers.
> [User impact if declined]: the overridden platform, appName, etc values from
> the preferences are not read at startup of firefox.

As I understand it, this bug is an issue with e10s. As we're not going to enable e10s in Firefox 34, is there any reason to uplift this fix?

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Lawrence Mandel [:lmandel] from comment #14)
> As I understand it, this bug is an issue with e10s. As we're not going to
> enable e10s in Firefox 34, is there any reason to uplift this fix?

I'm going to answer my own question. This fix is required to fix bug 1062920.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501615 [details] [diff] [review]
e10s.patch

Aurora+

Andrea - We have no sheriff coverage this weekend. Can you handle the uplift before the merge on Monday?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Workahol, I haz it.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e96a7a4f3bbe

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Went through the verification process using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-13-00-40-02-mozilla-aurora/

I went through the same test cases from comment #8 other than testing with e10s as it's currently not available under aurora.

Marking this as verified as fx33 is marked as "wontfix".