1227932 - ServiceWorkerRegistration.update() shouldn't use Soft Update

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

In gecko, we use the same code path for registration.update() and Soft Update, but their behaviour has diverged in the spec.

At the moment, a sequence of .update() will be coalesced into a single job, which I think is a bug.

Also see: https://bugzilla.mozilla.org/show_bug.cgi?id=1217367#c32

Comment 1

[Cătălin Badea (:catalinb)]

Attachment: Fix Service Workers SoftUpdate and registration.update code paths.

This patch splits the code paths for registration.update and soft update
since they have different behaviour. Next, it changes ServiceWorkerRegisterJob
to use just one callback and just prevents soft update from queuing a new
task if another one is pending.

Comment 2

[Cătălin Badea (:catalinb)]

Attachment: Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

Fixed a deadlock in UpdateInternal caused by SWM:Update trying to reject
the promise synchronously when GetNewestWorker() is null.

Comment 3

[(no longer active)]

Comment on attachment 8691921 [details] [diff] [review]
Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

Review of attachment 8691921 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ -1119,5 @@
>    // Callers MUST hold a strong ref before calling this!
>    void
>    Fail(ErrorResult& aRv)
>    {
> -    AssertIsOnMainThread();

This is still main thread only, right?  Can you please add this assertion back?

@@ +1259,5 @@
>      nsCOMPtr<nsIRunnable> r =
>        NS_NewRunnableMethod(this, &ServiceWorkerRegisterJob::ContinueUpdate);
>      NS_DispatchToMainThread(r);
> +
> +    mRegistration->mUpdating = true;

Can you please add an AssertIsOnMainThread() to the beginning of this function to make sure we have such assertions for all accesses to mUpdating?

@@ +3542,5 @@
>    MOZ_ASSERT(queue);
>  
> +  // TODO(catalinb): Should we just remove this and null check in
> +  // ServiceWorkerRegisterJob?
> +  RefPtr<ServiceWorkerUpdateFinishCallback> cb = new EmptyUpdateFinishCallback();

I think we should!

Any reason not to do it here?

@@ +3548,5 @@
> +  // "If the registration queue for registration is empty, invoke Update algorithm,
> +  // or its equivalent, with client, registration as its argument."
> +  // TODO(catalinb): We don't implement the force bypass cache flag.
> +  // See: https://github.com/slightlyoff/ServiceWorker/issues/759
> +  if (!registration->mUpdating) {

Can you please add the assertion to the beginning of this function as well?

Also, I think you can move the call to GetOrCreateJobQueue inside the body of this conditional statement.

Comment 4

[Cătălin Badea (:catalinb)]

Attachment: Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

This patch splits the code paths for registration.update and soft update
since they have different behaviour. Next, it changes ServiceWorkerRegisterJob
to use just one callback and just prevents soft update from queuing a new
task if another one is pending.

Comment 5

[Cătălin Badea (:catalinb)]

Attachment: Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

nit fixes.

Comment 6

[Cătălin Badea (:catalinb)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aeed7a35b92f&selectedJob=14103637

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9a43cbbdfa

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8680266bbc0a

Comment 9

[Cătălin Badea (:catalinb)]

(In reply to Pulsebot from comment #8)
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8680266bbc0a

Is this because it didn't apply cleanly on m-c?

Comment 10

[Carsten Book [:Tomcat]]

(In reply to Cătălin Badea (:catalinb) from comment #9)
> (In reply to Pulsebot from comment #8)
> > Backout:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/8680266bbc0a
> 
> Is this because it didn't apply cleanly on m-c?

yeah this didn't work when merging m-i to m-c and caused:
warning: conflicts while merging dom/workers/ServiceWorkerManager.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/workers/ServiceWorkerManager.h! (edit, then use 'hg resolve --mark')

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/957a33a8949b

Comment 12

[Cătălin Badea (:catalinb)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c64507dfcb&selectedJob=14148485

Comment 13

[Cătălin Badea (:catalinb)]

Comment on attachment 8692096 [details] [diff] [review]
Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: ServiceWorker update() will be broken in some cases.
[Describe test coverage new/current, TreeHerder]: Covered by existing web platform tests.
[Risks and why]: Low, SW only change.
[String/UUID change made/needed]: None

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/957a33a8949b

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8692096 [details] [diff] [review]
Fix Service Workers SoftUpdate and registration.update code paths. r=ehsan

SW is planned for FF44, taking the fix in Aurora44 as it has been on Nightly for a few days now.

Comment 16

[Carsten Book [:Tomcat]]

failed during aurora uplift:

merging dom/workers/ServiceWorkerManager.cpp
merging dom/workers/ServiceWorkerManager.h
merging dom/workers/ServiceWorkerManagerChild.cpp
merging dom/workers/ServiceWorkerRegistration.cpp
warning: conflicts while merging dom/workers/ServiceWorkerManager.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/workers/ServiceWorkerManager.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 17

[Cătălin Badea (:catalinb)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f36c621ac17e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f36c621ac17e