RemoteServiceWorkerRegistration::Update() does not implement delayed self-update mechanism

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
2 months ago

People

(Reporter: bkelly, Assigned: edenchuang)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: SW-MUST)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 months ago
The legacy worker update() implementation has a delay mechanism to prevent infinite self-update of the worker.  See:

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp#300-304

This needs to be implemented in the RemoteServiceWorkerRegistrationImpl update path in some way.  Perhaps in the ServiceWorkerRegistrationProxy since it needs to store information on the ServiceWorkerInfo/Private.
(Reporter)

Comment 1

11 months ago
Note, this bug explains this mochitest failure:

[task 2018-06-29T19:41:11.608Z]     INFO - TEST-START | dom/serviceworkers/test/test_self_update_worker.html
[task 2018-06-29T19:41:11.705Z]     INFO - GECKO(2118) | --DOMWINDOW == 35 (0xccf4d080) [pid = 2118] [serial = 492] [outer = (nil)] [url = http://example.com/tests/dom/serviceworkers/test/sanitize/example_check_and_unregister.html]
[task 2018-06-29T19:41:11.729Z]     INFO - GECKO(2118) | ++DOMWINDOW == 36 (0xcf0c3600) [pid = 2118] [serial = 514] [outer = 0xdabe17a0]
[task 2018-06-29T19:41:12.514Z]     INFO - GECKO(2118) | [2118, Main Thread] WARNING: 'NS_FAILED(rr->RetargetDeliveryTo(sts))', file /builds/worker/workspace/build/src/dom/fetch/FetchDriver.cpp, line 1018
[task 2018-06-29T19:41:13.525Z]     INFO - GECKO(2118) | --DOMWINDOW == 35 (0xccf4d7a0) [pid = 2118] [serial = 509] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache_save_after_load.html]
[task 2018-06-29T19:41:13.525Z]     INFO - GECKO(2118) | --DOMWINDOW == 34 (0xccf4d670) [pid = 2118] [serial = 507] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache.html]
[task 2018-06-29T19:41:13.525Z]     INFO - GECKO(2118) | --DOMWINDOW == 33 (0xccf4d540) [pid = 2118] [serial = 505] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache_with_sri.html]
[task 2018-06-29T19:41:13.525Z]     INFO - GECKO(2118) | --DOMWINDOW == 32 (0xccf4d8d0) [pid = 2118] [serial = 511] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache_syntax_error.html]
[task 2018-06-29T19:41:13.526Z]     INFO - GECKO(2118) | --DOMWINDOW == 31 (0xccf4d1b0) [pid = 2118] [serial = 499] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache.html]
[task 2018-06-29T19:41:13.526Z]     INFO - GECKO(2118) | --DOMWINDOW == 30 (0xccf4d2e0) [pid = 2118] [serial = 501] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache.html]
[task 2018-06-29T19:41:13.526Z]     INFO - GECKO(2118) | --DOMWINDOW == 29 (0xccf4d410) [pid = 2118] [serial = 503] [outer = (nil)] [url = http://mochi.test:8888/tests/dom/serviceworkers/test/file_js_cache_with_sri.html]
[task 2018-06-29T19:41:13.816Z]     INFO - TEST-INFO | started process screentopng
[task 2018-06-29T19:41:14.524Z]     INFO - TEST-INFO | screentopng: exit 0
[task 2018-06-29T19:41:14.524Z]     INFO - Buffered messages logged at 19:41:12
[task 2018-06-29T19:41:14.524Z]     INFO - AddTask.js | Entering test setupPrefs
[task 2018-06-29T19:41:14.524Z]     INFO - AddTask.js | Leaving test setupPrefs
[task 2018-06-29T19:41:14.525Z]     INFO - AddTask.js | Entering test test_update
[task 2018-06-29T19:41:14.525Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.1 
[task 2018-06-29T19:41:14.525Z]     INFO - Buffered messages logged at 19:41:13
[task 2018-06-29T19:41:14.525Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | got dummy! 
[task 2018-06-29T19:41:14.525Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.2 
[task 2018-06-29T19:41:14.526Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | got dummy! 
[task 2018-06-29T19:41:14.526Z]     INFO - Buffered messages finished
[task 2018-06-29T19:41:14.526Z]     INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.3 
[task 2018-06-29T19:41:14.527Z]     INFO -     test_update/navigator.serviceWorker.onmessage@dom/serviceworkers/test/test_self_update_worker.html:59:5
[task 2018-06-29T19:41:14.527Z]     INFO -     EventHandlerNonNull*test_update@dom/serviceworkers/test/test_self_update_worker.html:58:3
[task 2018-06-29T19:41:14.528Z]     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-06-29T19:41:14.528Z]     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-06-29T19:41:14.528Z]     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-06-29T19:41:14.529Z]     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-06-29T19:41:14.529Z]     INFO -     @dom/serviceworkers/test/test_self_update_worker.html:38:1
[task 2018-06-29T19:41:14.529Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | got dummy! 
[task 2018-06-29T19:41:14.530Z]     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-06-29T19:41:14.531Z]     INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.4 
[task 2018-06-29T19:41:14.532Z]     INFO -     test_update/navigator.serviceWorker.onmessage@dom/serviceworkers/test/test_self_update_worker.html:59:5
[task 2018-06-29T19:41:14.533Z]     INFO -     EventHandlerNonNull*test_update@dom/serviceworkers/test/test_self_update_worker.html:58:3
[task 2018-06-29T19:41:14.533Z]     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-06-29T19:41:14.534Z]     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-06-29T19:41:14.534Z]     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-06-29T19:41:14.534Z]     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-06-29T19:41:14.535Z]     INFO -     @dom/serviceworkers/test/test_self_update_worker.html:38:1
[task 2018-06-29T19:41:14.543Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | got dummy! 
[task 2018-06-29T19:41:14.544Z]     INFO - GECKO(2118) | ###!!! [Parent][DispatchAsyncMessage] Error: PServiceWorkerRegistration::Msg_Teardown Route error: message sent to unknown actor ID
[task 2018-06-29T19:41:14.544Z]     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-06-29T19:41:14.545Z]     INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.5 
[task 2018-06-29T19:41:14.545Z]     INFO -     test_update/navigator.serviceWorker.onmessage@dom/serviceworkers/test/test_self_update_worker.html:59:5
[task 2018-06-29T19:41:14.546Z]     INFO -     EventHandlerNonNull*test_update@dom/serviceworkers/test/test_self_update_worker.html:58:3
[task 2018-06-29T19:41:14.546Z]     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-06-29T19:41:14.547Z]     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-06-29T19:41:14.547Z]     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-06-29T19:41:14.547Z]     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-06-29T19:41:14.548Z]     INFO -     @dom/serviceworkers/test/test_self_update_worker.html:38:1
[task 2018-06-29T19:41:14.564Z]     INFO - TEST-PASS | dom/serviceworkers/test/test_self_update_worker.html | got dummy! 
[task 2018-06-29T19:41:14.708Z]     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-06-29T19:41:14.708Z]     INFO - TEST-UNEXPECTED-FAIL | dom/serviceworkers/test/test_self_update_worker.html | Service worker updated too many times.6 
[task 2018-06-29T19:41:14.708Z]     INFO -     test_update/navigator.serviceWorker.onmessage@dom/serviceworkers/test/test_self_update_worker.html:59:5
[task 2018-06-29T19:41:14.708Z]     INFO -     EventHandlerNonNull*test_update@dom/serviceworkers/test/test_self_update_worker.html:58:3
[task 2018-06-29T19:41:14.708Z]     INFO -     async*add_task/</<@SimpleTest/AddTask.js:57:34
[task 2018-06-29T19:41:14.708Z]     INFO -     async*add_task/<@SimpleTest/AddTask.js:31:10
[task 2018-06-29T19:41:14.708Z]     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2018-06-29T19:41:14.708Z]     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-06-29T19:41:14.708Z]     INFO -     @dom/serviceworkers/test/test_self_update_worker.html:38:1

Updated

11 months ago
Priority: -- → P2

Updated

9 months ago
Whiteboard: SW-MUST

Updated

6 months ago
Assignee: nobody → perry
Eden, would you mind taking a look at this?
Assignee: perry → echuang
(Assignee)

Comment 3

5 months ago
Sure, going to start on it.
(Assignee)

Comment 4

4 months ago

Perry, I wrote a patch for delay update mechanism in ServiceWorkerRegistrationProxy.

Could you give some feedbacks on it?

Thanks.

Attachment #9036340 - Flags: feedback?(perry)

Updated

4 months ago
Attachment #9036340 - Flags: feedback?(perry) → feedback-
Comment on attachment 9036340 [details] [diff] [review]
Implement delay update mechanism in ServiceWorkerRegistrationProxy

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

Overall, I think the main issue is that spam-attempting updates is still possible, but all the attempts are (possibly) delayed now. There should be a check to see if there's already a pending update and not dispatch a DelayedUpdateCallback if that's the case.

::: dom/serviceworkers/ServiceWorkerRegistrationProxy.cpp
@@ +231,5 @@
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  DelayedUpdateCallback(ServiceWorkerRegistrationProxy* aProxy,
> +      ServiceWorkerRegistrationPromise::Private* aPromise)

In the ServiceWorkerRegistrationProxy::Update function's lambda, DelayedUpdateCallback (conceptually) takes ownership of `promise`, so we could have this constructor take a RefPtr<ServiceWorkerRegistrationPromise::Private>&& or already_AddRefed<ServiceWorkerRegistrationPromise::Private>. This also avoids an AddRef/Release pair of calls from mPromise's initialization and `promise`'s destructor in the lambda.

Looking at the lambda again, I suppose ServiceWorkerRegistrationProxy could be an rvalue ref/already_AddRefed too.

@@ +233,5 @@
> +
> +  DelayedUpdateCallback(ServiceWorkerRegistrationProxy* aProxy,
> +      ServiceWorkerRegistrationPromise::Private* aPromise)
> +    : mProxy(aProxy)
> +    , mPromise(aPromise)

If the constructor is changed to take a rvalue reference for aPromise, this would then be mPromise(std::move(aPromise)).

@@ +239,5 @@
> +    MOZ_DIAGNOSTIC_ASSERT(mProxy);
> +    MOZ_DIAGNOSTIC_ASSERT(mPromise);
> +  }
> +
> +  bool IsDelayed();

This doesn't look like it's used, so it could be removed.

@@ +246,5 @@
> +NS_IMPL_ISUPPORTS(DelayedUpdateCallback, nsITimerCallback)
> +
> +NS_IMETHODIMP DelayedUpdateCallback::Notify(nsITimer* aTimer)
> +{
> +  mProxy->UpdateOnMainThread(mPromise);

This could also then be mPromise->UpdateOnMainThread(std::move(mPromise));.

@@ +250,5 @@
> +  mProxy->UpdateOnMainThread(mPromise);
> +  // remove timer
> +  if (aTimer) {
> +    aTimer = nullptr;
> +  }

I don't think this has any affect (i.e. nulling-out a local raw pointer), so the if statement/block could be removed.

@@ +267,5 @@
> +  NS_ENSURE_TRUE(swm, 0);
> +
> +  RefPtr<ServiceWorkerRegistrationInfo> registration =
> +  swm->GetRegistration(principal, mDescriptor.Scope());
> +  NS_ENSURE_TRUE(registration, 0);

I think the NS_ENSURE_TRUE(..., 0) is okay... but would we want to abort the update early, ASAP after principal/swm/registration is null? Otherwise the caller of GetUpdateDelay() would think it's time to update and proceed to do so (and some other assertion down the line might trigger anyways).

@@ +285,5 @@
> +
> +  RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +  NS_ENSURE_TRUE_VOID(swm);
> +
> +  RefPtr<UpdateCallback> cb = new UpdateCallback(std::move(aPromise));

std::move-ing a raw pointer won't have any effect (in terms of calling RefPtr's move constructor in UpdateCallback's constructor) and so UpdateCallback's constructor's RefPtr parameter will still AddRef() the aPromise passed in. We can rather have UpdateOnMainThread's parameter be of type RefPtr<ServiceWorkerRegistrationPromise::Private>&& (or already_Addrefed<ServiceWorkerRegistrationPromise::Private, I suppose), to be able to move it into the UpdateCallback.

@@ +301,2 @@
>  
>    nsCOMPtr<nsIRunnable> r =

Correct me if I'm wrong, but I think the main issue here is that we don't check if there's already a pending update that's been delayed. As a consequence, if Update() is spam-called 100 times, while an 100 update attempt's won't necessarily be immediately triggered, they will be in self->GetUpdateDelay seconds.

With that said, the class could keep track of some bool/state and check it before creating a runnable or attempting to update/delayed update.

@@ +303,5 @@
>        NS_NewRunnableFunction(__func__, [self, promise]() mutable {
> +        uint32_t delay = self->GetUpdateDelay();
> +        if (delay) {
> +          nsCOMPtr<nsITimerCallback> delayedCallback =
> +              new DelayedUpdateCallback(self, promise);

Could be new DelayedUpdateCallback(std::move(self), std::move(promise)) (see comment in the constructor).

@@ +313,5 @@
> +            return;
> +          }
> +          return;
> +        }
> +        self->UpdateOnMainThread(promise);

Could be self->UpdateOnMainThread(std::move(promise)) (see the comment in ServiceWorkerRegistrationProxy::UpdateOnMainThread's body).

::: dom/serviceworkers/ServiceWorkerRegistrationProxy.h
@@ +75,4 @@
>  
>    RefPtr<ServiceWorkerRegistrationPromise> Update();
>  
> +  void UpdateOnMainThread(ServiceWorkerRegistrationPromise::Private* aPromise);

(See the comment in ServiceWorkerRegistrationProxy::UpdateOnMainThread's body.)
Comment on attachment 9036340 [details] [diff] [review]
Implement delay update mechanism in ServiceWorkerRegistrationProxy

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

Looks good in terms of delaying updates. There are just some miscellaneous things to remove and possibly parameters to make more consistent with rvalue refs/raw pointers.

::: dom/serviceworkers/ServiceWorkerRegistrationProxy.cpp
@@ +301,2 @@
>  
>    nsCOMPtr<nsIRunnable> r =

I misread the bug (oops), ignore what I said above
(Assignee)

Comment 7

4 months ago

Update patch according to the feedbacks.

Attachment #9036340 - Attachment is obsolete: true
(Assignee)

Comment 8

4 months ago

To not expose any new public member function in ServiceWorkerRegistrationProxy, declaring DelayedUpdateCallback as a private member class of ServiceWorkerRegistrationProxy, such that we can keep ServiceWorkerRegistrationProxy's encapsulation, and also let DelayedUpdateCallback can access ServiceWorkerRegistrationProxy's private members what needed to perform "update".

Attachment #9036612 - Attachment is obsolete: true
(Assignee)

Comment 10

4 months ago
Attachment #9036911 - Attachment is obsolete: true
Attachment #9038231 - Flags: review?(bugmail)
(Assignee)

Updated

4 months ago
Attachment #9038231 - Attachment is obsolete: true
Attachment #9038231 - Flags: review?(bugmail)
(Assignee)

Comment 11

4 months ago
  1. Add a new private member class DelayedUpdateCallback in
    ServiceWorkerRegistrationProxy for handling the deley time up.

  2. Modify the lambda function in ServiceWorkerRegistrationProxy::Update(). Get
    the update delay time at first. If the delay time does not equal zero. create
    a timer and a timer callback(DelayedUpdateCallback) to perform the delay
    update. Otherwise execute update directly.

(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 12

2 months ago

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9e31edcc25a
Implement delay update mechanism in ServiceWorkerRegistrationProxy. r=asuth

Keywords: checkin-needed

Comment 13

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.