Closed Bug 1078163 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Workers, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
e10s + ---
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified

People

(Reporter: gk, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch e10s.patch (obsolete) — Splinter Review
Attachment #8500419 - Flags: review?(khuey)
I'm going to need some more explanation of what the problem was and how this fixes it.
Flags: needinfo?(amarchesini)
See Also: → 1062920
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(amarchesini)
If you're going to change this please clean out the |gRuntimeServiceDuringInit| stuff then?
tracking-e10s: --- → +
Attached patch e10s.patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=f892660d6fc2

pushed to try before landing
Attachment #8500419 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8c00d59b1c00
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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?
Flags: needinfo?(amarchesini)
> 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!
Flags: needinfo?(amarchesini)
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.
Attachment #8501615 - Flags: approval-mozilla-beta?
Attachment #8501615 - Flags: approval-mozilla-aurora?
> 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 on attachment 8501615 [details] [diff] [review]
e10s.patch

Too late for beta as the release is next tuesday.
Attachment #8501615 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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?
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
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?
Attachment #8501615 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(amarchesini)
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".
Status: RESOLVED → VERIFIED
Depends on: 1082178
You need to log in before you can comment on or make changes to this bug.