Closed Bug 1472307 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Service Workers, enhancement, P2)

Product:
Core
Component:
DOM: Service Workers
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: bkelly, Assigned: edenchuang)

References

Details

(Whiteboard: SW-MUST)

Attachments

(1 file, 4 obsolete files)

Implement delay update mechanism in ServiceWorkerRegistrationProxy.
47 bytes, text/x-phabricator-request
Details | Review 
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.
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
Priority: -- → P2
Whiteboard: SW-MUST
Assignee: nobody → perry
Eden, would you mind taking a look at this?
Assignee: perry → echuang
Sure, going to start on it.

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

Could you give some feedbacks on it?

Thanks.

Attachment #9036340 - Flags: feedback?(perry)
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

Update patch according to the feedbacks.

Attachment #9036340 - Attachment is obsolete: true

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
Attachment #9036911 - Attachment is obsolete: true
Attachment #9038231 - Flags: review?(bugmail)
Attachment #9038231 - Attachment is obsolete: true
Attachment #9038231 - Flags: review?(bugmail)

  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.

Keywords: checkin-needed

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

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/e9e31edcc25a

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: