Closed Bug 1469181 Opened 4 years ago Closed 3 years ago

Port Notification API to WorkerRef

Categories

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

enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1559919

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(8 files)

No description provided.
Attachment #8985802 - Flags: review?(mrbkap)
Attachment #8985803 - Flags: review?(mrbkap)
There are no reasons to keep this temporary reference when we can pass it as argument.
Attachment #8985804 - Flags: review?(mrbkap)
Moving Notification::GetPrincipal in the NotificationRef is a first step to remove WorkerPrivate. This patch makes sense with all the rest.
Attachment #8985805 - Flags: review?(mrbkap)
The following patches require RefPtr<NotificationRef>.
Attachment #8985806 - Flags: review?(mrbkap)
Attachment #8985807 - Flags: review?(mrbkap)
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.
Attachment #8985808 - Flags: review?(mrbkap)
Attached patch part 8 - testSplinter Review
We have good tests for this API but I needed to add 1 where the worker is terminated when the notification is shown.
Attachment #8985809 - Flags: review?(mrbkap)
Attachment #8985802 - Flags: review?(mrbkap) → review+
Comment on attachment 8985803 [details] [diff] [review]
part 2 - Dead code

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

Good catch!
Attachment #8985803 - Flags: review?(mrbkap) → review+
Attachment #8985804 - Flags: review?(mrbkap) → review+
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?
Attachment #8985805 - Flags: review?(mrbkap) → review+
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.
Attachment #8985806 - Flags: review?(mrbkap) → review+
Attachment #8985807 - Flags: review?(mrbkap) → review+
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.
Attachment #8985809 - Flags: review?(mrbkap) → review+
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".
Attachment #8985808 - Flags: review?(mrbkap) → review+
Pushed by amarchesini@mozilla.com:
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
: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
Priority: -- → P2
Component: DOM → DOM: Core & HTML
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(amarchesini)
Resolution: --- → DUPLICATE
Duplicate of bug: 1559919
You need to log in before you can comment on or make changes to this bug.