User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20160204142810 Steps to reproduce: 1. Create a website with a ServiceWorker installed. 2. Go to the website and allow the ServiceWorker to install/activate 3. Close the tab 4. Go to about:serviceworkers and unregister the ServiceWorker 5. Restart firefox Actual results: The just unregistered ServiceWorker is back in about:serviceworkers and when visiting the created site, it acts as if it were never uninstalled, even if the page has since commented out the ServiceWorker register code. Expected results: The ServiceWorker should be unregistered and deleted, and not affect the page. When I restart firefox my about:serviceworkers now has a list of about 6 or 7 ServiceWorkers registered for various localhost aliases, despite the fact that I have unregistered them multiple times and have not used the aliases for some time.

Can you share the site and aw script where you see this?

Also, what Firefox version and do you multiple process (e10s) enabled?

Now that you mention it I only see this with localhost, not on the live server. The sw can be seen here: https://github.com/vincekd/note-service/blob/master/src/main/resources/META-INF/resources/sticklet.service-worker.js I'm on 45.03b. I just enabled e10s today, but I've been seeing the problem since I started working with serviceworkers a couple of weeks ago.

I have confirmed this. To reproduce: 1) Visit many different medium.com sites. 2) Open serviceworker.txt in your profile dir. 3) Note that there are duplicate entries in the text file for medium. One for each article. 4) Unregister medium.com from about:serviceworkers. 5) Reopen serviceworker.txt and note that there is one fewer entry in the file for medium. 6) Restart firefox and see that its back in about:serviceworkers because it finds the next entry in the file.

After talking with Bobby about the principal stuff and came up with this plan: 1) Convert our IPC messaging and file content to use the nsIPrincipal string serialization. 2) Bump the file schema version. 3) For files with older schemas, implement an async upgrade process that uses nsIPrincipal on the main thread to convert and dedupe data. I'm unsure if I can do anything to delete any unused Cache objects yet. The files I've looked at, though, have had multiple chrome-only caches from the ServiceWorkerScriptCache stuff. Andrea, what do you think?

That... is probably more work than we need here. The attributes are used a lot of places through service worker manager. I think we can just limit our comparison to the attributes and exclude the spec. If we make the ::ReadData() dedupe, then it should all just work without a schema change.

Untested work-in-progress. I did end up changing the schema since I don't think it makes sense to store the principal spec. Its wrong and we end up just using the scope for the principal origin in the ServiceWorkerManager anyway. Lets just use the scope for this.

Comment on attachment 8718550 [details] [diff] [review] Avoid principal specs and dedupe service worker registrar entries. r=baku Local testing looks good. I went from a serviceworker.txt with 20 or so medium.com entries to a fully de-duped file. I'll look at any automated tests we have available for this tomorrow.

Comment on attachment 8718550 [details] [diff] [review] Avoid principal specs and dedupe service worker registrar entries. r=baku Actually, there are some other places I need to fix to use the new registration compare logic. Will load another patch tomorrow morning.

I decided to just focus on de-duping in this bug. We can remove the principal spec from the file in a follow-on. Not changing the schema version makes it easier to uplift this fix to beta/aurora.

This seems to perma-fail unregister-then-register-new-script.https.html wpt test on e10s. I'll have to investigate on monday.

Comment on attachment 8718880 [details] [diff] [review] Dedupe service worker registrar entries. r=baku Review of attachment 8718880 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +142,5 @@ > } > > +namespace { > + > +bool Equivalent(const ServiceWorkerRegistrationData& left, aLeft, aRight. @@ +407,5 @@ > } > > stream->Close(); > > + // Dedupe data in file. Old profiles had many duplicates. In theory right. file a follow up. Can we change the version number to avoid all of this?

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c173bd383e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/363bd6ab3411

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku Approval Request Comment [Feature/regressing bug #]: Service workers [User impact if declined]: We are currently storing duplicate entries for service workers on the same site. This can lead to strange, intermittent behavior for the user. For example, its possible for service workers to suddenly reappear after reboot or conceivably to use an old version of the service worker script. We should remove these duplicate entries as soon as possible. [Describe test coverage new/current, TreeHerder]: P2 includes a gtest verifying the de-duplication behavior. [Risks and why]: Low. Only effects service workers. [String/UUID change made/needed]: None.

Comment on attachment 8719007 [details] [diff] [review] P2 Verify entries are deduped from the ServiceWorkerRegistrar. r=baku See comment 16.

https://hg.mozilla.org/mozilla-central/rev/1c173bd383e7 https://hg.mozilla.org/mozilla-central/rev/363bd6ab3411

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku I want to do some more testing before uplifting this.

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku See comment 16.

Comment on attachment 8719007 [details] [diff] [review] P2 Verify entries are deduped from the ServiceWorkerRegistrar. r=baku See comment 16.

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku Unregistering service workers is not clearing out things as expected, seems like a basic functionality we should fix. Let's uplift to Aurora46.

Comment on attachment 8719007 [details] [diff] [review] P2 Verify entries are deduped from the ServiceWorkerRegistrar. r=baku Automated tests only, Aurora46+

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d7d82ab84d5 https://hg.mozilla.org/releases/mozilla-aurora/rev/4f415f781d39

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku Improve the feature, taking it. Should be in 45 beta 7

Comment on attachment 8719007 [details] [diff] [review] P2 Verify entries are deduped from the ServiceWorkerRegistrar. r=baku More tests, many thanks

This is hitting conflicts uplifting to beta, likely from the lack of Bug 1229795. Can we get a rebased patch (or get that bug uplifted)?

This actually introduced a regression. Please wait to uplift further.

Fix that regression P1 introduced.

Comment on attachment 8721011 [details] [diff] [review] P3 Fix service worker registry value update. r=bz r=me

Comment on attachment 8721011 [details] [diff] [review] P3 Fix service worker registry value update. r=bz This needs to be included with the uplift of the rest of this bug. P1 introduced a regression by not including this line. At a minimum we need to uplift this to aurora since P1 has already landed there. Or we need to back this whole bug out of aurora.

Comment on attachment 8721011 [details] [diff] [review] P3 Fix service worker registry value update. r=bz Added line to fix regression introduced by patch 1. Please uplift to aurora.

Thank you for the quick approval! Landed in aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/d064e8125eb5

Given this regression I'm somewhat inclined to just let this ride the trains in FF46 instead of uplifting to beta.

For reference, the issue requiring the P3 fix was that we would end up storing entries in the serviceworkers.txt registry file without a cache name, worker script, etc. The only reliably populated value was the scope. I verified locally that after the P3 fix these entries are corrected upon the next visit to the service worker enabled site in question. So I believe the problem should self correct itself without any particular disk fixup code.

Vincekd, could you please verify this issue is fixed as expected on Nightly 2/19 build? Part 3 of the fix will be included in tonight's Nightly and should be ready for you to test in the next 24 hours. Thank you so much for your help!

Ritu has suggested we try to get this into 45b8 which tags next Tuesday. I'll work on a rebase and we can land Monday if nightly/aurora still look good.

Note, the P3 patch missed landing in the 2/19 nightly build.

I have verified that these rebased P1 and P2 patches, with the P3 patch, do indeed work in a local beta build.

Sylvestre, Ritu, I'm on PTO next week. If you decide to proceed with the beta uplift please make sure the rebased P1, rebased P2, and existing P3 are all uplifted. I will respond to email next week, but probably with some delay. Thanks!

I've confirmed the P3 fix is in "47.0a1 (2016-02-20)" and it is working correctly.

Comment on attachment 8719554 [details] [diff] [review] P1 Dedupe service worker registrar entries. r=baku I am sorry but this is too late in the cycle. 45 will ship with this bug. We can discuss later if we want to fix in esr 45.1.0

Sounds good. We are disabling service workers in 45esr, so that should not be needed. Thanks!

Hi Ehsan, Baku: Do you think this is a bug worth fixing in Fx45? We will gtb 45.0 beta10 on Thursday. As such, we are pretty much in the end game and enter RC next week, so the room for error is pretty low. Do you think the benefits here outweigh the risk involved and we should take it to Beta45? Or let it ride 46 train? Thanks!

Given that we're so late in the cycle, and that Ben seemed to be OK with this not being fixed in 45 (in comment 47) and that the patches, while straightforward, are not exactly trivial, I'm inclined to say that we should not uplift this in 45 at this point, and let it ride the train in 46.

(In reply to :Ehsan Akhgari from comment #49) > Given that we're so late in the cycle, and that Ben seemed to be OK with > this not being fixed in 45 (in comment 47) and that the patches, while > straightforward, are not exactly trivial, I'm inclined to say that we should > not uplift this in 45 at this point, and let it ride the train in 46. Thanks Ehsan! I trust your judgement here.