Rework updating service workers according to the latest spec

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: bdahl, Assigned: dimi)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 12 obsolete attachments)

27.17 KB, patch
dimi
: review+
Details | Diff | Splinter Review
8.88 KB, patch
dimi
: review+
Details | Diff | Splinter Review
Reporter

Description

4 years ago
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
Reporter

Comment 1

4 years ago
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

4 years ago
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.
Summary: Service workers not updated on navigation → Rework updating service workers according to the latest spec
Reporter

Updated

4 years ago
Blocks: WADI
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

4 years ago
(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.  :-)
Its only at the end of navigation fetch events.  Not every fetch event.
Assignee

Updated

4 years ago
Blocks: 1181389
Assignee

Comment 6

4 years ago
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.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee

Comment 7

4 years ago
Posted patch (WIP) Patch v1 (obsolete) — Splinter Review
This is for backup
Assignee

Comment 8

4 years ago
Posted patch (WIP) Patch v2 (obsolete) — Splinter Review
Attachment #8670737 - Attachment is obsolete: true
Assignee

Comment 9

4 years ago
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!
Attachment #8671320 - Attachment is obsolete: true
Attachment #8672565 - Flags: feedback?(bkelly)
Sorry for the delay.  I will look at this tomorrow.
(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 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.
Attachment #8672565 - Flags: feedback?(bkelly) → feedback+

Comment 13

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

Updated

4 years ago
Blocks: 1214593
Assignee

Comment 14

4 years ago
(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.
Assignee

Comment 15

4 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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
Attachment #8672565 - Attachment is obsolete: true
Attachment #8674791 - Flags: review?(bkelly)
Assignee

Comment 16

4 years ago
(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 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?
Attachment #8674791 - Flags: review?(bkelly)
Assignee

Comment 18

4 years ago
(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.
Flags: needinfo?(bkelly)
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
Flags: needinfo?(bkelly)
Assignee

Comment 20

4 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
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".
Attachment #8674791 - Attachment is obsolete: true
Attachment #8676764 - Flags: review?(bkelly)
Assignee

Comment 21

4 years ago
Posted patch WPT test v1 (obsolete) — Splinter Review
Hi Ben,
I am not sure how to test "24 hours updating", do you have any suggestion ?
Attachment #8676766 - Flags: review?(bkelly)
(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?
Flags: needinfo?(ehsan)

Comment 23

4 years ago
(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.
Flags: needinfo?(ehsan)
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.
Attachment #8676764 - Flags: review?(bkelly)
Attachment #8676766 - Flags: review?(bkelly) → review+
Assignee

Comment 25

4 years ago
(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.
Assignee

Comment 26

4 years ago
Changes in this patch:
1. Remove |mUpdating| flag
2. Address nits in Comment 24
Attachment #8676764 - Attachment is obsolete: true
Attachment #8677387 - Flags: review?(bkelly)
Assignee

Comment 27

4 years ago
Just fix a file name naming error
Attachment #8676766 - Attachment is obsolete: true
Attachment #8677389 - Flags: review+
Assignee

Comment 28

4 years ago
In this patch also add a Pref:dom.serviceWorkers.testUpdateOverOneDay to always return true for |IsLastUpdateCheckTimeOverOneDay|
Attachment #8677390 - Flags: review?(bkelly)
Assignee

Updated

4 years ago
Blocks: 1217367
Assignee

Comment 29

4 years ago
Bug 1217367 as follow up bug according to Comment 24
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();
Attachment #8677387 - Flags: review?(bkelly) → review+
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.
Attachment #8677390 - Flags: review?(bkelly) → review+
Assignee

Comment 33

4 years ago
(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
Assignee

Comment 34

4 years ago
- 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
Attachment #8677387 - Attachment is obsolete: true
Attachment #8677920 - Flags: review+
Assignee

Comment 35

4 years ago
Add Comments
Attachment #8677389 - Attachment is obsolete: true
Attachment #8677921 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8677389 - Attachment is obsolete: false
Assignee

Updated

4 years ago
Attachment #8677390 - Attachment is obsolete: true
Assignee

Comment 36

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

obsolete because missing files in the patch
Attachment #8677921 - Attachment is obsolete: true
Assignee

Comment 37

4 years ago
fix a try error that caused by incorrect worker.py
Attachment #8677947 - Flags: review+
Assignee

Comment 38

4 years ago
Rebase to latest code
Attachment #8677389 - Attachment is obsolete: true
Attachment #8677920 - Attachment is obsolete: true
Attachment #8677947 - Attachment is obsolete: true
Attachment #8678641 - Flags: review+
Assignee

Comment 39

4 years ago
- 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)
Assignee

Updated

4 years ago
Attachment #8678644 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed

Comment 42

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9214ef441dab
https://hg.mozilla.org/mozilla-central/rev/fcde5d56c926
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.