1469181 - Port Notification API to WorkerRef

Status: RESOLVED DUPLICATE of bug 1559919

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - owning thread

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Dead code

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - no temp NotificationRef

There are no reasons to keep this temporary reference when we can pass it as argument.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - GetPrincipal

Moving Notification::GetPrincipal in the NotificationRef is a first step to remove WorkerPrivate. This patch makes sense with all the rest.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - RefPtr for NotificationRef

The following patches require RefPtr<NotificationRef>.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - Move more methods

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 7 - WorkerRef

I don't know how to split in smaller patches. Here there is a huge refactoring of Notification API. I wrote a long comment, I hope it can be useful.

The idea is:

0. NotificationTask -> NotificationTaskRunnable and make it a simple runnable.

1. NotificationRef -> NotificationTask. all the logic goes here.

2. Notification -> Simple object with just what is exposed to content.

3. a cycle between observer and NotificationTask

4. WorkerRef.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 8 - test

We have good tests for this API but I needed to add 1 where the worker is terminated when the notification is shown.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8985803 [details] [diff] [review]
part 2 - Dead code

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

Good catch!

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8985805 [details] [diff] [review]
part 4 - GetPrincipal

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

::: dom/notification/Notification.cpp
@@ +1810,4 @@
>        return NotificationPermission::Denied;
>      }
>  
> +    nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();

Do we actually need the strong ref here?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8985806 [details] [diff] [review]
part 5 - RefPtr for NotificationRef

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

::: dom/notification/Notification.h
@@ +327,5 @@
>                                                         const NotificationOptions& aOptions);
>  
>    nsresult Init();
>    bool IsInPrivateBrowsing();
> +  void ShowInternal(already_AddRefed<NotificationRef> aRef);

I was wondering if there was any guidance in the style guide about whether it's better to take an rvalue-reference (already_AddRefed<NotificaitonRef>&&) and force the callers to either provide an rvalue-reference or to do this and take a copy in order to avoid having to std::move in the implementation. I think that with already_AddRefed<T>, because it disallows copy-construction, it doesn't matter either way.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8985808 [details] [diff] [review]
part 7 - WorkerRef

I'm going to re-read this after lunch before stamping -- I think it looks fine but I want to make sure I understand what's going on and how it works.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8985808 [details] [diff] [review]
part 7 - WorkerRef

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

I have a few nitpicky questions. Overall, this is so much cleaner than the existing code!

::: dom/notification/Notification.cpp
@@ +552,5 @@
> +  nsresult
> +  DispatchToMainThread(already_AddRefed<nsIRunnable>&& aRunnable)
> +  {
> +    if (mWorkerRef) {
> +      return mWorkerRef->Private()->DispatchToMainThread(std::move(aRunnable));

Are these std::move calls necessary? I thought std::move simply casted (with some hand-waving for edge cases) to T&& anyway.

@@ +2169,5 @@
>    }
>  
> +  nsCOMPtr<nsIRunnable> r =
> +    new NotificationTaskRunnable("Notification::Close", task);
> +  nsresult rv = task->DispatchToMainThread(r.forget());

Question: Is it clearer now to use do_AddRef here instead of an explicit nsCOMPtr? Or is that more confusing?

nsresult rv = task->DispatchToMainThread(do_AddRef(new NotificationTaskRunnable(...));

@@ +2174,5 @@
>  
>    if (NS_FAILED(rv)) {
>      DispatchTrustedEvent(NS_LITERAL_STRING("error"));
> +    // If dispatch fails, NotificationTaskRunnable will release the task when it
> +    // goes out of scope at the end of this function.

This comment is a bit misleading now: it's possible for the runnable to have been deleted already since we gave our reference away to `DispatchToMainThread`.

@@ +2547,5 @@
> +
> +  RefPtr<StrongWorkerRef> workerRef =
> +    StrongWorkerRef::Create(GetCurrentThreadWorkerPrivate(), "Notification",
> +    [task]() {
> +      // When the worker is shutting down, just for eShow action, we must break

Nit: either "just for the eShow action" or "just for eShow".

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66de682a9c1b
Port Notification API to WorkerRef - part 1 - owning thread assertions, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef39038ae70e
Port Notification API to WorkerRef - part 2 - remove dead code, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c780655f6d1
Port Notification API to WorkerRef - part 3 - no temp NotificationRef, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/5665189c4b00
Port Notification API to WorkerRef - part 4 - Move GetPrincipal into NotificationRef, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/6888cf7b5253
Port Notification API to WorkerRef - part 5 - RefPtr for NotificationRef, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e4770914d7
Port Notification API to WorkerRef - part 6 - Move some methods into NotificationRef, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/289ce1ae93b6
Port Notification API to WorkerRef - part 7 - WorkerRef + NotificationTask, r=mrbkap

Comment 15

[Narcis Beleuzu [:NarcisB]]

Backed out for several crashes on:

1. [@ JS::ReadableStreamDefaultReaderRead(JSContext*, JS::Handle<JSObject*>)] - Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=183720042&repo=mozilla-inbound&lineNumber=1401
2. [@ js::jit::ReleaseProcessExecutableMemory()] - Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=183718294&repo=mozilla-inbound&lineNumber=13908
3. [@ mozilla::dom::WorkerNotificationObserver::Observe(nsISupports*, char const*, char16_t const*)] - Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=183717178&repo=mozilla-inbound&lineNumber=3284

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/81bb56f69788224c30d62dc42e60004ecb5fc860
Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=289ce1ae93b6c5f08f3e65c1997312bd901b101c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-classifiedState=unclassified

Comment 16

[Narcis Beleuzu [:NarcisB]]

:baku, there are also memory leaks -> https://treeherder.mozilla.org/logviewer.html#?job_id=183721564&repo=mozilla-inbound&lineNumber=3315

Relevant log data:

[task 2018-06-19T00:37:19.042Z] 00:37:19     INFO - GECKO(1971) | ###!!! [Child][MessageChannel] Error: (msgtype=0x2D0107,name=PContent::Msg_AsyncMessage) Closed channel: cannot send/recv
[task 2018-06-19T00:37:19.042Z] 00:37:19     INFO - GECKO(1971) | JavaScript error: jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/NotificationStorage.js, line 148: NS_ERROR_UNEXPECTED:
[task 2018-06-19T00:37:19.650Z] 00:37:19     INFO - GECKO(1971) | 1529368639645	Marionette	DEBUG	Received observer notification xpcom-will-shutdown
[task 2018-06-19T00:37:19.650Z] 00:37:19     INFO - GECKO(1971) | 1529368639645	Marionette	INFO	Stopped listening on port 2828
[task 2018-06-19T00:37:19.650Z] 00:37:19     INFO - GECKO(1971) | 1529368639647	Marionette	DEBUG	Remote service is inactive
[task 2018-06-19T00:37:20.561Z] 00:37:20     INFO - GECKO(1971) | -----------------------------------------------------
[task 2018-06-19T00:37:20.561Z] 00:37:20     INFO - GECKO(1971) | Suppressions used:
[task 2018-06-19T00:37:20.562Z] 00:37:20     INFO - GECKO(1971) |   count      bytes template
[task 2018-06-19T00:37:20.562Z] 00:37:20     INFO - GECKO(1971) |     638      20352 nsComponentManagerImpl
[task 2018-06-19T00:37:20.562Z] 00:37:20     INFO - GECKO(1971) |       3        624 mozJSComponentLoader::LoadModule
[task 2018-06-19T00:37:20.562Z] 00:37:20     INFO - GECKO(1971) |       2        288 libfontconfig.so
[task 2018-06-19T00:37:20.562Z] 00:37:20     INFO - GECKO(1971) | -----------------------------------------------------
[task 2018-06-19T00:37:32.363Z] 00:37:32     INFO - GECKO(1971) | =================================================================
[task 2018-06-19T00:37:32.364Z] 00:37:32    ERROR - GECKO(1971) | ==2025==ERROR: LeakSanitizer: detected memory leaks