Closed Bug 1256428 Opened 4 years ago Closed 3 years ago

Implement new service worker unified queue spec

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

Attachments

(20 files, 32 obsolete files)

1.35 KB, patch
jdm
: review+
Details | Diff | Splinter Review
5.23 KB, patch
jdm
: review+
Details | Diff | Splinter Review
21.51 KB, patch
jdm
: review+
Details | Diff | Splinter Review
4.77 KB, patch
jdm
: review+
Details | Diff | Splinter Review
6.56 KB, patch
jdm
: review+
Details | Diff | Splinter Review
16.66 KB, patch
jdm
: review+
Details | Diff | Splinter Review
2.39 KB, patch
jdm
: review+
Details | Diff | Splinter Review
2.91 KB, patch
jdm
: review+
Details | Diff | Splinter Review
45.38 KB, patch
jdm
: review+
Details | Diff | Splinter Review
4.50 KB, patch
jdm
: review+
Details | Diff | Splinter Review
4.45 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.52 KB, patch
jdm
: review+
Details | Diff | Splinter Review
2.19 KB, patch
jdm
: review+
Details | Diff | Splinter Review
3.26 KB, patch
jdm
: review+
Details | Diff | Splinter Review
12.20 KB, patch
jdm
: review+
Details | Diff | Splinter Review
5.46 KB, patch
jdm
: review+
Details | Diff | Splinter Review
48.33 KB, patch
jdm
: review+
Details | Diff | Splinter Review
9.49 KB, patch
jdm
: review+
Details | Diff | Splinter Review
1.39 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Blocks: 1227007
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.
Attachment #8731439 - Attachment is obsolete: true
Blocks: 1257977
Attachment #8731825 - Attachment is obsolete: true
Attachment #8731826 - Attachment is obsolete: true
Blocks: 1233291
Attachment #8733632 - Attachment is obsolete: true
Attachment #8733633 - Attachment is obsolete: true
Attachment #8733634 - Attachment is obsolete: true
Attachment #8733635 - Attachment is obsolete: true
Attachment #8733636 - Attachment is obsolete: true
Attachment #8734096 - Attachment description: bug1256428_p2_queue2.patch → P2 Add ServiceWorkerJobQueue2 class. 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
Attachment #8734096 - Attachment is obsolete: true
Attachment #8734097 - Attachment is obsolete: true
Attachment #8734095 - Attachment is obsolete: true
Attachment #8734099 - Attachment is obsolete: true
Attachment #8734101 - Attachment is obsolete: true
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
Attachment #8733631 - Attachment is obsolete: true
Attachment #8734920 - Flags: review?(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.
Attachment #8734568 - Attachment is obsolete: true
Attachment #8734925 - Flags: review?(ehsan)
This is the new job queue class.  It should be fairly straightforward.
Attachment #8734185 - Attachment is obsolete: true
Attachment #8734931 - Flags: review?(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.
Attachment #8734570 - Attachment is obsolete: true
Attachment #8734932 - Flags: review?(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.
Attachment #8734571 - Attachment is obsolete: true
Comment on attachment 8734934 [details] [diff] [review]
P4 Add ServiceWorkerRegisterJob2. r=ehsan

See previous comment for description of the patch.
Attachment #8734934 - Flags: review?(ehsan)
The new unregister job.  This is pretty much a direct port of the old code from ServiceWorkerManager.  Its cleaned up in later patches.
Attachment #8734572 - Attachment is obsolete: true
Attachment #8734936 - Flags: review?(ehsan)
Modify ServiceWorkerManager to use the new job classes.
Attachment #8734573 - Attachment is obsolete: true
Attachment #8734937 - Flags: review?(ehsan)
Fix the update.https.html wpt to expect the TypeError we now throw instead of an InvalidStateError.  This matches the current spec.
Attachment #8734103 - Attachment is obsolete: true
Attachment #8734939 - Flags: review?(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.
Attachment #8734105 - Attachment is obsolete: true
Attachment #8734940 - Flags: review?(ehsan)
Remove the old job classes from ServiceWorkerManager.
Attachment #8734186 - Attachment is obsolete: true
Attachment #8734569 - Attachment is obsolete: true
Attachment #8734574 - Attachment is obsolete: true
Attachment #8734941 - Flags: review?(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.
Attachment #8734575 - Attachment is obsolete: true
Attachment #8734942 - Flags: review?(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
Attachment #8734576 - Attachment is obsolete: true
Attachment #8734943 - Flags: review?(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.
Attachment #8734944 - Flags: review?(ehsan)
This removes what I believe to be an unnecessary step now.  I wrote a spec issue:

  https://github.com/slightlyoff/ServiceWorker/issues/855
Attachment #8734964 - Flags: review?(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
Attachment #8734968 - Flags: review?(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.
Attachment #8734969 - Flags: review?(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
Attachment #8734971 - Flags: review?(ehsan)
Blocks: 1260591
Attachment #8734920 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734925 [details] [diff] [review]
P1 Add ServiceWorkerJob2 base class. r=ehsan

See comment 32 for description of patch.
Attachment #8734925 - Flags: review?(ehsan) → review?(josh)
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.
Attachment #8734931 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734932 [details] [diff] [review]
P3 Add ServiceWorkerUpdateJob2. r=ehsan

See comment 34.
Attachment #8734932 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734934 [details] [diff] [review]
P4 Add ServiceWorkerRegisterJob2. r=ehsan

See comment 35.
Attachment #8734934 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734936 [details] [diff] [review]
P5 Add ServiceWorkerUnregisterJob2 class. r=ehsan

See comment 37.
Attachment #8734936 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734937 [details] [diff] [review]
P6 Use ServiceWorkerJobQueue2 and new job classes in ServiceWorkerManager. r=ehsan

See comment 38.
Attachment #8734937 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734939 [details] [diff] [review]
P7 Fix wpt update.https.html to expect TypeError per current spec. r=ehsan

See comment 39.
Attachment #8734939 - Flags: review?(ehsan) → review?(josh)
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.
Attachment #8734940 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734941 [details] [diff] [review]
P9 Remove now unused code from ServiceWorkerManager.cpp. r=ehsan

Remove the old job classes from ServiceWorkerManager.
Attachment #8734941 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734942 [details] [diff] [review]
P10 Remove ServiceWorkerRegistrationInfo::mUpdating flag. r=ehsan

See comment 42.
Attachment #8734942 - Flags: review?(ehsan) → review?(josh)
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.
Attachment #8734943 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734944 [details] [diff] [review]
P12 ServiceWorkerUnregisterJob2 should not use ServiceWorkerManager interna ls. r=ehsan

See comment 44.
Attachment #8734944 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734964 [details] [diff] [review]
P13 Remove unnecessary ServiceWorkerUnregsterJob2 stop. r=ehsan

See comment 45.
Attachment #8734964 - Flags: review?(ehsan) → review?(josh)
Attachment #8734966 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734968 [details] [diff] [review]
P15 Perform byte-for-byte comparison check after validating script URL. r=ehsan

See comment 47.
Attachment #8734968 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734969 [details] [diff] [review]
P16 Fix some issues calling purgeCache() in ServiceWorkerUpdateJob.cpp. r=ehsan

See comment 48.
Attachment #8734969 - Flags: review?(ehsan) → review?(josh)
Comment on attachment 8734971 [details] [diff] [review]
P17 Rename service worker job classes to remove "2" suffix. r=ehsan

See comment 49.
Attachment #8734971 - Flags: review?(ehsan) → review?(josh)
Attachment #8734920 - Flags: review?(josh) → review+
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?
Attachment #8734925 - Flags: review?(josh) → review+
Attachment #8734931 - Flags: review?(josh) → review+
(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.
Here is the updated Job class with review items addressed.
Attachment #8734925 - Attachment is obsolete: true
Attachment #8736756 - Flags: review?(josh)
Rebase for new P1.
Attachment #8734943 - Attachment is obsolete: true
Attachment #8734943 - Flags: review?(josh)
Attachment #8736757 - Flags: review?(josh)
Rebase for new P1.
Attachment #8734971 - Attachment is obsolete: true
Attachment #8734971 - Flags: review?(josh)
Attachment #8736759 - Flags: review?(josh)
I updated all the commit messages in my local patch queue to reflect Josh is reviewing.
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.
Attachment #8734932 - Flags: review?(josh) → review+
Attachment #8734934 - Flags: review?(josh) → review+
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
Attachment #8734936 - Flags: review?(josh) → review+
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.
(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.
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.
Attachment #8737007 - Flags: review?(josh)
(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
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.
Attachment #8737010 - Flags: review+
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.
Attachment #8734937 - Flags: review?(josh) → review+
Attachment #8734939 - Flags: review?(josh) → review+
Attachment #8734940 - Flags: review?(josh) → review+
Attachment #8734941 - Flags: review?(josh) → review+
Attachment #8734942 - Flags: review?(josh) → review+
Attachment #8734944 - Flags: review?(josh) → review+
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?
Attachment #8734966 - Flags: review?(josh) → review+
Attachment #8734968 - Flags: review?(josh) → review+
Attachment #8734969 - Flags: review?(josh) → review+
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?
Attachment #8736756 - Flags: review?(josh) → review+
(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.
Attachment #8736757 - Flags: review?(josh) → review+
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
Attachment #8736759 - Flags: review?(josh) → review+
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!
Attachment #8737007 - Flags: review?(josh) → review+
(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.
Attachment #8734964 - Flags: review?(josh) → review+
You need to log in before you can comment on or make changes to this bug.