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)
Core
DOM: Service Workers
Tracking
()
VERIFIED
FIXED
mozilla50
People
(Reporter: ehoogeveen, Assigned: baku)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
gkrizsanits
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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/
Reporter | ||
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
Reporter | ||
Comment 3•8 years ago
|
||
Thanks arai!
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
Better approach, without sync call.
Attachment #8770202 -
Attachment is obsolete: true
Attachment #8770202 -
Flags: review?(gkrizsanits)
Attachment #8770208 -
Flags: review?(gkrizsanits)
Comment 8•8 years ago
|
||
Would it be helpful to create a tryserver with this patch?
Comment 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e07e8b98deca
Move ServiceWorker initialization early in the ContentChild startup, r=gabor
Comment 11•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Tracking and marking affected for 49.
status-firefox49:
--- → affected
tracking-firefox49:
--- → +
This should uplift along with the patches from bug 1279503. baku can you request uplift here too?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 15•8 years ago
|
||
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+
Updated•8 years ago
|
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
It depends on bug 1279503. We need those 2 patches as well.
Comment 21•8 years ago
|
||
(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)
Comment 22•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Reporter | ||
Comment 23•8 years ago
|
||
(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)
Comment 24•8 years ago
|
||
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
Comment 25•8 years ago
|
||
Hi :Tomcat,
I approved the patches in bug 1279503.
Flags: needinfo?(lhenry)
Flags: needinfo?(gchang)
Flags: needinfo?(cbook)
Assignee | ||
Comment 26•8 years ago
|
||
Comment 27•8 years ago
|
||
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)
Assignee | ||
Comment 28•8 years ago
|
||
Patch 1285947 is needed. I asked to have that bug uplift as well.
Flags: needinfo?(amarchesini)
Comment 29•8 years ago
|
||
bugherder uplift |
Comment 30•8 years ago
|
||
We should verify this on Fx49 as well.
Flags: qe-verify+
Flags: needinfo?(cristian.comorasu)
Comment 31•8 years ago
|
||
Hi! This issue is no longer reproducible, I verified it on Fx 49.0b4.
You need to log in
before you can comment on or make changes to this bug.
Description
•