simplify persisted service worker registration data

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: dimi)

Tracking

(Blocks 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

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
Assignee

Comment 1

4 years ago
I would like to work on this bug.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Thanks for doing this! But remember that we have to support migration between version 2 and version 3.
Assignee

Comment 5

4 years ago
(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
Assignee

Updated

4 years ago
Attachment #8706243 - Flags: review?(amarchesini)
Assignee

Updated

4 years ago
Attachment #8706247 - Flags: review?(amarchesini)
Assignee

Updated

4 years ago
Attachment #8706723 - Flags: review?(amarchesini)
Attachment #8706243 - Flags: review?(amarchesini) → review+
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
Attachment #8706247 - Flags: review?(amarchesini) → review+
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);
Attachment #8706723 - Flags: review?(amarchesini) → review-
Assignee

Comment 9

4 years ago
(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 ?
Assignee

Comment 10

4 years ago
- 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!
Attachment #8706723 - Attachment is obsolete: true
Attachment #8707338 - Flags: review?(amarchesini)
Attachment #8707338 - Flags: review?(amarchesini) → review+
Assignee

Comment 11

4 years ago
Hi Ben,
I just notice Bug 1230030 you have fixed, could you help review this patch because |LoadRegistration| changes slightly? thanks!
Attachment #8706243 - Attachment is obsolete: true
Attachment #8707740 - Flags: review?(bkelly)
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.
Attachment #8707740 - Flags: review?(bkelly) → review-
Assignee

Comment 13

4 years ago
(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.
Assignee

Comment 14

4 years ago
Attachment #8707740 - Attachment is obsolete: true
Attachment #8708165 - Flags: review?(bkelly)
Attachment #8708165 - Flags: review?(bkelly) → review+
Assignee

Comment 16

4 years ago
Attachment #8706247 - Attachment is obsolete: true
Attachment #8709757 - Flags: review+
Assignee

Comment 17

4 years ago
Fix a try error(FILE_ACCESS_DEINIED) on windows platform.
Call stream->Close before writing
Attachment #8707338 - Attachment is obsolete: true
Attachment #8709759 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed

Comment 20

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/daa93db5442d
https://hg.mozilla.org/mozilla-central/rev/f10e63ff13d9
https://hg.mozilla.org/mozilla-central/rev/ae752b02da74
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.