[e10s] With e10s enabled and one or more service workers registered, AMO doesn't recognize browser as Firefox

VERIFIED FIXED in Firefox 49

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ehoogeveen, Assigned: baku)

Tracking

({regression})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49+ verified, firefox50 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted file serviceworker.txt
I noticed today that I couldn't use AMO (addons.mozilla.org). On the page for any given extension, instead of getting the "Add to Firefox" button, I get a button saying "Only with Firefox — Get Firefox Now!" which links to the latest version of Firefox.

After some troubleshooting, I realized that this only happened with e10s enabled, and only in my main profile. After asking around, it turned out that another user had experienced the same problem [1], and they (somehow) worked out that "serviceworker.txt" in their profile was responsible!

I'm not sure if this is the right component - maybe this is a problem with AMO. I chose this one because the problem only happens when e10s is enabled.

Steps to reproduce:
1) Create a fresh profile.
2) Add the attached "serviceworker.txt" to the profile (the actual contents don't seem to matter, so long as there is at least one service worker in there).
3) Ensure that e10s is enabled.
4) Visit an AMO page like [2].

Expected result:
AMO should offer to add the extension to Firefox.

Actual result:
AMO does not appear to recognize that the browser being used is Firefox.

Further information:
I'm running the latest 64-bit Nightly on Windows 10 with accessibility disabled so that e10s will work. I tried setting security.sandbox.content.level to 0 in case this was a problem with permissions, but it didn't help. Removing all the service workers using about:serviceworkers does not delete the file (serviceworker.txt), but does fix the problem.

[1] http://forums.mozillazine.org/viewtopic.php?p=14652557#p14652557
[2] https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
As I was writing this, user 'bastim' reported bug 1286125 about the problem with some additional information:

"After upgrading Firefox (a1 channel, 'nightly') to the 2016-07-11 build, the navigator.userAgent property only reported the following string when using an existing profile w/ serviceworker.txt:

"Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 /"

Using a new profile (or removing serviceworker.txt) causes the proper user agent to be reported to JS."

I didn't consider that this might be a recent regression (I don't really go to AMO that often), but I can confirm that the 2016-07-11 Nightly is the first Nightly that shows the problem.
Thanks arai!
Assignee: nobody → amarchesini
Posted patch bug.patch (obsolete) — Splinter Review
The problem here is that SWM->LoadRegistrations() calls (not directly) nsHttpHandler::Init(). nsHttphandler creates the userAgent strings but, at this point ContentChild has not received yet the userAgent from the parent. That will happen in RecvAppInfo().

My solution is to move SWM LoadingRegistrations() in RecvAppInit().

Another approach would be to 'extend' AppInfo() to pass also the SW registrations.
Attachment #8770202 - Flags: review?(gkrizsanits)
Duplicate of this bug: 1286015
Duplicate of this bug: 1286234
Posted patch bug.patchSplinter Review
Better approach, without sync call.
Attachment #8770202 - Attachment is obsolete: true
Attachment #8770202 - Flags: review?(gkrizsanits)
Attachment #8770208 - Flags: review?(gkrizsanits)
Would it be helpful to create a tryserver with this patch?
Comment on attachment 8770208 [details] [diff] [review]
bug.patch

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

::: dom/ipc/PContent.ipdl
@@ +528,5 @@
>  
> +    /**
> +     * Send ServiceWorkerRegistrationData to child process.
> +     */
> +    async GetServiceWorkerConfiguration(ServiceWorkerConfiguration aConfig);

I think this is no longer a GetServiceWorkerConfiguration but more like a SetServiceWorkerConfiguration / InitServiceWorker or something similar.
Attachment #8770208 - Flags: review?(gkrizsanits) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07e8b98deca
Move ServiceWorker initialization early in the ContentChild startup, r=gabor
https://hg.mozilla.org/mozilla-central/rev/e07e8b98deca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Tracking and marking affected for 49.
This should uplift along with the patches from bug 1279503. baku can you request uplift here too?
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1286558
Comment on attachment 8770208 [details] [diff] [review]
bug.patch

Approval Request Comment
[Feature/regressing bug #]: ServiceWorkers
[User impact if declined]: content process doesn't have the correct serviceWorker entries, sometimes.
[Describe test coverage new/current, TreeHerder]: no tests. Only manual.
[Risks and why]: it's stable in nightly... and it fixes a regression.
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8770208 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1286616
Comment on attachment 8770208 [details] [diff] [review]
bug.patch

Seems crucial to fix so that users with e10s enabled can interact with AMO. Let's try this on aurora. We should verify the fix.
Flags: needinfo?(andrei.vaida)
Attachment #8770208 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: 1286125
Duplicate of this bug: 1286125
does not apply cleanly like grafting 354060:e07e8b98deca "Bug 1286126 - Move ServiceWorker initialization early in the ContentChild startup, r=gabor"
merging dom/ipc/ContentChild.cpp
merging dom/ipc/ContentChild.h
merging dom/ipc/ContentParent.cpp
merging dom/ipc/ContentParent.h
merging dom/ipc/PContent.ipdl
warning: conflicts while merging dom/ipc/ContentChild.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/ipc/ContentParent.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/ipc/ContentParent.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/ipc/PContent.ipdl! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue') to aurora
Flags: needinfo?(amarchesini)
It depends on bug 1279503. We need those 2 patches as well.
(In reply to Andrea Marchesini (:baku) from comment #20)
> It depends on bug 1279503. We need those 2 patches as well.

liz, gchang can you take a look ?
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
I could not reproduce this bug using Firefox 50.0a1, build ID:20160711034039, on Windows 10 x64.

Can you please be more specific on step 2? ( I want to be sure I did the steps correctly)

Thank you!
Flags: needinfo?(andrei.vaida) → needinfo?(emanuel.hoogeveen)
Flags: needinfo?(amarchesini)
(In reply to Cristian Comorasu from comment #22)
> Can you please be more specific on step 2? ( I want to be sure I did the
> steps correctly)

By "Add the attached "serviceworker.txt" to the profile" I mean downloading the attached file, saving it with that name, and placing it in the top level of the profile you're testing with (the directory containing files like cookies.sqlite and places.sqlite). If you're testing with a fresh profile, you probably know where that is - if not, it should be in %AppData%\Mozilla\Firefox\Profiles\[some random name].

I haven't seen this problem since the fix landed by the way - on the latest Nightly with e10s enabled and a service worker for twitter in my profile, AMO works fine.
Flags: needinfo?(emanuel.hoogeveen)
I am sure I followed the steps accordingly, and tested it on several environments with the affected build, but could not reproduce it. 

Based on the comment #23 I will mark this bug as verified fixed.

Thank you!
Status: RESOLVED → VERIFIED
Hi :Tomcat,
I approved the patches in bug 1279503.
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Flags: needinfo?(cbook)
Depends on: 1285947
Posted patch m-aSplinter Review
has problems after the patches from bug 1279503 are applied like 

adding 1286126 to series file
renamed 1286126 -> 4.patch
applying 4.patch
patching file dom/ipc/ContentChild.cpp
Hunk #1 FAILED at 1085
1 out of 2 hunks FAILED -- saving rejects to file dom/ipc/ContentChild.cpp.rej
patching file dom/ipc/ContentParent.cpp
Hunk #2 FAILED at 5641
1 out of 2 hunks FAILED -- saving rejects to file dom/ipc/ContentParent.cpp.rej
patching file dom/ipc/ContentParent.h
Hunk #1 FAILED at 1137
1 out of 1 hunks FAILED -- saving rejects to file dom/ipc/ContentParent.h.rej
patching file dom/ipc/PContent.ipdl
Hunk #2 FAILED at 1119
1 out of 2 hunks FAILED -- saving rejects to file dom/ipc/PContent.ipdl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
Flags: needinfo?(cbook) → needinfo?(amarchesini)
Patch 1285947 is needed. I asked to have that bug uplift as well.
Flags: needinfo?(amarchesini)
Depends on: 1288232
We should verify this on Fx49 as well.
Flags: qe-verify+
Flags: needinfo?(cristian.comorasu)
Hi! This issue is no longer reproducible, I verified it on Fx 49.0b4.
Flags: qe-verify+
Flags: needinfo?(cristian.comorasu)
You need to log in before you can comment on or make changes to this bug.