Closed
Bug 1472307
Opened 7 years ago
Closed 7 years ago
RemoteServiceWorkerRegistration::Update() does not implement delayed self-update mechanism
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla67
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: bkelly, Assigned: edenchuang)
References
Details
(Whiteboard: SW-MUST)
Attachments
(1 file, 4 obsolete files)
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•7 years 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•7 years ago
|
Priority: -- → P2
|
|
||
Updated•7 years ago
|
Whiteboard: SW-MUST
|
||
Updated•7 years ago
|
Assignee: nobody → perry
|
Assignee | |
Comment 3•7 years ago
|
||
Sure, going to start on it.
|
|
Assignee | |
Comment 4•7 years 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•7 years ago
|
Attachment #9036340 -
Flags: feedback?(perry) → feedback-
|
|
||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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•7 years ago
|
||
Update patch according to the feedbacks.
Attachment #9036340 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 8•7 years 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 9•7 years ago
|
||
mochitest result for the patch
|
|
Assignee | |
Comment 10•7 years ago
|
||
Attachment #9036911 -
Attachment is obsolete: true
Attachment #9038231 -
Flags: review?(bugmail)
|
|
Assignee | |
Updated•7 years ago
|
Attachment #9038231 -
Attachment is obsolete: true
Attachment #9038231 -
Flags: review?(bugmail)
|
|
Assignee | |
Comment 11•7 years ago
|
||
-
Add a new private member class DelayedUpdateCallback in
ServiceWorkerRegistrationProxy for handling the deley time up. -
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•7 years ago
|
Keywords: checkin-needed
|
||
Comment 12•7 years 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•7 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 7 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.
Description
•