1469566 - Port 2 ServiceWorker WorkerHolders to WorkerRef

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - LifeCycleEventWatcher

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - LifeCycleEventWatcher

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - LifeCycleEventWatcher

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Cristina Coroiu [:ccoroiu]]

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

Comment 8

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 85d2aec2f4b6 (bug 1469566) for multiple failures at dom/serviceworkers/test/test_install_event_gc.html 

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

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

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3dbf24100b1a
https://hg.mozilla.org/mozilla-central/rev/af5830560a76