Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 9 years ago

      No description provided.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 1 • 9 years ago

/r/8285 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/8287 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/8289 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/8291 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/8293 - getNotifications() on worker thread
/r/8295 - ServiceWorkerRegistration.getNotifications() tests

Pull down these commits:

hg pull -r 3d0f232588f28a06d450d4fe8fccf879596a9ba0 https://reviewboard-hg.mozilla.org/gecko/

	William Chen [:wchen]
	Comment 2 • 9 years ago

https://reviewboard.mozilla.org/r/8285/#review7573

::: dom/base/nsGkAtomList.h:825
(Diff revision 1)
> +GK_ATOM(onnotificationclick, "notificationclick")

notificationclick -> onnotificationclick

::: dom/webidl/PushManager.webidl:10
(Diff revision 1)
> -[JSImplementation="@mozilla.org/push/PushManager;1",
> +[JSImplementation="@mozilla.org/push/PushManager;1"]

Part of a different patch?

	William Chen [:wchen]
	Comment 3 • 9 years ago

https://reviewboard.mozilla.org/r/8287/#review7679

::: dom/notification/Notification.cpp:197
(Diff revision 1)
> +    //XXXnsm Is it guaranteed mCallback will be called in case of failure?

I don't think you can make any guarantees about mCallback being called, there are a few places where the code can fail before getting to the callback. In fact, if the call to Get fails here, then mCallback is probably not going to be called.

::: dom/notification/Notification.cpp:1687
(Diff revision 1)
> +    p->MaybeReject(result);

It looks like we should return here.

::: dom/notification/Notification.cpp:1668
(Diff revision 1)
> +  NotificationPermission permission = NotificationPermission::Denied;

This block of code is almost identical to Notification::GetPermission, it may be worthwhile to factor it out. Also, will this ever be executed on main thread given that this is a service worker API?

::: dom/notification/Notification.h:250
(Diff revision 1)
> +  // Callers MUST bind the Notification to the correct DETH parent using

DETH?

::: dom/notification/Notification.h:335
(Diff revision 1)
> +  CreateAndShowUnsafe(nsIGlobalObject* aGlobal,

I'm not sure I see why this is unsafe or why we have a restriction against non-stack references. Once we return to reference to script, we can't stop it from getting put on the heap. Also, my understanding of NotificationRef is that it is meant to make lifecycle management a little easier within a function (so a function doesn't try to manually handle references after it passed ownership to a runnable or observer), rather than making uniquely owned references on the heap (because that's not going to happen when we give a reference to script).

::: dom/notification/NotificationStorage.js:202
(Diff revision 1)
> -        callback.handle(notification.id,
> +        Services.tm.currentThread.dispatch(

Perhaps add a comment that you're making this asynchronous to match the asynchronous behavour of fetching from the database.

::: dom/notification/Notification.cpp
(Diff revision 1)
> -    BindToOwner(window);

Is it necessary to remove this? It seems like all we've done is just moved BindToOwner to all the main thread callers.

::: dom/notification/Notification.h:294
(Diff revision 1)
> +    MOZ_ASSERT(!mAlertName.IsEmpty());

It looks like we are only using this method on the main thread so how about we get rid of SetAlertName and set it the first time we call GetAlertName.

	William Chen [:wchen]
	Comment 4 • 9 years ago

https://reviewboard.mozilla.org/r/8289/#review7727

In general, there are a few fixmes and tab characters that should be fixed up in the uploaded patches.

::: dom/notification/NotificationStorage.js:7
(Diff revision 1)
> -const DEBUG = false;
> +const DEBUG = true;

This should be changed back to DEBUG = false

::: dom/notification/Notification.cpp:1324
(Diff revision 1)
> +    // Notification and can release ownership at the end of this function.

Instead of saying that we don't care about the Notification, how about just saying that the observer does not need a reference to the Notification.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 5 • 9 years ago

https://reviewboard.mozilla.org/r/8287/#review7955

> It looks like we are only using this method on the main thread so how about we get rid of SetAlertName and set it the first time we call GetAlertName.

I've kept the function, but it is only called by GetAlertName().

> This block of code is almost identical to Notification::GetPermission, it may be worthwhile to factor it out. Also, will this ever be executed on main thread given that this is a service worker API?

This is a Service Worker API in the sense of being associated with SWs. The actual API is exposed on ServiceWorkerRegistration which is available on all threads.

> Is it necessary to remove this? It seems like all we've done is just moved BindToOwner to all the main thread callers.

I don't remember exactly why I had made these main thread only, it is likely that some version of my changes required that. I'll push a followup if this change isn't needed after i look at the rest of the patches.

> I'm not sure I see why this is unsafe or why we have a restriction against non-stack references. Once we return to reference to script, we can't stop it from getting put on the heap. Also, my understanding of NotificationRef is that it is meant to make lifecycle management a little easier within a function (so a function doesn't try to manually handle references after it passed ownership to a runnable or observer), rather than making uniquely owned references on the heap (because that's not going to happen when we give a reference to script).

I get what you mean now. The intention was "Don't hold weak/raw references to this Notification, since it may go away". I have modified the comment and method name.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 9 years ago

https://reviewboard.mozilla.org/r/8289/#review7981

My hg export isn't showing any bad whitespace or FIXMEs :/ Was this comment for patch 3?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 7 • 9 years ago

Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/8285 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/8287 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/8289 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/8291 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/8293 - Bug 1114554 - Patch 5 - getNotifications() on worker thread.
/r/8295 - Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.

Pull down these commits:

hg pull -r d6263edb1339aa78cdf29ee812de79493d966a8f https://reviewboard-hg.mozilla.org/gecko/

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/8285 - Bug 1114554 - Patch 7 - Call BindToOwner on all threads

Pull down this commit:

hg pull -r 350f7ab964f833300e889e6bcf0e3b75749b5732 https://reviewboard-hg.mozilla.org/gecko/

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/9293 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/9295 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/9297 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/9299 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/9301 - Bug 1114554 - Patch 5 - getNotifications() on worker thread.
/r/9303 - Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.
/r/8285 - Bug 1114554 - Patch 7 - Call BindToOwner on all threads

Pull down these commits:

hg pull -r 350f7ab964f833300e889e6bcf0e3b75749b5732 https://reviewboard-hg.mozilla.org/gecko/

	William Chen [:wchen]
	Comment 10 • 9 years ago

https://reviewboard.mozilla.org/r/9293/#review9193

	William Chen [:wchen]
	Comment 11 • 9 years ago

https://reviewboard.mozilla.org/r/9295/#review9187

::: dom/notification/Notification.cpp:1664
(Diff revision 1)
> +  // FIXME(nsm): ensure aGlobal's principal is allowed to load scope.

This FIXME

::: dom/workers/ServiceWorkerRegistration.cpp:986
(Diff revision 1)
> -  MOZ_ASSERT(false);
> +  // FIXME(nsm): Check for active worker.

This FIXME

::: dom/notification/Notification.cpp:701
(Diff revision 1)
> -  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> +  // FIXME(nsm): If the sticky flag is set, throw an error.

I was referring to these FIXMEs. It looks like we don't support "sticky" yet, so that comment is OK. It looks like you already took care of the next FIXME so you should remove it.

This patch looks like it has some tab characters in it.

	William Chen [:wchen]
	Comment 12 • 9 years ago

https://reviewboard.mozilla.org/r/9297/#review9201

	William Chen [:wchen]
	Comment 13 • 9 years ago

https://reviewboard.mozilla.org/r/9299/#review9203

	William Chen [:wchen]
	Comment 14 • 9 years ago

https://reviewboard.mozilla.org/r/9301/#review9195

::: dom/notification/Notification.cpp:1808
(Diff revision 1)
> +      // FIXME(nsm): Handle this and other failures.

This FIXME. Although it looks like you're already handling errors so maybe you just forgot to remove the comment.

	William Chen [:wchen]
	Comment 15 • 9 years ago

https://reviewboard.mozilla.org/r/9303/#review9199

Update the XXXXXX to the correct bug number.

	William Chen [:wchen]
	Comment 16 • 9 years ago

https://reviewboard.mozilla.org/r/8285/#review9191

::: dom/notification/Notification.cpp:781
(Diff revision 3)
> -    notification->BindToOwner(aGlobal);
> +  notification->BindToOwner(aGlobal);

Can we do this in the notification constructor?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 30 • 9 years ago

Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 31 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 32 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 33 • 9 years ago

Comment on attachment 8618953 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 34 • 9 years ago

Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

Bug 1114554 - Patch 5 - getNotifications() on worker thread.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 35 • 9 years ago

Comment on attachment 8618955 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 36 • 9 years ago

Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

Bug 1114554 - Patch 7 - Call BindToOwner on all threads

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 37 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 38 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 39 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 40 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 41 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 42 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 43 • 9 years ago

Something is wrong with RB, but please review patches 2, 5, and 7.

	William Chen [:wchen]
	Comment 44 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

https://reviewboard.mozilla.org/r/9295/#review9659

	William Chen [:wchen]
	Comment 45 • 9 years ago

Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

https://reviewboard.mozilla.org/r/9301/#review9671

::: dom/notification/Notification.cpp:1754
(Diff revisions 1 - 2)
>      MutexAutoLock lock(mPromiseProxy->GetCleanUpLock());

Let's also add MOZ_ASSERT(mPromiseProxy) here to make it easier to catch multiple calls to Done.

	William Chen [:wchen]
	Comment 46 • 9 years ago

Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

https://reviewboard.mozilla.org/r/8285/#review9673

::: dom/notification/Notification.h:292
(Diff revision 4)
>    // BindToOwner().

We can get rid of this comment now.

	Andrea Marchesini [:baku]
	Comment 48 • 9 years ago

Comment on attachment 8622682 [details] [diff] [review]
Patch 3.1 - ServiceWorker principal fixes.

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

::: dom/notification/Notification.cpp
@@ +1004,2 @@
>    {
>      AssertIsOnMainThread();

MOZ_ASSERT(aPrincipal)

@@ +1195,1 @@
>    {

MOZ_ASSERT(aPrincipal)
then if it's more than 80chars, new line.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 49 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=975170035cf0

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 50 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c0faf010556

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 51 • 9 years ago

Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 52 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 53 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen


Bug 1114554 - Patch 3.1 - ServiceWorker principal fixes. r=baku

Bug 1162088 introduced origin attributes that ServiceWorkerManager callers have to use. This patch updates notificationclick events to work.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 54 • 9 years ago

Comment on attachment 8618953 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 55 • 9 years ago

Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 56 • 9 years ago

Comment on attachment 8618955 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 57 • 9 years ago

Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 58 • 9 years ago

Ehsan, could I just get dom peer sign off on this. Thanks

	Andrea Marchesini [:baku]
	Comment 59 • 9 years ago

Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

https://reviewboard.mozilla.org/r/9297/#review10457

I just reviewed the WebIDL file and it looks good to me.

::: dom/notification/NotificationEvent.h:63
(Diff revision 3)
> +  {

This can be:

Notification* Notification_() const
{
  return mNotification;
}

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 60 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc25f539b1f

	Andrea Marchesini [:baku]
	Comment 61 • 9 years ago

Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

https://reviewboard.mozilla.org/r/9295/#review10477

Ship It!

	Andrea Marchesini [:baku]
	Comment 62 • 9 years ago

Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

https://reviewboard.mozilla.org/r/9293/#review10479

Ship It!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 63 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d860fd12534
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8a768782f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/75324b6862a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f92abe5ec784
https://hg.mozilla.org/integration/mozilla-inbound/rev/15bdf9c78e6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/404830c1ecf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5578d5c280a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/247ca4bf258e

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 64 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cbf74b9d533

	Robert Bindar
	Comment 65 • 9 years ago

We crashed when a notification was removed and the application that created the notification had previously been killed.
It happened only in b2g-desktop. This fixes the crash and we need it until we remove the close event from implementation.

Nikhil I assigned you as a reviewer, but if you want, you can forward it to someone else.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 66 • 9 years ago

Quoting email from Robert about why this check is enough to prevent the crash. Note that this issue affects b2g-desktop (where the same process seems to be used for apps) and is more of a testing issue than something that could happen in production.

"""The thing is if it's the case of b2g where we have an app as a different process,
AlertsHelper.jsm is gonna see the listener as invalid (the process was killed right?) and unpersist the notification by itself from the parent process (and it's going to send a system message to start the app and notify it about the close event, but this is not gonna be the case in the nearest future).

If it's the case of b2g-desktop then the listener is gonna be seen as valid from AlertsHelper.jsm 
(AlertsService.js does not just die right away after the app's window is killed -- b2g-desktop is using the same process for its apps--), so an async message is sent from AlertsHelper.jsm to AlertsService.js, then

AlertsService.js is trying to notify the oberver by calling .observe("alertfinished"), this is where we crashed because the window is not valid anymore, but the observer was still alive (maybe still not GC'd yet?).
If we throw an error from Notification.cpp in this use-case, AlertsService.js is gonna catch the exception and unpersist the notification by itself."""

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 67 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7e527b495b6

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 68 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f2bd48a54a
https://hg.mozilla.org/integration/mozilla-inbound/rev/efc270ffbd34
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e06a7f9dbb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c560af0970a
https://hg.mozilla.org/integration/mozilla-inbound/rev/315e191855b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab42661d239
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2f2f3f4c4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a737b7b67901
https://hg.mozilla.org/integration/mozilla-inbound/rev/deff1c84785f

	Ryan VanderMeulen [:RyanVM]
	Comment 69 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/22f2bd48a54a
https://hg.mozilla.org/mozilla-central/rev/efc270ffbd34
https://hg.mozilla.org/mozilla-central/rev/2e06a7f9dbb0
https://hg.mozilla.org/mozilla-central/rev/8c560af0970a
https://hg.mozilla.org/mozilla-central/rev/315e191855b2
https://hg.mozilla.org/mozilla-central/rev/dab42661d239
https://hg.mozilla.org/mozilla-central/rev/1f2f2f3f4c4d
https://hg.mozilla.org/mozilla-central/rev/a737b7b67901
https://hg.mozilla.org/mozilla-central/rev/deff1c84785f

	Chris Mills [:cmills]
	Comment 70 • 9 years ago

Relevant compat information added to:

https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/onnotificationclick
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/getNotifications
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/showNotification

Also needs to be added to Firefox 42 for developers, when Jean-Yves creates it, so I've not ddc'd the bug yet.

	Jean-Yves Perrier [:teoli]
	Comment 71 • 9 years ago

Added https://developer.mozilla.org/en-US/Firefox/Releases/42#Service_Workers

	Florian Bender
	Comment 72 • 9 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: "Real" Push Notifications for Websites available in your favourite browser!
[Suggested wording]: Implemented Notifications API for Service Workers
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API

	Daniel Holbert [:dholbert]
	Comment 73 • 9 years ago

(In reply to William Chen [:wchen] from comment #4)
> ::: dom/notification/NotificationStorage.js:7
> (Diff revision 1)
> > -const DEBUG = false;
> > +const DEBUG = true;
> 
> This should be changed back to DEBUG = false

This doesn't seem to have happened -- the patch that landed here included this DEBUG = true change:
 http://hg.mozilla.org/mozilla-central/rev/2e06a7f9dbb0#l7.10

nsm, can that change be reverted?

(I ran across this because I saw some various NotificationStorage.js terminalspew for various things on mail.google.com today, in up-to-date Nightly with my normal browsing profile. I dug in to find where that logging was coming from, and ran across this DEBUG variable which seems to control logging in this file.)

	Daniel Holbert [:dholbert]
	Comment 75 • 9 years ago

Comment on attachment 8660979 [details] [diff] [review]
disable debugging

Looks good to me - thanks!

(One optional nit: in the commit message, maybe s/debugging/debug logging/? Unless there's further stuff [beyond logging] that this variable controls.)

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 76 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d862877309f

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 77 • 9 years ago

Only this new patch needs to land

	Pulsebot
	Comment 78 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9738f1b3f11a

	Carsten Book [:Tomcat]
	Comment 79 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/9738f1b3f11a

	Sylvestre Ledru [:Sylvestre]
	Comment 80 • 9 years ago

This has been mentioned in the 42 release notes with "Ship Push messaging with disabled web notifications from ServiceWorkers" as wording. I moved it to 43 as it has been disabled in 42 by bug 1208560.