Closed Bug 1227932 Opened 4 years ago Closed 4 years ago

ServiceWorkerRegistration.update() shouldn't use Soft Update

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: catalinb, Assigned: catalinb)

References

Details

Attachments

(1 file, 3 obsolete files)

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
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.
Attachment #8691915 - Flags: review?(ehsan)
Fixed a deadlock in UpdateInternal caused by SWM:Update trying to reject
the promise synchronously when GetNewestWorker() is null.
Attachment #8691915 - Attachment is obsolete: true
Attachment #8691915 - Flags: review?(ehsan)
Attachment #8691921 - Flags: review?(ehsan)
Assignee: nobody → catalin.badea392
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.
Attachment #8691921 - Flags: review?(ehsan) → review+
Blocks: 1189659
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.
Attachment #8691921 - Attachment is obsolete: true
(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?
Flags: needinfo?(cbook)
(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')
Flags: needinfo?(cbook)
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
Attachment #8692096 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/957a33a8949b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8692096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(catalin.badea392)
You need to log in before you can comment on or make changes to this bug.