Port 2 ServiceWorker WorkerHolders to WorkerRef

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

No description provided.
Attachment #8986179 - Flags: review?(bkelly)
Attachment #8986182 - Flags: review?(bkelly)
Priority: -- → P2
Comment on attachment 8986179 [details] [diff] [review]
part 1 - LifeCycleEventWatcher

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

::: dom/serviceworkers/ServiceWorkerPrivate.cpp
@@ +794,5 @@
>        ReportResult(false);
>        return false;
>      }
>  
> +    mWorkerRef = std::move(workerRef);

nit: It seems like you could just assign to the mWorkerRef to start above.

@@ +806,2 @@
>  
> +    if (!mWorkerRef) {

This seems broken.  You call ReportResult(false) if you fail to create the worker ref.  But then it doesn't do anything.  I think you need to keep mDone.

@@ +810,3 @@
>  
>      mCallback->SetResult(aResult);
> +    nsresult rv = mWorkerRef->Private()->DispatchToMainThread(mCallback);

If there is no mWorkerRef you could dispatch to SystemGroup instead.
Attachment #8986179 - Flags: review?(bkelly) → review-
Comment on attachment 8986182 [details] [diff] [review]
part 2 - LifeCycleEventWatcher

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

r=me with comment addressed.

::: dom/serviceworkers/ServiceWorkerPrivate.cpp
@@ +302,5 @@
>    {
> +    MOZ_ASSERT(IsCurrentThreadRunningWorker());
> +
> +    RefPtr<KeepAliveHandler> self = this;
> +    RefPtr<StrongWorkerRef> workerRef =

It seems you don't save this to mWorkerRef anywhere.
Attachment #8986182 - Flags: review?(bkelly) → review+
Attachment #8986179 - Attachment is obsolete: true
Attachment #8986187 - Flags: review?(bkelly)
Attachment #8986187 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85d2aec2f4b6
Port 2 ServiceWorker WorkerHolders to WorkerRef - part 1, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/258a2da40ef0
Port 2 ServiceWorker WorkerHolders to WorkerRef - part 2, r=bkelly
Backed out changeset 258a2da40ef0 (bug 1469566) for failing dom/push/test/test_subscription_change.html 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bad496f81b6a56078d01836bca5a902a68aeb5

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=258a2da40ef0bdbd538eddb4680fc1b511875b88

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183828557&repo=mozilla-inbound&lineNumber=9579

[task 2018-06-19T17:54:14.229Z] 17:54:14     INFO - TEST-START | dom/push/test/test_subscription_change.html
[task 2018-06-19T17:59:39.651Z] 17:59:39     INFO - TEST-INFO | started process screentopng
[task 2018-06-19T17:59:40.157Z] 17:59:40     INFO - TEST-INFO | screentopng: exit 0
[task 2018-06-19T17:59:40.158Z] 17:59:40     INFO - Buffered messages logged at 17:54:14
[task 2018-06-19T17:59:40.159Z] 17:59:40     INFO - AddTask.js | Entering test start
[task 2018-06-19T17:59:40.160Z] 17:59:40     INFO - Buffered messages finished
[task 2018-06-19T17:59:40.160Z] 17:59:40     INFO - TEST-UNEXPECTED-FAIL | dom/push/test/test_subscription_change.html | Test timed out. 
[task 2018-06-19T17:59:40.161Z] 17:59:40     INFO -     reportError@SimpleTest/TestRunner.js:121:7
[task 2018-06-19T17:59:40.162Z] 17:59:40     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2018-06-19T17:59:40.163Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.163Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.164Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.165Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.166Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.166Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.166Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.166Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     hookupTests@SimpleTest/setup.js:266:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO -     hookup@SimpleTest/setup.js:246:5
[task 2018-06-19T17:59:40.167Z] 17:59:40     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp&cleanupCrashes=true:11:1
[task 2018-06-19T17:59:40.677Z] 17:59:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-06-19T17:59:40.678Z] 17:59:40     INFO - TEST-UNEXPECTED-FAIL | dom/push/test/test_subscription_change.html | This test left a service worker registered without cleaning it up
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dbf24100b1a
Port ServiceWorkers to WorkerRef - part 1 - LifeCycleEventWatcher, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/af5830560a76
Port ServiceWorkers to WorkerRef - part 2 - KeepAliveHandler, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/3dbf24100b1a
https://hg.mozilla.org/mozilla-central/rev/af5830560a76
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.