Currently we persist this information in our SW registrations: struct ServiceWorkerRegistrationData { nsCString scope; nsCString scriptSpec; nsCString currentWorkerURL; nsString activeCacheName; nsString waitingCacheName; PrincipalInfo principal; }; There are a few things wrong with this: 1) The registration should not have a scriptSpec. Only service workers themselves have a script and the transient spec being registered should not be persisted to disk. See bug 1227015. 2) There is no need to start the cache name for both the active and waiting workers. Once a worker becomes waiting we can discard the persisted bits for the active worker. Closing the browser is like the active worker being stopped which allows the waiting one to be promoted. This is what chrome does as described in [1]. I'm marking this as compat as I think it might have some subtle interactions on startup, etc. [1]: https://github.com/slightlyoff/ServiceWorker/issues/794#issuecomment-161209599

I would like to work on this bug.

Thanks for doing this! But remember that we have to support migration between version 2 and version 3.

(In reply to Andrea Marchesini (:baku) from comment #4) > Thanks for doing this! But remember that we have to support migration > between version 2 and version 3. Thanks for reminding, i will add migration code in next patch

Comment on attachment 8706247 [details] [diff] [review] P2. remove waitingCacheName from registartion data Review of attachment 8706247 [details] [diff] [review]: ----------------------------------------------------------------- What about about:serviceworkers ? I guess you have to update that too. separate patch? ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +560,5 @@ > > buffer.Append(data[i].currentWorkerURL()); > buffer.Append('\n'); > > buffer.Append(NS_ConvertUTF16toUTF8(data[i].activeCacheName())); rename this as cacheName ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh @@ +13,5 @@ > { > nsCString scope; > nsCString currentWorkerURL; > > nsString activeCacheName; cacheName

Comment on attachment 8706723 [details] [diff] [review] P3. Migrate service worker registrar data between version 2 and version 3 Review of attachment 8706723 [details] [diff] [review]: ----------------------------------------------------------------- r- for the console log. The rest of the patch looks good. ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +309,5 @@ > return rv; > } > > // The file is corrupted ? > // FIXME: in case we implement a version 2, we should inform the user using Remove this comment or update it. @@ +312,5 @@ > // The file is corrupted ? > // FIXME: in case we implement a version 2, we should inform the user using > // the console about this issue. > + if (!IsSupportedVersion(version)) { > + NS_WARNING(nsPrintfCString("Unsupported service worker registrar version: %s", We should write this in the console log. @@ +375,5 @@ > + GET_LINE(cacheName); > + CopyUTF8toUTF16(cacheName, entry->activeCacheName()); > + > + // waitingCacheName is no more used in latest version. > + GET_LINE(unused); At this point you should rewrite the file again in order to get rid of version 2. @@ +533,5 @@ > +bool > +ServiceWorkerRegistrar::IsSupportedVersion(const nsACString& aVersion) const > +{ > + uint32_t numVersions = > + sizeof(gSupportedRegistrarVersions) / sizeof(gSupportedRegistrarVersions[0]); ArrayLength(gSupportedRegistrarVersions);

(In reply to Andrea Marchesini (:baku) from comment #7) > Comment on attachment 8706247 [details] [diff] [review] > P2. remove waitingCacheName from registartion data > > Review of attachment 8706247 [details] [diff] [review]: > ----------------------------------------------------------------- > > What about about:serviceworkers ? I guess you have to update that too. > separate patch? > I think about:serviceworkers do not have to update because it use .activeWorker.cacheName .waitingWorker.cacheName remove waitingCacheName actually doesn't change anything for about:serviceworkers Do you agree with this ?

- Address Comment 8 - Use console log instead of NS_WAARNING - Re-write registrar file after migration Hi Andrea, Could you help also check re-write part? Thanks!

Hi Ben, I just notice Bug 1230030 you have fixed, could you help review this patch because |LoadRegistration| changes slightly? thanks!

Comment on attachment 8707740 [details] [diff] [review] P1. Remove scriptSpec from registration data. v2 Review of attachment 8707740 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +2919,5 @@ > } else { > RefPtr<ServiceWorkerInfo> newest = registration->Newest(); > // If the script spec matches and our active worker state matches our > // expectations for a "current worker", then we are done. > + if (newest && newest->ScriptSpec() == aRegistration.currentWorkerURL() && I don't think this is correct. The newest worker may still be installing, so doesn't reflect the currentWorkerURL(). I would change this check to compare the active worker's script spec (if there is one) to the currentWorkerURL(). If they are the same, then do the short-circuit return. This will need to be checked against all our tests, though. Maybe with a few retriggers each in e10s mode. Hopefully this part of LoadRegistration() will go away soon once we move the SWM to the parent process. Its kind of terrible to kill the SW mid-work here since we might lose a bunch of events. Thankfully this should never happen on non-e10s or e10s with only 1 content process. ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh @@ +15,4 @@ > nsCString currentWorkerURL; > > nsString activeCacheName; > nsString waitingCacheName; Why do we need both of these cache names? It seems we should only need one given the same logic for only having one script spec. If we are saving one of the cache names for purging old caches after reboot or something, I think I would prefer to be explicit and calling purgingCacheName.

(In reply to Ben Kelly [:bkelly] from comment #12) > > I don't think this is correct. The newest worker may still be installing, > so doesn't reflect the currentWorkerURL(). Yes, you are right. I didn't think |newest| clearly, will update in next patch. > > ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh > @@ +15,4 @@ > > nsCString currentWorkerURL; > > > > nsString activeCacheName; > > nsString waitingCacheName; > > Why do we need both of these cache names? It seems we should only need one > given the same logic for only having one script spec. This is in patch part2, using |cacheName| suggested by baku.

Fix a try error(FILE_ACCESS_DEINIED) on windows platform. Call stream->Close before writing

Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b1f92b1c00

https://hg.mozilla.org/integration/mozilla-inbound/rev/daa93db5442d https://hg.mozilla.org/integration/mozilla-inbound/rev/f10e63ff13d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ae752b02da74

https://hg.mozilla.org/mozilla-central/rev/daa93db5442d https://hg.mozilla.org/mozilla-central/rev/f10e63ff13d9 https://hg.mozilla.org/mozilla-central/rev/ae752b02da74