1207727 - Rework updating service workers according to the latest spec

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Brendan Dahl [:bdahl]]

STR:
1) Load https://levin.websitewelcome.com/~xyrka/service_worker_reload/go.html
2) Click the link "one"

Expected:
After clicking "one" the message "new ServiceWorker installed" should appear the page. Firefox should find that the service worker has changed and initiate a soft update.

Actual:
Soft update is never run

Comment 1

[Brendan Dahl [:bdahl]]

And relevant notes from irc:

bkelly_pto: the spec says to run the update at the tail end of the navigation in Handle Fetch algorithm
steps 20.3.1, 21.2.1, and 22.2.1 here: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm

Comment 2

[(no longer active)]

So the spec around this has changed completely.  Ben told me on IRC that this changed during the last f2f around <https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-123504727>.  We need to implement the 24 hour updating at the end of handle functional events, plus after navigation.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Isn't running softupdate at the end of every fetch going to be expensive? Could we do this once every document or something?

Comment 4

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #3)
> Isn't running softupdate at the end of every fetch going to be expensive?
> Could we do this once every document or something?

That is exactly what the spec tells us to do if I'm reading it right.  :-)

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Its only at the end of navigation fetch events.  Not every fetch event.

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

I would like to take this bug, but because of the new security model work week, i will start to work on this next week.
Please feel free to take this if it is too late.

Comment 7

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: (WIP) Patch v1

This is for backup

Comment 8

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: (WIP) Patch v2

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v1(without verification)

Hi Ben,
I haven't done lots of test about this patch, would like to know if you have any suggestion first.

Also I have one question about the implementation, currently i will check 24 hour updating at the end of functional events and according to spec |lastUpdateCheckTime| is udpated in Update algorithm. So if i put a page over 24 hours and then refresh the page, it may trigger multiple fetch events at the same time and each of them will be treated as "over 24 hours" because the timer is not yet updated(It is updated when we really execute Update). So should I update the timer right after one functional event find the timer is over 24 hours ? or do you have any suggestion ? thank!

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the delay.  I will look at this tomorrow.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #9)
> Also I have one question about the implementation, currently i will check 24
> hour updating at the end of functional events and according to spec
> |lastUpdateCheckTime| is udpated in Update algorithm. So if i put a page
> over 24 hours and then refresh the page, it may trigger multiple fetch
> events at the same time and each of them will be treated as "over 24 hours"
> because the timer is not yet updated(It is updated when we really execute
> Update). So should I update the timer right after one functional event find
> the timer is over 24 hours ? or do you have any suggestion ? thank!

So the spec does not execute update asynchronously, but we do.  This extra async dispatch between checking update time and running update means we can queue up multiple updates.

I think the best way to do this is to move the functional event time check into the main thread runnable.  I'll comment in the review.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8672565 [details] [diff] [review]
Patch v1(without verification)

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

Looks pretty good!  I think there is a spec issue with the many update fetches as you noted.  I filed an issue.

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

I also think it would be worth writing some tests.  I think a good wpt test (if it doesn't already exist in our tree or chrome) would help a lot.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +407,5 @@
>  
>  void
>  RespondWithHandler::RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
>  {
> +  // 4.5.3, if r rejected, then set the respond-with error flag

I'm not sure the section numbers are correct here.  I think this is section 6.2.1 in respondWith()?

@@ +432,5 @@
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return;
>    }
>  
> +  // 4.5.3, If the respond-with entered flag is set, then:

I think this is 6.2.1?

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +292,5 @@
> +    const uint64_t kSecondsPerDay = 86400;
> +    const uint64_t now = PR_IntervalNow() / PR_MSEC_PER_SEC;
> +
> +    if ((mRegistration->mLastUpdateCheckTime != 0) &&
> +        (now - mRegistration->mLastUpdateCheckTime > kSecondsPerDay)) {

I think it would be nice to make the "24 hour check" a method on ServiceWorkerRegistration.

@@ +295,5 @@
> +    if ((mRegistration->mLastUpdateCheckTime != 0) &&
> +        (now - mRegistration->mLastUpdateCheckTime > kSecondsPerDay)) {
> +      nsCOMPtr<nsIRunnable> runnable;
> +      runnable = new SoftUpdateRequest(mRegistration->mScope,
> +                                       mRegistration->mPrincipal);

I was going to say we should move the 24 hour check into the main thread runnable we create here.  But update is still async.

This seems like a spec issue to me.

I guess in the meantime we should still debounce the update in some way.  I think keeping track of if an update is in progress for a registration makes sense.  If another update request comes in while another is still running, just skip the second update and mark it complete when the first completes.

I think SoftUpdateRequest is probably the right way to do this.  That way you can track the update state and do the debouncing logic all on the main thread.

I wrote a spec issue:

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

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +365,5 @@
>  
>    void
> +  SetLastUpdateCheckTime(uint64_t aTime)
> +  {
> +    mRegistration->mLastUpdateCheckTime = aTime;

I think this would be better as a method on ServiceWorkerRegistration.

Comment 13

[(no longer active)]

Dimi, are you also planning to remove the periodic updater in this bug?  If not, please file a follow-up.  Thanks!

Comment 14

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #13)
> Dimi, are you also planning to remove the periodic updater in this bug?  If
> not, please file a follow-up.  Thanks!
I create bug 1214593 to remove periodic updater.

Comment 15

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v2

I will add test cases for review next week.

Main changes for this patch:
1. Address Comment 12
2. Only set LOAD_BYPASS_CACHE when last update time is over one day(86400 sec)
3. Add |mUpdating| flag in ServiceWorkerRegistrationInfo to avoid performance issue discussed in https://github.com/slightlyoff/ServiceWorker/issues/759

Comment 16

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Dimi Lee[:dimi][:dlee] from comment #15)
> Created attachment 8674791 [details] [diff] [review]
> Patch v2
> 
> I will add test cases for review next week.
> 
> Main changes for this patch:
> 1. Address Comment 12
> 2. Only set LOAD_BYPASS_CACHE when last update time is over one day(86400
> sec)
> 3. Add |mUpdating| flag in ServiceWorkerRegistrationInfo to avoid
> performance issue discussed in
> https://github.com/slightlyoff/ServiceWorker/issues/759

There is a redundant line in ServiceWorkerScriptCache.cpp and will remove it in next patch:
DLOG(("[Dimi]CompareNetwork::OnStreamComplete is 'NOT' from cache\n"));

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8674791 [details] [diff] [review]
Patch v2

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

So I think I might have missed in my previous review that error handling is dealt with differently than what you are proposing here.  Can you take a look at the autoCancel RAII object?  I think it already handles the error cases and we don't need the new boolean flag.

I'd like to see it again to make sure we're on the same page with the error handling.

Thanks!

::: dom/workers/ServiceWorkerEvents.cpp
@@ +314,5 @@
>    nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
>    if (NS_FAILED(rv)) {
> +    // [4.5.3 event.respondWith(r)]6.3.2:
> +    // If response is not a Response object, set the respond-with error flag
> +    mFetchEvent->SetRespondWithError(true);

Note, this error is actually reported via the autoCancel RAII object.

@@ +374,5 @@
>    if (body) {
> +    // [4.5.3 event.respondWith(r)]6.3.1.1:
> +    // If response's used flag is set, set the respond-with error flag.
> +    if (NS_WARN_IF(response->BodyUsed())) {
> +      mFetchEvent->SetRespondWithError(true);

I think this is missing a return.

Also, I think we want to set a different error code.  For example, something like:

  autoCancel.SetCancelStatus(NS_ERROR_INTERCEPTED_USED_BODY);

You will need to add the new error code to ErrorList.h.  Also, add it to these files:

  https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?case=true&from=nsDocShell.cpp#5007
  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp?from=nsContentUtils.cpp#3480

And then a new error message here:

  https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#186

I have plans to change the logging a bit in bug 1215140, but for now, that is what we need.

::: dom/workers/ServiceWorkerEvents.h
@@ +57,5 @@
>    UniquePtr<ServiceWorkerClientInfo> mClientInfo;
>    nsRefPtr<Promise> mPromise;
>    bool mIsReload;
>    bool mWaitToRespond;
> +  bool mRespondWithEntered;

Can we replace this with simply |mPromise != nullptr|?

@@ +58,5 @@
>    nsRefPtr<Promise> mPromise;
>    bool mIsReload;
>    bool mWaitToRespond;
> +  bool mRespondWithEntered;
> +  bool mRespondWithError;

After further review, I don't think we need the mRespondWithError.  We already track errors in respondWith() using the autoCancel RAII object.  We need to cancel the intercepted channel in these cases, so a boolean flag is inadequate.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1283,5 @@
> +  Done(nsresult aStatus)
> +  {
> +    ServiceWorkerJob::Done(aStatus);
> +    if (mJobType == UPDATE_JOB) {
> +      mRegistration->mUpdating = false;

Can you MOZ_ASSERT(mUpdating) here?  Seems it must be true, right?

Also, please MOZ_ASSERT(NS_IsMainThread()) to ensure we're only modifying this on one thread.

@@ +2376,5 @@
> +  mLastUpdateCheckTime = PR_IntervalNow() / PR_MSEC_PER_SEC;
> +}
> +
> +bool
> +ServiceWorkerRegistrationInfo::IsLastUpdateCheckTimeOverOneDay() const

Assert main thread in the time methods as well.

@@ +2391,5 @@
> +
> +bool
> +ServiceWorkerRegistrationInfo::IsUpdating() const
> +{
> +  return mUpdating;

MOZ_ASSERT(NS_IsMainThread())

::: dom/workers/ServiceWorkerManager.h
@@ +76,5 @@
> +  uint64_t mLastUpdateCheckTime;
> +
> +  // This flag is used to avoid multiple update events triggered at the same time
> +  // See. https://github.com/slightlyoff/ServiceWorker/issues/759
> +  bool mUpdating;

Please note that it should only be accessed on main thread.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +290,5 @@
> +  void
> +  PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
> +  {
> +    if (mRegistration->IsLastUpdateCheckTimeOverOneDay() &&
> +        !mRegistration->IsUpdating()) {

nit: Do the IsUpdating() check first as its cheaper than checking the time.

@@ +1142,5 @@
>        MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));
>      }
>  
> +    // 9.8.20 If respondWithEntered is false
> +    if (!event->RespondWithEntered()) {

I think the main thing to do here is check to see if the event has been canceled.  This is 20.1 in respondWith().  Note, the intercepted channel would need to be canceled in that case.

Again, I don't think we need to check RespondWithEntered() here.  We can just treat event->GetPromise() == nullptr as "not entered".

@@ +1151,5 @@
> +
> +    // 9.8.21 If handleFetchFailed is true, then:
> +    // 1. Return a network error and continue running these substeps in parallel.
> +    // 2. If request is a non-subresource request, then: Invoke Soft Update algorithm.
> +    if (event->RespondWithError()) {

I don't think this condition is really necessary.  All errors are already handled in the RespondWithHandler via the autoCancel RAII object.  In addition, returning false from this method doesn't really do anything.  It just gets passed to the WorkerRunnable::PostRun() method which is unimplemented here.

I think we should drop the RespondWithError() flag and accessor.

@@ +1168,5 @@
> +
> +    // 9.8.22 If request is a non-subresource request, then: Invoke Soft Update algorithm
> +    if (internalReq->IsNavigationRequest()) {
> +      SoftUpdate();
> +    }

I think we just need this one SoftUpdate() call.  The other duplicate calls above can be removed.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +652,5 @@
> +
> +  // Don't let serviceworker intercept.
> +  nsCOMPtr<nsIHttpChannelInternal> internalChannel = do_QueryInterface(mChannel);
> +  if (internalChannel) {
> +    internalChannel->ForceNoIntercept();

You need to rebase.  This method is gone.  Please use LOAD_BYPASS_SERVICE_WORKER load flag now.

@@ +760,5 @@
> +
> +    // [9.2 Update]4.13, If response's cache state is not "local",
> +    // set registration's last update check time to the current time
> +    if (!isFromCache) {
> +      DLOG(("[Dimi]CompareNetwork::OnStreamComplete is 'NOT' from cache\n"));

Debugging left in?

Comment 18

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Ben Kelly [:bkelly] from comment #17)
> Comment on attachment 8674791 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8674791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +374,5 @@
> >    if (body) {
> > +    // [4.5.3 event.respondWith(r)]6.3.1.1:
> > +    // If response's used flag is set, set the respond-with error flag.
> > +    if (NS_WARN_IF(response->BodyUsed())) {
> > +      mFetchEvent->SetRespondWithError(true);
> 
> I think this is missing a return.
> 
> Also, I think we want to set a different error code.  For example, something
> like:
> 
>   autoCancel.SetCancelStatus(NS_ERROR_INTERCEPTED_USED_BODY);
> 
I found we have already implement this, so i will remove it in next patch
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#350

> 
> @@ +1168,5 @@
> > +
> > +    // 9.8.22 If request is a non-subresource request, then: Invoke Soft Update algorithm
> > +    if (internalReq->IsNavigationRequest()) {
> > +      SoftUpdate();
> > +    }
> 
> I think we just need this one SoftUpdate() call.  The other duplicate calls
> above can be removed.
> 
Yes, you are right. I will remove |mRespondWithEntered| and |mRespondWithError|

And one question,
In Spec [9.8 Handle Fetch]step 19 it says: Wait for task to have executed or been discarded.
I assume task is |FetchEventRunnable|.
Then step 22. ... invoke softupdate algorithm ...

In my current patch, the step 22 is called in the end of |DispatchFetchEvent|
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1080

Is this right place ? I am not sure if end of |DispatchFetchEvent| is already "AFTER" step 19.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

I think the end of DispatchFetchEvent() is a reasonable place to trigger soft update.  We could also maybe do it in FetchEventRunnable::PostRun(), but then we might not have access to all the variables we need.

In this case I think "task" roughly equates to a WorkerRunnable.

Also, to answer one of my review questions, I see we do handle the cancellation of the event already:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#1065

Comment 20

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch v3

Major change of this patch:
1. Remove |mRespondWithEntered| boolean flag since we can use |mPromise != nullptr| instead
2. Remove |mRespondWithError| boolean flag since it is handled by autoCancel object
3. Remove redundant softupdate call, now only call it in the end of |DispatchFetchEvent|
4. Refactor code to make sure |mUpdating| & |mLastUpdateCheckTime| will only be called in the main thread.
5. Check mUpdating flag before create |ServiceWorkerRegisterJob|(Update Job only), this is to make sure only one update is ongoing(for same registration). Because it is possible to create two softupdate request when we invoke an "over 24 hours navigation fetch event".

Comment 21

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WPT test v1

Hi Ben,
I am not sure how to test "24 hours updating", do you have any suggestion ?

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #21)
> I am not sure how to test "24 hours updating", do you have any suggestion ?

I don't think you can without creating some kind of chrome-only interface manually create a back-dated entry.

Ehsan, any ideas?

Comment 23

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #22)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #21)
> > I am not sure how to test "24 hours updating", do you have any suggestion ?
> 
> I don't think you can without creating some kind of chrome-only interface
> manually create a back-dated entry.

That sounds fine to me.  Another option would be to set a testing pref and make IsLastUpdateCheckTimeOverOneDay() return true unconditionally when that pref is set, or something like that.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8676764 [details] [diff] [review]
Patch v3

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

Sorry, I think we have an issue with the anti-burst optimization.  We don't call any of the completion callbacks if we are coalescing the updates.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +422,5 @@
> +  // Here we use |mPromise != nullptr| as respond-with enter flag
> +  if (mPromise) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return;
> +  } else {

nit: You can remove this else and just assign to mPromise outside of the block.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3306,5 @@
>    }
>  
>    // "Invoke Update algorithm, or its equivalent, with client, registration as
>    // its argument."
> +  if (!registration->mUpdating) {

Hmm, if we're already updating we fail to ever call the callback.  This may cause some things to never resolve.  I think we probably need to chain these to the currently running update in some way.

I'm sorry I missed this before.

If its too complicated, I'm fine with removing the update coalescing optimization for now.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +638,5 @@
> +      return rv;
> +    }
> +
> +    flags |= nsIRequest::LOAD_BYPASS_CACHE;
> +    rv = mChannel->SetLoadFlags(flags);

nit: I think you could move this block to be prior to the NS_NewChannel() and then just pass the correct flags directly.  Would save a few lines to read and update the flags.

Comment 25

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Ben Kelly [:bkelly] from comment #24)
> Comment on attachment 8676764 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8676764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I think we have an issue with the anti-burst optimization.  We don't
> call any of the completion callbacks if we are coalescing the updates.
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +3306,5 @@
> >    }
> >  
> >    // "Invoke Update algorithm, or its equivalent, with client, registration as
> >    // its argument."
> > +  if (!registration->mUpdating) {
> 
> Hmm, if we're already updating we fail to ever call the callback.  This may
> cause some things to never resolve.  I think we probably need to chain these
> to the currently running update in some way.
> 
> I'm sorry I missed this before.
> 
> If its too complicated, I'm fine with removing the update coalescing
> optimization for now.
> 
Thanks for catch this ! I didn't notice the callback issue :(
I will create a follow up bug for the optimization.

Comment 26

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. rework update algorithm. v4

Changes in this patch:
1. Remove |mUpdating| flag
2. Address nits in Comment 24

Comment 27

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. Navigation fetch event WPT test. v2

Just fix a file name naming error

Comment 28

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part3. 24 hours update WPT test. v1

In this patch also add a Pref:dom.serviceWorkers.testUpdateOverOneDay to always return true for |IsLastUpdateCheckTimeOverOneDay|

Comment 29

[Dimi Lee [ooo 16.04 - 21.04］]

Bug 1217367 as follow up bug according to Comment 24

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8677387 [details] [diff] [review]
Part1. rework update algorithm. v4

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

Looks great!  Just a few nits.  Thanks!  r=me

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +306,5 @@
> +  void
> +  PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
> +  {
> +    nsCOMPtr<nsIRunnable> runnable;
> +    runnable = new CheckLastUpdateTimeRequest(mRegistration);

nit: these should probably be a single statement

@@ +308,5 @@
> +  {
> +    nsCOMPtr<nsIRunnable> runnable;
> +    runnable = new CheckLastUpdateTimeRequest(mRegistration);
> +
> +    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));

nit: runnable.forget()

@@ +1168,5 @@
> +
> +    // 9.8.22 If request is a non-subresource request, then: Invoke Soft Update algorithm
> +    if (internalReq->IsNavigationRequest()) {
> +      nsCOMPtr<nsIRunnable> runnable;
> +      runnable = new SoftUpdateRequest(mRegistration);

nit: these should probably be a single statement

@@ +1170,5 @@
> +    if (internalReq->IsNavigationRequest()) {
> +      nsCOMPtr<nsIRunnable> runnable;
> +      runnable = new SoftUpdateRequest(mRegistration);
> +
> +      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));

nit: runnable.forget()

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +748,5 @@
> +    // set registration's last update check time to the current time
> +    if (!isFromCache) {
> +      RefPtr<ServiceWorkerRegistrationInfo> registration =
> +        mManager->GetRegistration();
> +      registration->RefreshLastUpdateCheckTime();

nit: this might be simpler as a single line:

mManager->GetRegistration()->RefreshLastUpdateCheckTime();

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8677390 [details] [diff] [review]
Part3. 24 hours update WPT test. v1

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/update-after-oneday.https.html
@@ +24,5 @@
> +       })
> +       .then(function() {
> +          // Trigger a non-navigation fetch event
> +          frame.contentWindow.load_image(normalizeURL('resources/update/dummy'));
> +          return wait_for_update(t, registration);

Please add a comment at the top of this file that the test requires the browser to treat all registrations as greater than 24 hours old.  This should be set as a preference during executing this test.

Comment 32

[Brendan Dahl [:bdahl]]

Original test case moved to https://adriftwith.me/sandbox/service_worker_reload/go.html

Comment 33

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Brendan Dahl [:bdahl] from comment #32)
> Original test case moved to
> https://adriftwith.me/sandbox/service_worker_reload/go.html
Following message shows after press one button:

ServiceWorker registration successful with scope: https://adriftwith.me/sandbox/service_worker_reload/
new ServiceWorker installed

Comment 34

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. rework update algorithm. v5

- Address nits in Comment 30

Hi Ben, I didn't update this:
mManager->GetRegistration()->RefreshLastUpdateCheckTime();
because GetRegistration return |already_AddRefed| which doesn't have -> operand

Comment 35

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part3. 24 hours update WPT test. v2

Add Comments

Comment 36

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8677921 [details] [diff] [review]
Part3. 24 hours update WPT test. v2

obsolete because missing files in the patch

Comment 37

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part3. 24 hours update WPT test. v3

fix a try error that caused by incorrect worker.py

Comment 38

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part1. rework update algorithm. v6

Rebase to latest code

Comment 39

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Part2. WPT tests. v3

- merge test cases into one patch
- disable periodic update test because reworked new update algorithm will trigger more update than origin test case expected. And the periodic update will removed after this bug land(bug 1214593)

Comment 40

[Dimi Lee [ooo 16.04 - 21.04］]

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

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9214ef441dab
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcde5d56c926

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9214ef441dab
https://hg.mozilla.org/mozilla-central/rev/fcde5d56c926