1229795 - simplify persisted service worker registration data

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Dimi Lee [:dimi]]

I would like to work on this bug.

Comment 2

[Dimi Lee [:dimi]]

Attachment: P1. Remove scriptSpec from registration data

Comment 3

[Dimi Lee [:dimi]]

Attachment: P2. remove waitingCacheName from registartion data

Comment 4

[Andrea Marchesini [:baku]]

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

Comment 5

[Dimi Lee [:dimi]]

(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 6

[Dimi Lee [:dimi]]

Attachment: P3. Migrate service worker registrar data between version 2 and version 3

Comment 7

[Andrea Marchesini [:baku]]

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 8

[Andrea Marchesini [:baku]]

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);

Comment 9

[Dimi Lee [:dimi]]

(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 ?

Comment 10

[Dimi Lee [:dimi]]

Attachment: P3. Migrate service worker registrar data between version 2 and version 3. v2

- 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!

Comment 11

[Dimi Lee [:dimi]]

Attachment: P1. Remove scriptSpec from registration data. v2

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Dimi Lee [:dimi]]

(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.

Comment 14

[Dimi Lee [:dimi]]

Attachment: P1. Remove scriptSpec from registration data. v3

Comment 15

[Dimi Lee [:dimi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab902dde4730

Comment 16

[Dimi Lee [:dimi]]

Attachment: P2. Remove waitingCacheName from registartion data

Comment 17

[Dimi Lee [:dimi]]

Attachment: P3. Migrate service worker registrar data between version 2 and version 3. v3

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

Comment 18

[Dimi Lee [:dimi]]

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

Comment 19

[Pulsebot]

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

Comment 20

[Carsten Book [:Tomcat]]

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