1256428 - Implement new service worker unified queue spec

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The service worker spec was updated to use a unified job queue to avoid races.  We should implement this as we still have a few persistent wpt intermittents that seem related to job life cycle.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Work in progress.

After discussing with Ehsan I'm going to be breaking some of the job related code out into separate classes to improve readability.  This base class is just a start.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Fix unified build failures in dom/workers. r=ehsan

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ServiceWorkerUpdateJob2. r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add ServiceWorkerRegisterJob2. r=ehsan

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ServiceWorkerUpdateJob2. r=ehsan

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add ServiceWorkerRegisterJob2. r=ehsan

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Fix wpt update.https.html to expect TypeError per current spec. r=ehsan

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Fix wpt unregister-then-register-new-script.https.html to new spec expectations matching blink's tests. r=ehsan

This set of patches passes tests locally.  Still a lot of cleanup to do, however.

Let's see how badly it blows up in try:

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ServiceWorkerUpdateJob2. r=ehsan

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

Fix some static analysis failures:

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

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ServiceWorkerUpdateJob2. r=ehsan

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add ServiceWorkerRegisterJob2. r=ehsan

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Remove now unused code from ServiceWorkerManager.cpp. r=ehsan

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Remove ServiceWorkerRegistrationInfo::mUpdating flag. r=ehsan

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Don't coalesce SW jobs after the existing job has already resolved its promise. r=ehsan

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Still some more cleanup to do, but getting close.  Hope to flag for review by the end of tomorrow.

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

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Fix unified build failures in dom/workers. r=ehsan

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

This patch adds a job base class.  I am using the "Job2" suffix on names for now to make the patches incremental.  The last patch will rename the classes to their final names.

Some differences will the old job base class:

1) Start() always calls the job entry point asynchronously.  We don't have to worry about jobs accidentally completely synchronously and blowing the stack.  Also, this matches chrome's behavior of always running jobs async.
2) Waiting for the PBackground actor is handled in a single place instead of in each job.

Job coalescing is handled later in P11.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add ServiceWorkerJobQueue2 class. r=ehsan

This is the new job queue class.  It should be fairly straightforward.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add ServiceWorkerUpdateJob2. r=ehsan

This is a fairly large patch, but it is mostly existing code from ServiceWorkerManager.  This job represents the Update and Install algorithm steps.  The code previously was split across part of the ServiceWorkerRegisterJob and ServiceWorkerInstallJob.

I did my best to leave the code the same, but some things had to be changed for the new job format.  Also, some checks no longer made sense given that Update and Install run as part of the same job.  Things cannot change in between  those two steps any more.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add ServiceWorkerRegisterJob2. r=ehsan

This is the new register job.  It is a slight extension to the new update job to perform the few steps that are different from updating.  I think its easier to see whats going on this way instead of using an "if(type)" statement like in the old code.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734934 [details] [diff] [review]
P4 Add ServiceWorkerRegisterJob2. r=ehsan

See previous comment for description of the patch.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

The new unregister job.  This is pretty much a direct port of the old code from ServiceWorkerManager.  Its cleaned up in later patches.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

Modify ServiceWorkerManager to use the new job classes.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Fix wpt update.https.html to expect TypeError per current spec. r=ehsan

Fix the update.https.html wpt to expect the TypeError we now throw instead of an InvalidStateError.  This matches the current spec.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Fix wpt unregister-then-register-new-script.https.html to new spec expec tations matching blink's tests. r=ehsan

Fix the unregister-then-register-new-script.https.html wpt expectations to match our new behavior.  This also matches what chrome does.  The difference here is a result of our "always run jobs async" behavior which prevents the uninstall-flag from being cleared synchronously.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Remove now unused code from ServiceWorkerManager.cpp. r=ehsan

Remove the old job classes from ServiceWorkerManager.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Remove ServiceWorkerRegistrationInfo::mUpdating flag. r=ehsan

Now that we implement coalescing of jobs, we don't need to maintain the mUpdating flag on the registration.  That spec language is long dead.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Don't coalesce SW jobs after the existing job has already resolved its promise. r=ehsan

Avoid coalescing jobs if the active job has already "resolved its promise".  This was a spec issue I wrote:

  https://github.com/slightlyoff/ServiceWorker/issues/853

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P12 ServiceWorkerUnregisterJob2 should not use ServiceWorkerManager interna ls. r=ehsan

Fix some places the unregister code was reaching into the SWM guts.  This was allowed due to friend class.  We don't need to do it, though.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P13 Remove unnecessary ServiceWorkerUnregsterJob2 stop. r=ehsan

This removes what I believe to be an unnecessary step now.  I wrote a spec issue:

  https://github.com/slightlyoff/ServiceWorker/issues/855

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P14 Remove dead code in SeviceWorkerUpdateJob.cpp. r=ehsan

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P15 Perform byte-for-byte comparison check after validating script URL. r=ehsan

We had a slight bug in that we accepted a byte-for-byte match even if the service-worker-allowed header disallowed the script URL.  Reorder things to check for a bad script URL before accepting the comparison match.

The main impact of this would be that we will not reject the promise where we would have resolved the promise with success before.  In either case no worker is actually installed, though.

I wrote a spec issue asking if we should do something with the installed worker:

  https://github.com/slightlyoff/ServiceWorker/issues/857

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P16 Fix some issues calling purgeCache() in ServiceWorkerUpdateJob.cpp. r=ehsan

Tweak how we purgeCache() some things in the update worker.  Basically this removes some duplicate code, fixes a use where we had already nulled the worker, and removes the stderr warnings that probably no one looks at anyway.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Rename service worker job classes to remove "2" suffix. r=ehsan

Finally, rename the classes to their final names by removing the "2" suffix.

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

Note, there was another spec issue I found that I might try to fix here if I get an answer in time:

  https://github.com/slightlyoff/ServiceWorker/issues/856

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734925 [details] [diff] [review]
P1 Add ServiceWorkerJob2 base class. r=ehsan

See comment 32 for description of patch.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734931 [details] [diff] [review]
P2 Add ServiceWorkerJobQueue2 class. r=ehsan

This is the new job queue class.  It should be fairly straightforward.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734932 [details] [diff] [review]
P3 Add ServiceWorkerUpdateJob2. r=ehsan

See comment 34.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734934 [details] [diff] [review]
P4 Add ServiceWorkerRegisterJob2. r=ehsan

See comment 35.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734936 [details] [diff] [review]
P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

See comment 37.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734937 [details] [diff] [review]
P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

See comment 38.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734939 [details] [diff] [review]
P7 Fix wpt update.https.html to expect TypeError per current spec. r=ehsan

See comment 39.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734940 [details] [diff] [review]
P8 Fix wpt unregister-then-register-new-script.https.html to new spec expec tations matching blink's tests. r=ehsan

See comment 40.

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734941 [details] [diff] [review]
P9 Remove now unused code from ServiceWorkerManager.cpp. r=ehsan

Remove the old job classes from ServiceWorkerManager.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734942 [details] [diff] [review]
P10 Remove ServiceWorkerRegistrationInfo::mUpdating flag. r=ehsan

See comment 42.

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734943 [details] [diff] [review]
P11 Don't coalesce SW jobs after the existing job has already resolved its promise. r=ehsan

See comment 43.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734944 [details] [diff] [review]
P12 ServiceWorkerUnregisterJob2 should not use ServiceWorkerManager interna ls. r=ehsan

See comment 44.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734964 [details] [diff] [review]
P13 Remove unnecessary ServiceWorkerUnregsterJob2 stop. r=ehsan

See comment 45.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734968 [details] [diff] [review]
P15 Perform byte-for-byte comparison check after validating script URL. r=ehsan

See comment 47.

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734969 [details] [diff] [review]
P16 Fix some issues calling purgeCache() in ServiceWorkerUpdateJob.cpp. r=ehsan

See comment 48.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8734971 [details] [diff] [review]
P17 Rename service worker job classes to remove "2" suffix. r=ehsan

See comment 49.

Comment 66

[Josh Matthews [:jdm]]

Comment on attachment 8734925 [details] [diff] [review]
P1 Add ServiceWorkerJob2 base class. r=ehsan

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

I wouldn't mind seeing another iteration of this.

::: dom/workers/ServiceWorkerJob.cpp
@@ +143,5 @@
> +{
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT(mState == State::Started);
> +
> +  for (RefPtr<Callback>& callback : mResultCallbackList) {

mResultCallbackList could be mutated by AppendResultCallback, so this isn't safe.

@@ +151,5 @@
> +    aRv.CloneTo(rv);
> +
> +    callback->JobFinished(this, rv);
> +
> +    // The callback may not consume the error.

Is that "may not" as in "must not", or "might not"?

@@ +167,5 @@
> +
> +void
> +ServiceWorkerJob2::Finish(ErrorResult& aRv)
> +{
> +  AssertIsOnMainThread();

Should we assert that state is Started here?

@@ +195,5 @@
> +
> +  mFinalCallback->JobFinished(this, aRv);
> +  mFinalCallback = nullptr;
> +
> +  // The callback may not consume the error.

might not

@@ +200,5 @@
> +  aRv.SuppressException();
> +
> +  // Async release this object to ensure that our caller methods complete
> +  // as well.
> +  NS_ReleaseOnMainThread(kungFuDeathGrip.forget(), true /* always proxy */);

This sounds like it's compensating for callers not having their own death grips. I guess that's fine?

::: dom/workers/ServiceWorkerJob.h
@@ +29,5 @@
> +  public:
> +    // Called once when the job completes.  If the job is started, then this
> +    // will be called.  If a job is never executed due to browser shutdown,
> +    // then this method will never be called.  This method is always called
> +    // asynchronous from Start() on the main thread.

"always called asynchronous"? It might be clearer to state "This method is always called on the main thread asynchronously after Start() completes."

@@ +82,5 @@
> +  void
> +  StealResultCallbacksFrom(ServiceWorkerJob2* aJob);
> +
> +  // Start the job.  All work will be performed asynchronously on
> +  // the main thread.  Once called the Finish() method must be called

"The Finish() method must be called exactly once after this point."

@@ +113,5 @@
> +  InvokeResultCallbacks(nsresult aRv);
> +
> +  // Indicate that the job has completed.  The must be called exactly
> +  // once after Start() has initiated job execution.  It must be called
> +  // asynchronously from Start().

"asynchronously from Start"? Maybe "It may not be called until Start() has returned" instead?

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #66)
> ::: dom/workers/ServiceWorkerJob.cpp
> @@ +143,5 @@
> > +{
> > +  AssertIsOnMainThread();
> > +  MOZ_ASSERT(mState == State::Started);
> > +
> > +  for (RefPtr<Callback>& callback : mResultCallbackList) {
> 
> mResultCallbackList could be mutated by AppendResultCallback, so this isn't
> safe.

This AppendResultCallback() doesn't get called during this loop.  Patch P11 adds an assert verifying it doesn't happen.

> @@ +151,5 @@
> > +    aRv.CloneTo(rv);
> > +
> > +    callback->JobFinished(this, rv);
> > +
> > +    // The callback may not consume the error.
> 
> Is that "may not" as in "must not", or "might not"?

Might not.

> @@ +200,5 @@
> > +  aRv.SuppressException();
> > +
> > +  // Async release this object to ensure that our caller methods complete
> > +  // as well.
> > +  NS_ReleaseOnMainThread(kungFuDeathGrip.forget(), true /* always proxy */);
> 
> This sounds like it's compensating for callers not having their own death
> grips. I guess that's fine?

I'd rather do one death grip instead of force everyone else to add one and possibly miss that its needed.  This seems safer and requires less duplicated code.

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorkerJob2 base class. r=ehsan

Here is the updated Job class with review items addressed.

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Don't coalesce SW jobs after the existing job has already resolved its promise. r=ehsan

Rebase for new P1.

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P17 Rename service worker job classes to remove "2" suffix. r=ehsan

Rebase for new P1.

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

I updated all the commit messages in my local patch queue to reflect Josh is reviewing.

Comment 72

[Josh Matthews [:jdm]]

Comment on attachment 8734932 [details] [diff] [review]
P3 Add ServiceWorkerUpdateJob2. r=ehsan

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

Most of my comments her are a reflection of me finding it difficult to follow the code while reading the parts of the specification that I think were relevant. I'd like to see the next revision.

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +129,5 @@
> +{
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::FailUpdateJob(ErrorResult& aRv)

Let's link to the appropriate part of the spec here and add comments indicating the steps being implemented. I'm assuming this is https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#installation-algorithm step 12.

@@ +173,5 @@
> +  FailUpdateJob(rv);
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::AsyncExecute()

Let's annotate this with a specification link and comments indicating steps being implemented.

@@ +196,5 @@
> +    return;
> +  }
> +
> +  // If a different script has been registered between when this update
> +  // was scheduled and it running now, then simply abort.

"between when this update was scheduled and it running now"?

@@ +221,5 @@
> +  mRegistration = aRegistration;
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::Update()

Specification link and step annotations.

@@ +231,5 @@
> +
> +  // TODO: I think we can remove mUpdating from registration now
> +  mRegistration->mUpdating = true;
> +
> +  if (Canceled()) {

This is duplicated in the caller; can we just assert that we're not canceled here?

@@ +260,5 @@
> +  }
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::ComparisonResult(nsresult aStatus,

Specification link and step annotations.

@@ +359,5 @@
> +  }
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::ContinueUpdateAfterScriptEval(bool aScriptEvaluationResult)

Specification link and step annotations.

@@ +404,5 @@
> +  RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +  swm->InvalidateServiceWorkerRegistrationWorker(mRegistration,
> +                                                 WhichServiceWorker::INSTALLING_WORKER);
> +
> +  InvokeResultCallbacks(NS_OK);

Is this the [Resolve Job Promise] thing from step 6?

@@ +406,5 @@
> +                                                 WhichServiceWorker::INSTALLING_WORKER);
> +
> +  InvokeResultCallbacks(NS_OK);
> +
> +  // The job should NOT fail from this point on.

Is it possible to assert this before returning?

@@ +436,5 @@
> +  }
> +}
> +
> +void
> +ServiceWorkerUpdateJob2::ContinueAfterInstallEvent(bool aInstallEventSuccess)

More annotations for steps being followed wouldn't go amiss here.

@@ +480,5 @@
> +                                                 WhichServiceWorker::WAITING_WORKER);
> +
> +  Finish(NS_OK);
> +
> +  // Activate() is invoked out of band of atomic.

I'm not clear on what this means in relation to the spec, since it says to invoke Finish after Activate. Also, how do we handle the "skip waiting" bits?

::: dom/workers/ServiceWorkerUpdateJob.h
@@ +39,5 @@
> +                          nsILoadGroup* aLoadGroup);
> +
> +  virtual ~ServiceWorkerUpdateJob2();
> +
> +  // FailUpdateJob() must be called if an update job needs Finish() with

Is there a risk of some caller invoking Finish instead of FailUpdateJob? I haven't figured out which code actually invokes these yet.

Comment 73

[Josh Matthews [:jdm]]

Comment on attachment 8734936 [details] [diff] [review]
P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

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

::: dom/workers/ServiceWorkerUnregisterJob.cpp
@@ +25,5 @@
> +{
> +  AssertIsOnMainThread();
> +  return mResult;
> +}
> +ServiceWorkerUnregisterJob2::~ServiceWorkerUnregisterJob2()

nit: add a newline above this line.

@@ +45,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    Finish(NS_OK);
> +    return;
> +  }
> +

Where is step 1 implemented?

@@ +94,5 @@
> +  InvokeResultCallbacks(NS_OK);
> +
> +  // "If no service worker client is using registration..."
> +  if (!registration->IsControllingDocuments()) {
> +    // "If registration's uninstalling flag is set.."

is unset

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

So, I used to like adding spec steps to the code, but over time I came to dislike it.  The steps are almost always out of date with the actual spec.  It also encourages you to mirror the spec structure, which is not always good given the spec is not written with real implementation constraints in mind.

That being said, I'll annotate and link as requested.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #72)
> > +ServiceWorkerUpdateJob2::FailUpdateJob(ErrorResult& aRv)
> 
> Let's link to the appropriate part of the spec here and add comments
> indicating the steps being implemented. I'm assuming this is
> https://slightlyoff.github.io/ServiceWorker/spec/service_worker/
> #installation-algorithm step 12.

Kind of.  The spec doesn't really acknowledge that errors can happen all throughout the algorithm.  We need to clean up in those cases as well even though the spec doesn't reference them.

> @@ +406,5 @@
> > +                                                 WhichServiceWorker::INSTALLING_WORKER);
> > +
> > +  InvokeResultCallbacks(NS_OK);
> > +
> > +  // The job should NOT fail from this point on.
> 
> Is it possible to assert this before returning?

That comment is not strictly correct.  The job can still fail (see step 12), but we can't reject the promise after this point.

I'm just going to clarify the comment.

> @@ +480,5 @@
> > +                                                 WhichServiceWorker::WAITING_WORKER);
> > +
> > +  Finish(NS_OK);
> > +
> > +  // Activate() is invoked out of band of atomic.
> 
> I'm not clear on what this means in relation to the spec, since it says to
> invoke Finish after Activate. Also, how do we handle the "skip waiting" bits?

The spec says to run Finish and then Activate.

I think the comment is just trying to say we let the next job run at this point.  I'll clarify the comment.

The skipWaiting bits are done in TryToActivate() in ServiceWorkerManager.cpp.

> ::: dom/workers/ServiceWorkerUpdateJob.h
> @@ +39,5 @@
> > +                          nsILoadGroup* aLoadGroup);
> > +
> > +  virtual ~ServiceWorkerUpdateJob2();
> > +
> > +  // FailUpdateJob() must be called if an update job needs Finish() with
> 
> Is there a risk of some caller invoking Finish instead of FailUpdateJob? I
> haven't figured out which code actually invokes these yet.

This really only matters for RegisterJob and UpdateJob.  The code just needs to do cleanup on failure so there is a different method to call.  I could make Finish() virtual and override it instead if you prefer.

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P18 Add spec annotations and tweak asserts in ServiceWorkerUpdateJob. r=jdm

This patch addresses your review comments on P3.  Since it was such a large patch to begin with I thought a separate patch with just the further changes would be easier to review.  Also, doing it this way avoids rebasing the whole patch queue.

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #73)
> @@ +45,5 @@
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    Finish(NS_OK);
> > +    return;
> > +  }
> > +
> 
> Where is step 1 implemented?

We implement it in the registration update() method because we don't have the client origin readily available.  We could assume the service worker principal matches the client, but I think it would be safer to use the real caller's principal.

I'll add a comment to this effect.

> @@ +94,5 @@
> > +  InvokeResultCallbacks(NS_OK);
> > +
> > +  // "If no service worker client is using registration..."
> > +  if (!registration->IsControllingDocuments()) {
> > +    // "If registration's uninstalling flag is set.."
> 
> is unset

This code actually goes away in P13.  I wrote a spec issue for it as well:

  https://github.com/slightlyoff/ServiceWorker/issues/855

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P19 Address ServiceWorkerUnregisterJob review feedback. r=jdm

Address P5 review feedback.  I'm doing this as a separate patch to avoid rebasing hell, but claiming r+ since you didn't ask to see it again and its only comments/whitespace.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

I had to rebase 4 or 5 patches here for changes in m-c.  Mainly because some logging was changed in SWM.  I can upload if you want, but for now I will leave the patches to avoid churning the review flags.

Comment 80

[Josh Matthews [:jdm]]

Comment on attachment 8734964 [details] [diff] [review]
P13 Remove unnecessary ServiceWorkerUnregsterJob2 stop. r=ehsan

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

::: dom/workers/ServiceWorkerUnregisterJob.cpp
@@ -70,5 @@
>    InvokeResultCallbacks(NS_OK);
>  
>    // "If no service worker client is using registration..."
>    if (!registration->IsControllingDocuments()) {
> -    // "If registration's uninstalling flag is set.."

Why is this unnecessary? What if resolving the promise sets the flag back to false?

Comment 81

[Josh Matthews [:jdm]]

Comment on attachment 8736756 [details] [diff] [review]
P1 Add ServiceWorkerJob2 base class. r=ehsan

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

::: dom/workers/ServiceWorkerJob.cpp
@@ +157,5 @@
> +
> +    // The callback might not consume the error.
> +    rv.SuppressException();
> +  }
> +}

Would it be useful to assert that mResultCallbackList is still empty to ensure that we don't miss any callbacks that are appended while invoking existing ones?

Comment 82

[Josh Matthews [:jdm]]

(In reply to Josh Matthews [:jdm] from comment #81)
> Comment on attachment 8736756 [details] [diff] [review]
> P1 Add ServiceWorkerJob2 base class. r=ehsan
> 
> Review of attachment 8736756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerJob.cpp
> @@ +157,5 @@
> > +
> > +    // The callback might not consume the error.
> > +    rv.SuppressException();
> > +  }
> > +}
> 
> Would it be useful to assert that mResultCallbackList is still empty to
> ensure that we don't miss any callbacks that are appended while invoking
> existing ones?

Nevermind, I see that patch 11 deals with this in a clearer fashion.

Comment 83

[Josh Matthews [:jdm]]

Comment on attachment 8736759 [details] [diff] [review]
P17 Rename service worker job classes to remove "2" suffix. r=ehsan

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

rs=me

Comment 84

[Josh Matthews [:jdm]]

Comment on attachment 8737007 [details] [diff] [review]
P18 Add spec annotations and tweak asserts in ServiceWorkerUpdateJob. r=jdm

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

Thanks!

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #80)
> > -    // "If registration's uninstalling flag is set.."
> 
> Why is this unnecessary? What if resolving the promise sets the flag back to
> false?

This can't happen.  The only way to set the uninstalling flag is within an UnregisterJob.  The UnregisterJob can't run until this job completes, though.  So the flag cannot change here anymore.

Removing this kind of race is exactly why we moved the spec to a single job queue.  It avoids different jobs stepping on each other.

Comment 86

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18604f22aea7
https://hg.mozilla.org/integration/mozilla-inbound/rev/691183bd2da1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26e66f6a854
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89f08e282fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eaa1b15b11e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb2828dc235
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de7a43bb8e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/586879b9c3f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0ebe724067
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0db42310daa
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c199e5700ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10919e1102f
https://hg.mozilla.org/integration/mozilla-inbound/rev/95dd71ea8f20
https://hg.mozilla.org/integration/mozilla-inbound/rev/3624db0bbe3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/209f8369a848
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d20968bc19d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81bcbb3caa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7cfe9e9cf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/be35e93fe941
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c01df3e3c6

Comment 87

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18604f22aea7
https://hg.mozilla.org/mozilla-central/rev/691183bd2da1
https://hg.mozilla.org/mozilla-central/rev/d26e66f6a854
https://hg.mozilla.org/mozilla-central/rev/c89f08e282fb
https://hg.mozilla.org/mozilla-central/rev/6eaa1b15b11e
https://hg.mozilla.org/mozilla-central/rev/8eb2828dc235
https://hg.mozilla.org/mozilla-central/rev/6de7a43bb8e5
https://hg.mozilla.org/mozilla-central/rev/586879b9c3f7
https://hg.mozilla.org/mozilla-central/rev/7b0ebe724067
https://hg.mozilla.org/mozilla-central/rev/b0db42310daa
https://hg.mozilla.org/mozilla-central/rev/1c199e5700ff
https://hg.mozilla.org/mozilla-central/rev/f10919e1102f
https://hg.mozilla.org/mozilla-central/rev/95dd71ea8f20
https://hg.mozilla.org/mozilla-central/rev/3624db0bbe3f
https://hg.mozilla.org/mozilla-central/rev/209f8369a848
https://hg.mozilla.org/mozilla-central/rev/1d20968bc19d
https://hg.mozilla.org/mozilla-central/rev/b81bcbb3caa2
https://hg.mozilla.org/mozilla-central/rev/fe7cfe9e9cf6
https://hg.mozilla.org/mozilla-central/rev/be35e93fe941
https://hg.mozilla.org/mozilla-central/rev/c8c01df3e3c6