Closed Bug 1286126 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox49 + verified
firefox50 --- verified

People

(Reporter: ehoogeveen, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached file serviceworker.txt (deleted) —
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
Attached patch bug.patch (obsolete) (deleted) — 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)
Attached patch bug.patch (deleted) — Splinter 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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This should uplift along with the patches from bug 1279503. baku can you request uplift here too?
Flags: needinfo?(amarchesini)
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?
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+
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
Attached patch m-a (deleted) — Splinter 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.

Attachment

General

Created:
Updated:
Size: