899574 - Notification API follow-up: provide a way to get current Notification objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julien Wajsberg [:julienw]]

Thinking about the Firefox OS use cases, it looks like something is missing.

With the current implementation (that matches the API AFAIK), we create the Notification object using a normal constructor, it's displayed as soon as it's created (if the permission is granted), and we can programmatically close it by calling `close` on the very same object.

However, in real cases, we may display the notification in one app instance, and close the notification in another app instance. I mean that a notification might be long-lived, survive app restart, and even survive reboot (see bug 881159 and bug 874364).

On the Desktop, it might survive a Firefox Desktop restart, or navigating away the website and coming back to it.

Probably the easier way to fix this is to have a static method to get the active/pending notification list for this website (using the origin or the page url as an internal key).

Anne, I'm sure you'll have wise ideas here :)

Comment 1

[Anne (:annevk)]

I added this to the API the other week: http://notifications.spec.whatwg.org/#dom-notification-get

Comment 2

[Julien Wajsberg [:julienw]]

Also remember that on Firefox OS, when the user taps a notification, a system message is sent to the app, and even the app is launched if necessary, and the message handler is given the notification object.

Comment 3

[Julien Wajsberg [:julienw]]

Anne> oh, nice to know, I was looking at http://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html

Are you aware of a bug to implement this in Gecko ?

Comment 4

[Anne (:annevk)]

Other than this bug, no.

Comment 5

[Julien Wajsberg [:julienw]]

need info wchen in case he wants to work on this.

Comment 6

[William Chen [:wchen]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> need info wchen in case he wants to work on this.

No, this isn't something I currently want to work on.

Comment 7

[Julien Wajsberg [:julienw]]

Gregor, if I understand correctly, I think this is part of your team now ?

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Gregor, if I understand correctly, I think this is part of your team now ?

Yes, this will need to be implemented as part of Bug 892528 and Bug 892530, both of which are slated to be finished with our current sprint.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notification-get.patch

WIP

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notification-get.patch

Added test suite around Notification API, as well as some tests. Moved old desktop notification tests into its own directory.

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notification-get.patch

WIP: make rename notificationService to notificationStorage, make sure notification.get returns a promise, add mochitest plain cases for notificationStorage

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch - Promise->Resolver()

This should be enough. Let me know if you need more.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: patch -> promise->MaybeResolve public

exposes promise resolve/reject

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 804300 [details] [diff] [review]
patch -> promise->MaybeResolve public

# HG changeset patch
# User Michael Henretty <[e-mail]>
# Date 1379060366 25200
# Node ID 40c76da479a3104c7f1497ef511364828112bf7d
# Parent  b9029b1de41096eebd6808f169464a5ee38215a4
[mq]: promise-public

diff --git a/dom/promise/Promise.cpp b/dom/promise/Promise.cpp
--- a/dom/promise/Promise.cpp
+++ b/dom/promise/Promise.cpp
@@ -184,16 +184,30 @@ Promise::EnabledForScope(JSContext* aCx,
     return workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker();
   }
 
   nsIPrincipal* prin = nsContentUtils::GetSubjectPrincipal();
   return nsContentUtils::IsSystemPrincipal(prin) ||
     prin->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED;
 }
 
+void
+Promise::MaybeResolve(JSContext* aCx,
+                      const Optional<JS::Handle<JS::Value> >& aValue)
+{
+  MaybeResolveInternal(aCx, aValue);
+}
+
+void
+Promise::MaybeReject(JSContext* aCx,
+                     const Optional<JS::Handle<JS::Value> >& aValue)
+{
+  MaybeRejectInternal(aCx, aValue);
+}
+
 static void
 EnterCompartment(Maybe<JSAutoCompartment>& aAc, JSContext* aCx,
                  const Optional<JS::Handle<JS::Value> >& aValue)
 {
   // FIXME Bug 878849
   if (aValue.WasPassed() && aValue.Value().isObject()) {
     JS::Rooted<JSObject*> rooted(aCx, &aValue.Value().toObject());
     aAc.construct(aCx, rooted);
@@ -224,19 +238,19 @@ Promise::JSCallback(JSContext *aCx, unsi
   if (aArgc) {
     value.Value() = args[0];
   }
 
   v = js::GetFunctionNativeReserved(&args.callee(), SLOT_TASK);
   PromiseCallback::Task task = static_cast<PromiseCallback::Task>(v.toInt32());
 
   if (task == PromiseCallback::Resolve) {
-    promise->MaybeResolve(aCx, value);
+    promise->MaybeResolveInternal(aCx, value);
   } else {
-    promise->MaybeReject(aCx, value);
+    promise->MaybeRejectInternal(aCx, value);
   }
 
   return true;
 }
 
 /* static */ JSObject*
 Promise::CreateFunction(JSContext* aCx, JSObject* aParent, Promise* aPromise,
                         int32_t aTask)
@@ -295,17 +309,17 @@ Promise::Constructor(const GlobalObject&
   aRv.WouldReportJSException();
 
   if (aRv.IsJSException()) {
     Optional<JS::Handle<JS::Value> > value(cx);
     aRv.StealJSException(cx, &value.Value());
 
     Maybe<JSAutoCompartment> ac;
     EnterCompartment(ac, cx, value);
-    promise->MaybeReject(cx, value);
+    promise->MaybeRejectInternal(cx, value);
   }
 
   return promise.forget();
 }
 
 /* static */ already_AddRefed<Promise>
 Promise::Resolve(const GlobalObject& aGlobal, JSContext* aCx,
                  JS::Handle<JS::Value> aValue, ErrorResult& aRv)
@@ -314,34 +328,34 @@ Promise::Resolve(const GlobalObject& aGl
   if (!window) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
 
   nsRefPtr<Promise> promise = new Promise(window);
 
   Optional<JS::Handle<JS::Value> > value(aCx, aValue);
-  promise->MaybeResolve(aCx, value);
+  promise->MaybeResolveInternal(aCx, value);
   return promise.forget();
 }
 
 /* static */ already_AddRefed<Promise>
 Promise::Reject(const GlobalObject& aGlobal, JSContext* aCx,
                 JS::Handle<JS::Value> aValue, ErrorResult& aRv)
 {
   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
   if (!window) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
 
   nsRefPtr<Promise> promise = new Promise(window);
 
   Optional<JS::Handle<JS::Value> > value(aCx, aValue);
-  promise->MaybeReject(aCx, value);
+  promise->MaybeRejectInternal(aCx, value);
   return promise.forget();
 }
 
 already_AddRefed<Promise>
 Promise::Then(const Optional<OwningNonNull<AnyCallback> >& aResolveCallback,
               const Optional<OwningNonNull<AnyCallback> >& aRejectCallback)
 {
   nsRefPtr<Promise> promise = new Promise(GetParentObject());
@@ -436,29 +450,29 @@ Promise::MaybeReportRejected()
     new AsyncErrorReporter(JS_GetObjectRuntime(&mResult.toObject()),
                            report,
                            nullptr,
                            nsContentUtils::GetObjectPrincipal(&mResult.toObject()),
                            win));
 }
 
 void
-Promise::MaybeResolve(JSContext* aCx,
+Promise::MaybeResolveInternal(JSContext* aCx,
                       const Optional<JS::Handle<JS::Value> >& aValue,
                       PromiseTaskSync aAsynchronous)
 {
   if (mResolvePending) {
     return;
   }
 
   ResolveInternal(aCx, aValue, aAsynchronous);
 }
 
 void
-Promise::MaybeReject(JSContext* aCx,
+Promise::MaybeRejectInternal(JSContext* aCx,
                      const Optional<JS::Handle<JS::Value> >& aValue,
                      PromiseTaskSync aAsynchronous)
 {
   if (mResolvePending) {
     return;
   }
 
   RejectInternal(aCx, aValue, aAsynchronous);
diff --git a/dom/promise/Promise.h b/dom/promise/Promise.h
--- a/dom/promise/Promise.h
+++ b/dom/promise/Promise.h
@@ -38,16 +38,21 @@ public:
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Promise)
 
   Promise(nsPIDOMWindow* aWindow);
   ~Promise();
 
   static bool PrefEnabled();
   static bool EnabledForScope(JSContext* aCx, JSObject* /* unused */);
 
+  void MaybeResolve(JSContext* aCx,
+                    const Optional<JS::Handle<JS::Value> >& aValue);
+  void MaybeReject(JSContext* aCx,
+                   const Optional<JS::Handle<JS::Value> >& aValue);
+
   // WebIDL
 
   nsPIDOMWindow* GetParentObject() const
   {
     return mWindow;
   }
 
   virtual JSObject*
@@ -109,20 +114,20 @@ private:
 
   void AppendCallbacks(PromiseCallback* aResolveCallback,
                        PromiseCallback* aRejectCallback);
 
   // If we have been rejected and our mResult is a JS exception,
   // report it to the error console.
   void MaybeReportRejected();
 
-  void MaybeResolve(JSContext* aCx,
+  void MaybeResolveInternal(JSContext* aCx,
                     const Optional<JS::Handle<JS::Value> >& aValue,
                     PromiseTaskSync aSync = AsyncTask);
-  void MaybeReject(JSContext* aCx,
+  void MaybeRejectInternal(JSContext* aCx,
                    const Optional<JS::Handle<JS::Value> >& aValue,
                    PromiseTaskSync aSync = AsyncTask);
 
   void ResolveInternal(JSContext* aCx,
                        const Optional<JS::Handle<JS::Value> >& aValue,
                        PromiseTaskSync aSync = AsyncTask);
 
   void RejectInternal(JSContext* aCx,

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: promise.patch

Sorry for the comment spam, I'm terrible at bugzilla apparently.

Comment 16

[Candice Serran (:cserran)]

Michael Henretty changed story state to started in Pivotal Tracker

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.patch

Another WIP patch, now with file persistence. Need to add more tests and cycle collection and we should be good.

Comment 18

[Gregor Wagner [:gwagner]]

Nice Work!

We should also deal with low memory situations and clear the cache if we get the corresponding event. Also grep for inner-window-destroyed and see if you have to clear anything when we destroy a window.

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.patch

Olli, I'm only r?'ing you for the Cycle Collection stuff on the NotificationStorageCallback class. The rest of these changes I'll have someone else like wchen look at.

Thanks!

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 807439 [details] [diff] [review]
notifications.patch

>+class NotificationStorageCallback MOZ_FINAL : public nsINotificationStorageCallback
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NotificationStorageCallback)
>+
>+  NotificationStorageCallback(const GlobalObject& aGlobal, nsPIDOMWindow* aWindow, Promise* aPromise)
>+    : mGlobal(aGlobal.Get()),
>+      mWindow(aWindow),
>+      mPromise(aPromise)
>+  {
>+    MOZ_ASSERT(aWindow);
>+    MOZ_ASSERT(aPromise);
>+    mCount = 0;
>+    JSContext* cx = aGlobal.GetContext();
>+    mNotifications = JS_NewArrayObject(cx, 0, nullptr)
I don't know why you need this, but if you try to keep JSObject* or JS::Value alive
you need to call mozilla::HoldJSObjects(this) or some such. That will make sure your object is traced.
You need to also call DropJSObjects(this) at some point. At least in destructor.

>+  NS_IMETHOD Done(JSContext* aCx) {
Note, { goes to the next line when defining a method.

In the unlink clear also all the JS member variables (mNotifications and mGlobal)


Btw Handle() method looks odd. You should usually keep reference to the native object (Notification in this case), 
not to the JS wrapper.

Comment 21

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.patch

Olli,

You were correct, adding DropData() to ~NotificationStorageCallback destructor fixed the crash we discussed.

(In reply to Olli Pettay [:smaug] from comment #20)
> Btw Handle() method looks odd. You should usually keep reference to the
> native object (Notification in this case), 
> not to the JS wrapper.

I am using the JS wrapper here because I could not figure out how to load these native Notifications objects into a JS Array that I could return to Content. I am open to suggestions here.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 807528 [details] [diff] [review]
notifications.patch

nsINotificationStorageCallback is something to be used only within Gecko, not exposed to web pages? (Since if it is exposed, webidl should be used.)

Giving an array of native objects to JS is a bit annoying in .idl.
One option is to use variant
http://mxr.mozilla.org/comm-esr17/source/mozilla/content/base/src/nsDOMMutationObserver.cpp?mark=581-608#581

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Olli Pettay [:smaug] from comment #22)\
> nsINotificationStorageCallback is something to be used only within Gecko,
> not exposed to web pages? (Since if it is exposed, webidl should be used.)

Yes, NotificationStorage (and it's associated callback class) will only be used internally by Gecko.

> Giving an array of native objects to JS is a bit annoying in .idl.
> One option is to use variant
> http://mxr.mozilla.org/comm-esr17/source/mozilla/content/base/src/
> nsDOMMutationObserver.cpp?mark=581-608#581

Cool, I'll look into this. Thanks!

Comment 24

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Notifications.get

Added guids for each notification. Made tag truly optional.

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.get with origin, and atomic read/write

After speaking with Sicking and Bent, it turns out there is no easy way to validate what origin a message comes from over the IPC wall. Since the parent notification DB should only return messages for a particular origin in Notification.get(), this poses a security problem for my patch. 

I have filed bug 919824 to make validating origin easier. But for the time being, we are going to insecurely pass the origin string in the message body, and simply trust this origin in the parent. I will file a follow up to fix this hole once bug 919824 lands.

Ben, wanna do a code review for this patch? Note: the cycle collector stuff has already been looked at by smaug.

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

We already prepend the origin as part of tag creation from any notification created by an app. Might be able to use that instead of having to also add it to the body?

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #26)
> We already prepend the origin as part of tag creation from any notification
> created by an app. Might be able to use that instead of having to also add
> it to the body?

True, we concatenate the origin with the tag when we create the alert message (http://hg.mozilla.org/mozilla-central/file/793b9afc6332/dom/src/notification/Notification.cpp#l534), but the tags themselves don't contain the origin. Since notificationStorage now needs the origin too, I abstracted out the origin fetching logic (notification->GetOrigin), so that both the alertService and notificationStorage can use it.

Comment 28

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #26)
> We already prepend the origin as part of tag creation from any notification
> created by an app. Might be able to use that instead of having to also add
> it to the body?

Also, when I say "add origin to the body," I mean IPC message body (as in https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#254), not the notification body.

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Geck Patch for Notification.get()

After review from Fabrice, we need to use ManifestURL instead of Origin when we have it (since b2g apps can sometimes share origins).

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 810133 [details] [diff] [review]
Geck Patch for Notification.get()

Re-adding bent as a reviewer after a sanity check from fabrice.

Comment 31

[Julien Wajsberg [:julienw]]

I think that nowadays b2g apps don't share origin unless they use entry points. But in that case they share manifest urls too ;)

In the Gaia System app, we used the tuple (manifestURL, entry point) as identifier (but sometimes in old code we just use the origin), but I don't know if that's sensible here.

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> I think that nowadays b2g apps don't share origin unless they use entry
> points. But in that case they share manifest urls too ;)

The issue is that the communications apps in gaia all share a single manifestURL, so the current grouping leads to all comms apps sharing notifications. However, after speaking to Julien and Fabrice about this on IRC, we decided that the ideal case is for each app to have its own manifestURL, and even if it has multiple entry points it should still use all the same notifications. So we will group by manifestURL for now, and deal with any bugs this causes in the comms apps as they come up.

Comment 33

[Andrea Marchesini [:baku]]

Comment on attachment 804303 [details] [diff] [review]
promise.patch

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

lgtm!

::: dom/promise/Promise.cpp
@@ +459,1 @@
>                        const Optional<JS::Handle<JS::Value> >& aValue,

indent

@@ +471,1 @@
>                       const Optional<JS::Handle<JS::Value> >& aValue,

indent

::: dom/promise/Promise.h
@@ +123,1 @@
>                      const Optional<JS::Handle<JS::Value> >& aValue,

indent

@@ +126,1 @@
>                     const Optional<JS::Handle<JS::Value> >& aValue,

indent

Comment 34

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Promises, add public api for resolving in c++

Fixed indent based on baku's comments.

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 810133 [details] [diff] [review]
Geck Patch for Notification.get()

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

This looks really good overall! We kinda threw you into the deep end with this assignment and you've done really well.

This will need some more work (especially on the queue in NotificationDB.jsm) but I think we're really close to being done here.

::: dom/interfaces/notification/nsINotificationStorage.idl
@@ +27,5 @@
> +interface nsINotificationStorage : nsISupports
> +{
> +
> +  /**
> +   * Add/replace a notification to the persistence layer.

We should expand these comments a bit to explain what the arguments mean.

@@ +48,5 @@
> +
> +  /**
> +   * Remove a notification from storage.
> +   */
> +  void delete(in DOMString origin, in DOMString id);

Nit: one arg per line

::: dom/src/notification/Notification.cpp
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "PCOMContentPermissionRequestChild.h"
>  #include "mozilla/dom/Notification.h"
> +#include "mozilla/dom/Promise.h"

Nit: Alphabetize

@@ +18,5 @@
>  #include "nsServiceManagerUtils.h"
>  #include "nsToolkitCompsCID.h"
>  #include "nsGlobalWindow.h"
>  #include "nsDOMJSUtils.h"
> +#include "nsCycleCollectionParticipant.h"

Nit: This shouldn't be needed since it's in your header already.

@@ +39,5 @@
> +      mPromise(aPromise)
> +  {
> +    MOZ_ASSERT(aWindow);
> +    MOZ_ASSERT(aPromise);
> +    mCount = 0;

Nit: This can be moved into the initializer list

@@ +45,5 @@
> +    mNotifications = JS_NewArrayObject(cx, 0, nullptr);
> +    HoldData();
> +  }
> +
> +  ~NotificationStorageCallback()

Make sure that all destructors for refcounted classes are protected or private.

@@ +58,5 @@
> +                    const nsAString& aBody,
> +                    const nsAString& aTag,
> +                    const nsAString& aIcon,
> +                    JSContext* aCx)
> +  {

MOZ_ASSERT(!aID.IsEmpty()), maybe aTitle too?

@@ +62,5 @@
> +  {
> +    // create the notification options
> +    NotificationOptions options;
> +    nsString dir(aDir);
> +    options.mDir = Notification::StringToDirection(dir);

Nit: s/dir/nsString(aDir)/

@@ +72,5 @@
> +                                                                       aID,
> +                                                                       aTitle,
> +                                                                       options);
> +    JSAutoCompartment ac(aCx, mGlobal);
> +    JS::Rooted<JSObject *> scope(aCx, mGlobal);

The JS folks just decided that we should use their fancy typedefs from here on out... JS::RootedObject here.

@@ +73,5 @@
> +                                                                       aTitle,
> +                                                                       options);
> +    JSAutoCompartment ac(aCx, mGlobal);
> +    JS::Rooted<JSObject *> scope(aCx, mGlobal);
> +    JSObject* element = notification->WrapObject(aCx, scope);

This can fail, you should bail out if it does.

@@ +74,5 @@
> +                                                                       options);
> +    JSAutoCompartment ac(aCx, mGlobal);
> +    JS::Rooted<JSObject *> scope(aCx, mGlobal);
> +    JSObject* element = notification->WrapObject(aCx, scope);
> +    JS::Rooted<JS::Value> handleElement(aCx, JS::ObjectValue(*element));

This should probably be:

  JS::RootedObject element(aCx, notification->WrapObject(aCx, scope));
  NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);

  if (!JS_DefineElement(..., JS::ObjectValue(element), ...) {
    return NS_ERROR_FAILURE;
  }

@@ +86,5 @@
> +  }
> +
> +  NS_IMETHOD Done(JSContext* aCx)
> +  {
> +    JSAutoCompartment ac(aCx, mNotifications);

Nit: Use mGlobal instead of mNotifications

@@ +87,5 @@
> +
> +  NS_IMETHOD Done(JSContext* aCx)
> +  {
> +    JSAutoCompartment ac(aCx, mNotifications);
> +    JS::Value value = OBJECT_TO_JSVAL(mNotifications);

JS::RootedValue, JS::ObjectValue instead of OBJECT_TO_JSVAL

@@ +88,5 @@
> +  NS_IMETHOD Done(JSContext* aCx)
> +  {
> +    JSAutoCompartment ac(aCx, mNotifications);
> +    JS::Value value = OBJECT_TO_JSVAL(mNotifications);
> +    Optional<JS::Handle<JS::Value> > result(aCx, value);

This should be Optional<JS::HandleValue>

@@ +89,5 @@
> +  {
> +    JSAutoCompartment ac(aCx, mNotifications);
> +    JS::Value value = OBJECT_TO_JSVAL(mNotifications);
> +    Optional<JS::Handle<JS::Value> > result(aCx, value);
> +    mPromise->MaybeResolve(aCx, result);

Should check this for failure.

@@ +107,5 @@
> +    mozilla::DropJSObjects(this);
> +  }
> +
> +  uint32_t  mCount;
> +  JS::Heap<JSObject *> mGlobal;

JS::HeapObject, and below.

@@ +395,5 @@
>  
>    return NS_OK;
>  }
>  
> +Notification::Notification(const nsString& aID, const nsAString& aTitle, const nsAString& aBody,

Nit: s/nsString/nsAString/

@@ +405,5 @@
>    SetIsDOMBinding();
>  }
>  
>  already_AddRefed<Notification>
>  Notification::Constructor(const GlobalObject& aGlobal,

Let's add a MOZ_ASSERT(NS_IsMainThread()) here.

Nit: Add a // static marker above.

@@ +420,5 @@
>  
>    // Queue a task to show the notification.
>    nsCOMPtr<nsIRunnable> showNotificationTask =
>      new NotificationTask(notification, NotificationTask::eShow);
>    NS_DispatchToMainThread(showNotificationTask);

Nit: change to NS_DispatchToCurrentThread

@@ +428,5 @@
> +  nsCOMPtr<nsINotificationStorage> notificationStorage =
> +    do_GetService(NS_NOTIFICATION_STORAGE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);
> +    return notification.forget();

Return nullptr here.

@@ +434,5 @@
> +
> +  nsString origin;
> +  aRv = GetOrigin(window, origin);
> +  if (aRv.Failed()) {
> +    return notification.forget();

nullptr

@@ +439,5 @@
> +  }
> +
> +  nsString id;
> +  notification->GetID(id);
> +  notificationStorage->Put(origin,

This can fail, please check the return value.

@@ +457,5 @@
> +                             const nsAString& aID,
> +                             const nsAString& aTitle,
> +                             const NotificationOptions& aOptions)
> +{
> +  nsString i;

Nit: id.

@@ +458,5 @@
> +                             const nsAString& aTitle,
> +                             const NotificationOptions& aOptions)
> +{
> +  nsString i;
> +  if (aID != EmptyString()) {

Nit: !aID.IsEmpty()

@@ +463,5 @@
> +    i = aID;
> +  } else {
> +    nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +      do_GetService("@mozilla.org/uuid-generator;1");
> +    MOZ_ASSERT(uuidgen);

This can't be asserted, just return a nullptr if uuidgen is null.

@@ +464,5 @@
> +  } else {
> +    nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +      do_GetService("@mozilla.org/uuid-generator;1");
> +    MOZ_ASSERT(uuidgen);
> +    nsID id;

Nit: uuid

@@ +465,5 @@
> +    nsCOMPtr<nsIUUIDGenerator> uuidgen =
> +      do_GetService("@mozilla.org/uuid-generator;1");
> +    MOZ_ASSERT(uuidgen);
> +    nsID id;
> +    uuidgen->GenerateUUIDInPlace(&id);

This can (unfortunately) fail. Check and bail if it does.

@@ +469,5 @@
> +    uuidgen->GenerateUUIDInPlace(&id);
> +
> +    char buffer[NSID_LENGTH];
> +    id.ToProvidedString(buffer);
> +    NS_ConvertUTF8toUTF16 convertedID(buffer);

Let's use NS_ConvertASCIItoUTF16

@@ +482,5 @@
> +                                                         aOptions.mTag,
> +                                                         aOptions.mIcon);
> +
> +  notification->BindToOwner(aWindow);
> +  return notification;

This will become notification.forget()

@@ +644,5 @@
> +                  const GetNotificationOptions& aFilter,
> +                  ErrorResult& aRv)
> +{
> +  nsRefPtr<Promise> promise;
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());

MOZ_ASSERT(window) here

@@ +645,5 @@
> +                  ErrorResult& aRv)
> +{
> +  nsRefPtr<Promise> promise;
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal.GetAsSupports());
> +  promise = new Promise(window);

Let's put the def with the assignment here, and actually you can move this way below to where you actually need it.

@@ +650,5 @@
> +
> +  nsIDocument* doc = window->GetExtantDoc();
> +  if (!doc) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return promise.forget();

nullptr, here and in your other returns below.

@@ +654,5 @@
> +    return promise.forget();
> +  }
> +
> +  nsString origin;
> +  aRv = nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), origin);

Let's call Notification::GetOrigin here to make sure we only have one place where we do that.

@@ +672,5 @@
> +    new NotificationStorageCallback(aGlobal, window, promise);
> +  nsString tag = aFilter.mTag.WasPassed() ?
> +                 aFilter.mTag.Value() :
> +                 EmptyString();
> +  notificationStorage->Get(origin, tag, callback);

This can fail, please check the result.

@@ +699,5 @@
>    NS_DispatchToMainThread(showNotificationTask);
>  }
>  
>  nsresult
>  Notification::CloseInternal()

Let's restructure this so that you at least try to call 'alertService->CloseAlert' if something in the storage stuff fails.

And CloseInternal can just return void.

@@ +727,4 @@
>  
> +nsresult
> +Notification::GetOrigin(nsPIDOMWindow* aWindow, nsString& aOrigin)
> +{

Nit: I'd MOZ_ASSERT(aWindow) here.

@@ +737,5 @@
> +  uint16_t appStatus = principal->GetAppStatus();
> +  uint32_t appId = principal->GetAppId();
> +
> +  // if we are in "app code", use manifest URL as unique origin since
> +  // multiple apps can share the same origin but not same notifications

Nit: Capital letters to start the sentence, and a period at the end, for all your comments.

@@ +741,5 @@
> +  // multiple apps can share the same origin but not same notifications
> +  if (appStatus == nsIPrincipal::APP_STATUS_NOT_INSTALLED ||
> +      appId == nsIScriptSecurityManager::NO_APP_ID ||
> +      appId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> +    rv = nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), aOrigin);

Let's file a followup to replace this with GetASCIIOrigin.

Nit: You've already gotten the node principal, just pass that through

@@ +748,5 @@
> +    // we are in "app code", use manifest url as origin
> +    nsCOMPtr<nsIAppsService> appsService =
> +      do_GetService("@mozilla.org/AppsService;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    appsService->GetManifestURLByLocalId(appId, aOrigin);

This could fail too.

::: dom/src/notification/Notification.h
@@ +31,5 @@
>    IMPL_EVENT_HANDLER(show)
>    IMPL_EVENT_HANDLER(error)
>    IMPL_EVENT_HANDLER(close)
>  
> +  Notification(const nsString& aID, const nsAString& aTitle, const nsAString& aBody,

Nit: Let's move this to the protected section.

@@ +39,5 @@
>    static already_AddRefed<Notification> Constructor(const GlobalObject& aGlobal,
>                                                      const nsAString& aTitle,
>                                                      const NotificationOptions& aOption,
>                                                      ErrorResult& aRv);
> +  void GetID(nsString& aRetval) {

Nit: These should be nsAString&

@@ +82,5 @@
>                                                ErrorResult& aRv);
>  
> +  static already_AddRefed<Promise> Get(const GlobalObject& aGlobal,
> +                     const GetNotificationOptions& aFilter,
> +                     ErrorResult& aRv);

Nit: align with the (

@@ +96,5 @@
>  
>    virtual JSObject* WrapObject(JSContext* aCx,
>                                 JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
>  protected:
> +  static nsRefPtr<Notification> CreateInternal(nsPIDOMWindow* aCx,

This should return already_AddRefed<Notification> to simplify ownership.

Nit: s/aCx/aWindow/

@@ +119,5 @@
>        return NS_LITERAL_STRING("auto");
>      }
>    }
>  
> +  static const NotificationDirection StringToDirection(nsString aDirection)

Nit: s/nsString/const nsAString&/

@@ +121,5 @@
>    }
>  
> +  static const NotificationDirection StringToDirection(nsString aDirection)
> +  {
> +    if (aDirection == NS_LITERAL_STRING("ltr")) {

use aDirection.EqualsLiteral("ltr"), here, and below.

@@ +123,5 @@
> +  static const NotificationDirection StringToDirection(nsString aDirection)
> +  {
> +    if (aDirection == NS_LITERAL_STRING("ltr")) {
> +      return NotificationDirection::Ltr;
> +    } else if (aDirection == NS_LITERAL_STRING("rtl")) {

Nit: No else after returns.

::: dom/src/notification/NotificationDB.jsm
@@ +29,5 @@
> +});
> +
> +
> +const NOTIFICATION_STORE_DIR =
> +        OS.Path.join(OS.Constants.Path.profileDir, "notifications");

Let's just use the profileDir directly.

@@ +46,5 @@
> +    ppmm.addMessageListener("Notification:Delete", this);
> +    ppmm.addMessageListener("Notification:GetAll", this);
> +  },
> +
> +  // attempt to read notification file, if its not there we will create it

Nit: s/its/it's/, plus usual stuff about making this comment into a real sentence.

@@ +50,5 @@
> +  // attempt to read notification file, if its not there we will create it
> +  load: function(callback) {
> +    var promise = OS.File.read(NOTIFICATION_STORE_PATH);
> +    promise.then(
> +      function onSuccess(data) {

We have support for => functions now that remove the need for bind... Maybe you'd prefer that? It would look something like this:

  promise.then(data => {
    // ...
    try { this.notifications = JSON.parse(gDecoder.decode(data)); }
    // ...
  }, reason => {
    // ...
    this.createStore(callback);
    // ...
  });

But your existing code is correct. No need to change.

@@ +57,5 @@
> +        } catch (e) {
> +          if (DEBUG) { debug("Unable to parse file data " + e); }
> +        }
> +        this.loaded = true;
> +        callback && callback();

Is this just shorthand(ish) for |if (callback) { callback(); }| ?

@@ +87,5 @@
> +  },
> +
> +  // creates the notification file, once the directory is created
> +  createFile: function(callback) {
> +    try {

Hm, I'd think this try/catch is unnecessary, any failures should trigger your onFailure function, right?

@@ +106,5 @@
> +  },
> +
> +  // save current notifications to the file
> +  save: function(callback) {
> +    var data = gEncoder.encode(JSON.stringify(this.notifications));

I think we should probably file a followup to move the string encoding/decoding to a worker since it could cause main thread jank with a big file.

@@ +112,5 @@
> +    promise.then(
> +      function onSuccess() {
> +        callback && callback();
> +      },
> +

Nit: unnecessary newline here.

@@ +160,5 @@
> +        break;
> +
> +      default:
> +        if (DEBUG) { debug("Invalid message name" + message.name); }
> +        break;

Nit: No need for 'break' here.

@@ +165,5 @@
> +    }
> +  },
> +
> +
> +  // we need to make sure any read/write operations are atomic,

Nit: extra newline above

@@ +167,5 @@
> +
> +
> +  // we need to make sure any read/write operations are atomic,
> +  // so use a queue to run each operation sequentially
> +  queueTask: function(operation, data, callback) {

I think there's a race here. If you receive two messages from a child in quick succession then I think you're going to get your state messed up:

  * receiveMessage()       // message 1 ("getall")
    - queueTask()          // this.runningTask == false
      - runNextTask()      // this.tasks.length == 1
        - ensureLoaded()   // this.loaded == false
          - load()         // async op, may take a while
  * receiveMessage()       // message 2 ("save")
    - queueTask()          // this.runningTask == false still!
      - runNextTask()      // this.tasks.length == 2
        - ensureLoaded()   // this.loaded == false still!
          - load()         // will reload file from disk

Then, imagine what happens when the first load finishes:

  * runNextTask() // from message 1 above, ensureLoaded()
                  // has returned, this.runningTask == true,
                  // task == "getall".
    - taskGetAll()
      - callback()
        - wrappedCallback()
          - runNextTask()    // this.tasks.length == 1
            - ensureLoaded() // this.loaded == true, calls back immediately,
                             // task == "save"
              - taskSave()
                - callback()
                  - wrappedCallback()
                    - runNextTask() // this.tasks.length == 0,
                                    // this.runningTask == false

Now the second load finishes:

  * runNextTask() // from message 2 above, ensureLoaded()
                  // has returned, this.runningTask == true,
                  // task == "getall".
    - this.tasks.shift() is going to return undefined.

This queue mechanism will need some more thought.

@@ +197,5 @@
> +      // wrap the task callback to make sure we immediately
> +      // run the next task after running the original callback
> +      var wrappedCallback = function() {
> +        if (DEBUG) { debug("Finishing task: " + task.operation); }
> +        task.callback.apply(this, Array.prototype.slice.call(arguments));

Hm, I don't understand why you need to copy the arguments array. Couldn't you just pass it directly?

::: dom/src/notification/NotificationStorage.js
@@ +15,5 @@
> +
> +const NOTIFICATIONSTORAGE_CID = "{37f819b0-0b5c-11e3-8ffd-0800200c9a66}";
> +const NOTIFICATIONSTORAGE_CONTRACTID = "@mozilla.org/notificationStorage;1";
> +
> +const STORAGE_PATH = "notifications.txt";

Nit: Remove

@@ +117,5 @@
> +
> +  _fetchFromCache: function(tag, callback) {
> +    var notifications = [];
> +    // if we are given a tag, only return that tag,
> +    // otherwise return all stored notifications

Nit: Maybe make this comment a little more clear

@@ +128,5 @@
> +    }
> +
> +    // pass each notification back separately
> +    notifications.forEach(function(notification) {
> +      callback.handle(notification.id,

This and callback.done() should be wrapped in individual try/catch blocks.

@@ +150,5 @@
> +  },
> +
> +  classID : Components.ID(NOTIFICATIONSTORAGE_CID),
> +  contractID : NOTIFICATIONSTORAGE_CONTRACTID,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINotificationStorage]),

You should add Ci.nsIMessageListener to this list of QI-able interfaces.

::: dom/src/notification/NotificationStorage.manifest
@@ +1,2 @@
> +# NotificationStorage.js
> +component {37f819b0-0b5c-11e3-8ffd-0800200c9a66} NotificationStorage.js

This needs to be different from your IID in the IDL file.

::: dom/tests/mochitest/notification/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +#needs IPC support, also tests do not run successfully in Firefox for now

This needs to be removed.

And talk to Doug Turner and/or William Chen about maybe removing the old tests?

Comment 36

[Michael Henretty [:mikehenrty][:mhenretty]]

Thank you for the review! This is all very useful information.

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #35)
> > +    rv = nsContentUtils::GetUTFOrigin(doc->NodePrincipal(), aOrigin);
> 
> Let's file a followup to replace this with GetASCIIOrigin.

Will do.

> @@ +57,5 @@
> > +        callback && callback();
> 
> Is this just shorthand(ish) for |if (callback) { callback(); }| ?

Yes.

> @@ +87,5 @@
> > +  // creates the notification file, once the directory is created
> > +  createFile: function(callback) {
> > +    try {
> 
> Hm, I'd think this try/catch is unnecessary, any failures should trigger
> your onFailure function, right?

It should, but the example was using a try/catch block, so I put this here just in case: https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.Error#Example.3A_Finding_out_whether_the_problem_is_due_to_a_file_that_does_not_exist
 
> @@ +106,5 @@
> > +  save: function(callback) {
> > +    var data = gEncoder.encode(JSON.stringify(this.notifications));
> 
> I think we should probably file a followup to move the string
> encoding/decoding to a worker since it could cause main thread jank with a
> big file.

Will do.

> @@ +167,5 @@
> > +  // we need to make sure any read/write operations are atomic,
> > +  // so use a queue to run each operation sequentially
> > +  queueTask: function(operation, data, callback) {
> 
> I think there's a race here. If you receive two messages from a child in
> quick succession then I think you're going to get your state messed up:
> 
> This queue mechanism will need some more thought.

Good catch! The problem is that ensureLoaded does an async task before we set the runningTask flag to true. I need to set this flag before calling ensureLoaded and we should be good. I'll add an appropriate test case as well.

> @@ +197,5 @@
> > +      var wrappedCallback = function() {
> > +        if (DEBUG) { debug("Finishing task: " + task.operation); }
> > +        task.callback.apply(this, Array.prototype.slice.call(arguments));
> 
> Hm, I don't understand why you need to copy the arguments array. Couldn't
> you just pass it directly?

Yes, I can just pass it directly. I am used to casting the arguments object to an array before using it, but here it is unnecessary. 
 
> And talk to Doug Turner and/or William Chen about maybe removing the old
> tests?

Will do.

Comment 37

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Updated Gecko Patch based on Bent's comments

Updated based on Bent's comments. Also fixed a minor bug in the storage logic (dealing with tag replacement) I found while testing.

Comment 38

[Peter Dolanjski [:pdol]]

We pulled this into the SystemsFE backlog, but decided that it won't block.

Comment 39

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Geck Patch for Notification.get()

Rebased with Makefile to Manifest changes from bug 920223, so that I can push to try.

Comment 40

[Michael Henretty [:mikehenrty][:mhenretty]]

Try Submission: https://tbpl.mozilla.org/?tree=Try&rev=838079b04e9b

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 812707 [details] [diff] [review]
Geck Patch for Notification.get()

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

This looks really close, I can't quite r+ yet due to the problem in NotificationStorage.js, see below. Should be easy to fix though!

::: dom/src/notification/Notification.cpp
@@ +77,5 @@
> +    NS_ENSURE_TRUE(element, NS_ERROR_FAILURE);
> +
> +    if (!JS_DefineElement(aCx, mNotifications, mCount++,
> +                          JS::ObjectValue(*element), nullptr, nullptr, 0))
> +    {

Nit: { on the previous line.

@@ +526,1 @@
>                                                     mIconUrl, doc, baseUri);

Nit: looks like your indentation is off here.

@@ +526,2 @@
>                                                     mIconUrl, doc, baseUri);
> +        if (NS_SUCCEEDED(rv) && srcUri) {

Nit: No need to check srcUri here, a successful return value should guarantee that it is non-null.

@@ +536,5 @@
>    nsCOMPtr<nsIObserver> observer = new NotificationObserver(this);
>  
>    nsString alertName;
>    rv = GetAlertName(alertName);
> +  if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS_VOID here.

@@ +762,5 @@
> +  uint16_t appStatus = principal->GetAppStatus();
> +  uint32_t appId = principal->GetAppId();
> +
> +  // If we are in "app code", use manifest URL as unique origin since
> +  // multiple apps can share the same origin but not same notifications.

This comment talks about the 'else' block only I think. Maybe move this below and talk instead about that the cases are where we're not in "app code"?

::: dom/src/notification/Notification.h
@@ +9,5 @@
>  
>  #include "nsDOMEventTargetHelper.h"
>  #include "nsIObserver.h"
>  
> +#include "mozilla/dom/Promise.h"

Can you try just forward-declaring this (in the mozilla::dom namespace, just like you do for NotificationObserver).

@@ +96,5 @@
> +  Notification(const nsAString& aID, const nsAString& aTitle, const nsAString& aBody,
> +               NotificationDirection aDir, const nsAString& aLang,
> +               const nsAString& aTag, const nsAString& aIconUrl);
> +
> +  static already_AddRefed<Notification> CreateInternal(nsPIDOMWindow* aCx,

Nit: This should be 'aWindow', not 'aCx'

::: dom/src/notification/NotificationDB.jsm
@@ +87,5 @@
> +  },
> +
> +  // Creates the notification file once the directory is created.
> +  createFile: function(callback) {
> +    try {

Yeah, let's nix this. It shouldn't be needed.

@@ +196,5 @@
> +      // run the next task after running the original callback.
> +      var wrappedCallback = function() {
> +        if (DEBUG) { debug("Finishing task: " + task.operation); }
> +        task.callback.apply(this, arguments);
> +        this.runNextTask();

Hm. This has the unfortunate side effect of making things slower... Theoretically there's no reason why you should have to wait for pending writes to complete in order to service a GetAll request, for example. I don't think it's worth worrying about bit we should have a followup to speed this up (I bet we can remove the queue entirely!).

::: dom/src/notification/NotificationStorage.js
@@ +24,5 @@
> +function NotificationStorage() {
> +  // cache objects
> +  this._notifications = {};
> +  this._byTag = {};
> +  this._cached = false;

I don't see where this is ever set to true or otherwise modified. This means that you're going to ask the database every time you do a get().

Comment 42

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Gecko Patch

Updated patch based on Bent's comments, and added a few more tests.

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #41)
> @@ +196,5 @@
> > +      // run the next task after running the original callback.
> > +      var wrappedCallback = function() {
> > +        if (DEBUG) { debug("Finishing task: " + task.operation); }
> > +        task.callback.apply(this, arguments);
> > +        this.runNextTask();
> 
> Hm. This has the unfortunate side effect of making things slower...
> Theoretically there's no reason why you should have to wait for pending
> writes to complete in order to service a GetAll request, for example. I
> don't think it's worth worrying about bit we should have a followup to speed
> this up (I bet we can remove the queue entirely!).

I agree with you that upon receiving a save/delete request we should be able to update our in-memory object, kick off an async write, and continue handling get/save/delete requests without waiting for the callback. However, for our first operation(s) we will need to load notifications from the file system, and since this can require several async ops we will at least need the queue here to record incoming get/save/delete requests. Perhaps you are suggesting we kill the queue once we are assured the notifications have been loaded, which is totally workable. I chose to do it this way since I thought of this as a service which assured atomicity in read/writes, so that any time you call Get you are assured of getting whats actually stored on the FS. But it's true this approach will negatively affect performance. I'm open to filing a follow-up, should be a pretty simple change.

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 813410 [details] [diff] [review]
Gecko Patch

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

I think this looks great! r=me.

Comment 44

[Gregor Wagner [:gwagner]]

Great Job!

https://hg.mozilla.org/integration/b2g-inbound/rev/0c08e9e0f505
https://hg.mozilla.org/integration/b2g-inbound/rev/a81c996e06c1

Comment 45

[Carsten Book [:Tomcat]]

Hey Guys sorry, had to back out these changes because of build bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=28740640&tree=B2g-Inbound 



(In reply to Gregor Wagner [:gwagner] from comment #44)
> Great Job!
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/0c08e9e0f505
> https://hg.mozilla.org/integration/b2g-inbound/rev/a81c996e06c1

Comment 46

[Michael Henretty [:mikehenrty][:mhenretty]]

Gregor, it looks like the changeset that was pushed to b2g-inbound was missing some of the files from the patch.

It should look like this: https://hg.mozilla.org/try/rev/deb4a9bd6996
But instead looks like this: https://hg.mozilla.org/integration/b2g-inbound/rev/a81c996e06c1

Was this just a copy and paste error?

Comment 47

[Gregor Wagner [:gwagner]]

It somehow doesn't import the whole patch for me :(

Comment 48

[Carsten Book [:Tomcat]]

(In reply to Gregor Wagner [:gwagner] from comment #47)
> It somehow doesn't import the whole patch for me :(

adding the other sheriffs, maybe we can help here

Comment 49

[Ryan VanderMeulen [:RyanVM]]

The patch appears to be malformed somehow. Mercurial errors out for me when I try to apply it. Try rebasing (it doesn't apply cleanly anyway) on top of m-c and attaching a fresh hg export here.

Comment 50

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Gecko Patch

Rebased and tested local import.

Comment 51

[Gregor Wagner [:gwagner]]

Lets try again
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/b1cff47ba03a
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/08cf11b864f8

Comment 52

[Phil Ringnalda (:philor)]

And https://hg.mozilla.org/integration/b2g-inbound/rev/9e9f6d05f1e5 since at least on Windows it needed a clobber.

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b1cff47ba03a
https://hg.mozilla.org/mozilla-central/rev/08cf11b864f8
https://hg.mozilla.org/mozilla-central/rev/9e9f6d05f1e5

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Part 2 appears to be breaking the gaia integration tests as described in bug 924130.  Looking for someone to back this out.

Comment 55

[Reuben Morais [:reuben]]

Backed out for breaking Gaia tests.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e5fedcaa0f40

Comment 56

[Andreas Gal :gal]

Can we do a little post mortem here to find out why this landed and then set the tree on fire? We need a process fix here to avoid this.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Because they're hidden by default on tbpl die to not meeting the visibility requirements and have been some form of busted for awhile now

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

And I believe in order to get the gaia tests running reliably on tbpl we need "gaia try" support in order to be able to diagnose and fix tbpl-only failures.  This was discussed in Oslo and I believe its being worked on.

Comment 59

[Gregor Wagner [:gwagner]]

(In reply to Ben Kelly [:bkelly] from comment #54)
> Part 2 appears to be breaking the gaia integration tests as described in bug
> 924130.  Looking for someone to back this out.

Is this travis?

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Noticed in Travis, but reproduced locally with "make test-integration".

Comment 61

[Gregor Wagner [:gwagner]]

(In reply to Andreas Gal :gal from comment #56)
> Can we do a little post mortem here to find out why this landed and then set
> the tree on fire? We need a process fix here to avoid this.

As Ryan said, it doesn't show up on tbpl and we have no way to test it.
We had endless travis vs. tbpl discussions and I don't see any solution that will fix this problem in the short and long term.

Comment 62

[Andreas Gal :gal]

Thats unacceptable. We need a solution for this. Why is there no way to test this?

Comment 63

[Gregor Wagner [:gwagner]]

(In reply to Andreas Gal :gal from comment #62)
> Thats unacceptable. We need a solution for this. Why is there no way to test
> this?

So just to make sure we are talking about the same problem here. We are not talking about bug 919676 where a gecko patch that landed caused a bricked/non-bootable phone even though the tree was completely green.

This gecko-patch caused a travis failure. Travis only runs from within GH for gaia PRs.
We don't have a gaia try-sever yet and after the last b2g-meeting I heard that we will never have one because there are legal issues. We might have one for moz employees in the future but this doesn't really solve the problem. Potentially every gecko push can cause travis failures.

The testing situation is annoying but I am more concerned about gecko pushes that don't trigger travis runs.
I am not sure how often we update the build that travis uses but with all the different trees (m-inbound, b2g-inbound, mc) we might end up in a situation where we have a huge merge set that causes a travis failure and it takes hours/maybe days to find the right change-set.

I am not 100% up-to-date with all our testing plans and maybe we have solution for this but the current situation is indeed concerning.

Comment 64

[[:fabrice] Fabrice Desré]

(In reply to Gregor Wagner [:gwagner] from comment #63)

> We don't have a gaia try-sever yet and after the last b2g-meeting I heard
> that we will never have one because there are legal issues. We might have
> one for moz employees in the future but this doesn't really solve the
> problem. Potentially every gecko push can cause travis failures.

Which legal issues?

Comment 65

[Andreas Gal :gal]

(In reply to Gregor Wagner [:gwagner] from comment #63)
> (In reply to Andreas Gal :gal from comment #62)
> > Thats unacceptable. We need a solution for this. Why is there no way to test
> > this?
> 
> So just to make sure we are talking about the same problem here. We are not
> talking about bug 919676 where a gecko patch that landed caused a
> bricked/non-bootable phone even though the tree was completely green.
> 
> This gecko-patch caused a travis failure. Travis only runs from within GH
> for gaia PRs.
> We don't have a gaia try-sever yet and after the last b2g-meeting I heard
> that we will never have one because there are legal issues. We might have
> one for moz employees in the future but this doesn't really solve the
> problem. Potentially every gecko push can cause travis failures.

I am not aware of this and it doesn't make sense to me. We can trivially trigger the same testing travis does on our infrastructure for try. We might even be able to trigger travis itself without having to run that on our infra. In addition, a FFOS engineer broke gaia here. Could we at least run the test locally? Was that done?

> 
> The testing situation is annoying but I am more concerned about gecko pushes
> that don't trigger travis runs.
> I am not sure how often we update the build that travis uses but with all
> the different trees (m-inbound, b2g-inbound, mc) we might end up in a
> situation where we have a huge merge set that causes a travis failure and it
> takes hours/maybe days to find the right change-set.

Yep, we need to run travis with recent builds.

> 
> I am not 100% up-to-date with all our testing plans and maybe we have
> solution for this but the current situation is indeed concerning.

Can you grab a few people and figure out something better? Thanks.

Comment 66

[Julien Wajsberg [:julienw]]

I really think there is an existing plan to run the Gaia UI Tests as part of Try. I don't understand the controversy here.

This bug is not the place to discuss it, we all agree this needs to be done, and talented people are already working on this.

The only way forward is to meet the visibility requirements in tbpl as Ryan outlined it, and people like James Lal and Jonathan Griffin are working on this.

Comment 67

[James Lal [:lightsofapollo]]

There are various issues here:

Legal?

There are no legal issues with b2g-desktop. We need to start with this since that is the only good development environment right now.

WTF Travis?

Travis is nothing more then a gate keeper for gaia pushes into the mirroring for TBPL. We will have all suites running on TBPL. This is not a fast process and not having testing infrastructure in unacceptable so we need travis to keep running our gaia tests... All suites running on travis will report to TBPL.

Emulators (off topic):

We need good emulator support there are legal issues here we need to solve these so we can stop breaking shit only seen on phones.

Comment 68

[Andreas Gal :gal]

We are in an unacceptable state right now. We can't keep closing the tree and keep it closed for days because we are hunting regressions. If there is no short-term plan in place to avoid tree closures, we will have to stop tree closures and live with regressions instead. Thats also a terrible place to be in, but still better than ongoing tree closures. I would like to see a comprehensive timeline and plan for this. Faramarz, please come up with something. This is mostly a trunk issue I guess.

Comment 69

[Gregor Wagner [:gwagner]]

(In reply to Fabrice Desré [:fabrice] from comment #64)
> (In reply to Gregor Wagner [:gwagner] from comment #63)
> 
> > We don't have a gaia try-sever yet and after the last b2g-meeting I heard
> > that we will never have one because there are legal issues. We might have
> > one for moz employees in the future but this doesn't really solve the
> > problem. Potentially every gecko push can cause travis failures.
> 
> Which legal issues?

I remember joduinn was saying this about gaia-try during the last b2g-meeting but James just
confirmed that we don't need any private builds for gaia-try. So we should be good here. I will talk to joduinn about it.

Yep this is not the place but Andreas asked for a post-mortem.
After talking to James I am less concerned and seems like we can avoid this scenario in the future.

(In reply to Ben Kelly [:bkelly] from comment #60)
> Noticed in Travis, but reproduced locally with "make test-integration".

This is the first time I heard about 'make test-integration' and mdn also doesn't know it:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Automated_testing

Comment 70

[James Lal [:lightsofapollo]]

The wiki is crap the README is the closest thing to the code and where updates should live https://github.com/mozilla-b2g/gaia#integration-tests.

As I have been repeating for the last year we should not maintain state about how to hack on gaia here it always is out of date and is subject to zero review.

Comment 71

[Andreas Gal :gal]

(In reply to Gregor Wagner [:gwagner] from comment #69)
> (In reply to Fabrice Desré [:fabrice] from comment #64)
> > (In reply to Gregor Wagner [:gwagner] from comment #63)
> > 
> > > We don't have a gaia try-sever yet and after the last b2g-meeting I heard
> > > that we will never have one because there are legal issues. We might have
> > > one for moz employees in the future but this doesn't really solve the
> > > problem. Potentially every gecko push can cause travis failures.
> > 
> > Which legal issues?
> 
> I remember joduinn was saying this about gaia-try during the last
> b2g-meeting but James just
> confirmed that we don't need any private builds for gaia-try. So we should
> be good here. I will talk to joduinn about it.
> 
> Yep this is not the place but Andreas asked for a post-mortem.
> After talking to James I am less concerned and seems like we can avoid this
> scenario in the future.
> 
> (In reply to Ben Kelly [:bkelly] from comment #60)
> > Noticed in Travis, but reproduced locally with "make test-integration".
> 
> This is the first time I heard about 'make test-integration' and mdn also
> doesn't know it:
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/
> Automated_testing

Please send an email to dev-b2g about this. Thanks.

And no need to talk to joduinn. I will do that.

Comment 72

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 813872 [details] [diff] [review]
Gecko Patch

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

::: dom/bindings/Bindings.conf
@@ +118,5 @@
>      'resultNotAddRefed': [ 'playbackRate' ],
>  },
>  
> +'Notification' : {
> +    'nativeType': 'mozilla::dom::Notification'

This is in the wrong place, and isn't necessary anyway. Please fix that.

Comment 73

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Andreas Gal :gal from comment #65)
> In addition, a FFOS engineer broke gaia here. Could we at least run
> the test locally? Was that done?

This was not done, and it should have been. I incorrectly assumed that if we had a green tree in try (and also I could test my feature manually on the device) then we should be good. This was my fault because I did indeed know about the integration tests, but did not verify that these tests were running in Try. I am deeply sorry this held up gaia development.


(In reply to :Ms2ger from comment #72)
> Comment on attachment 813872 [details] [diff] [review]
> Gecko Patch
> 
> Review of attachment 813872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +118,5 @@
> >      'resultNotAddRefed': [ 'playbackRate' ],
> >  },
> >  
> > +'Notification' : {
> > +    'nativeType': 'mozilla::dom::Notification'
> 
> This is in the wrong place, and isn't necessary anyway. Please fix that.

Will do.


In any case, the integration tests will need to be updated to work with this patch. I will fix that before re-landing this.

Comment 74

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Gaia Integration Test Patch (PR)

Comment 75

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.patch

Updated based on Ms2ger's comments, and added more descriptive alert names.

Waiting on Travis and Try: https://tbpl.mozilla.org/?tree=Try&rev=8dba1a1cd31a

Note: we should land the Gaia Integration Test Patch first, since it should work both with or without this Gecko patch.

Comment 76

[Michael Henretty [:mikehenrty][:mhenretty]]

Integration tests landed: https://github.com/mozilla-b2g/gaia/commit/e68a11adb9e29f60043737f9d8bf81f6c5fd34e5

Comment 77

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: notifications.patch

Fixing js compartment crash found by qDot.

Comment 78

[Gregor Wagner [:gwagner]]

Lets try again
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/ae1c322dab45
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/119d46e5cec3

Comment 79

[Phil Ringnalda (:philor)]

(In reply to Phil Ringnalda (:philor) from comment #52)
> And https://hg.mozilla.org/integration/b2g-inbound/rev/9e9f6d05f1e5 since at
> least on Windows it needed a clobber.

Still does.

Comment 80

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Phil Ringnalda (:philor) from comment #79)
> (In reply to Phil Ringnalda (:philor) from comment #52)
> > And https://hg.mozilla.org/integration/b2g-inbound/rev/9e9f6d05f1e5 since at
> > least on Windows it needed a clobber.
> 
> Still does.

https://hg.mozilla.org/integration/b2g-inbound/rev/1b1567b456a5

Comment 81

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae1c322dab45
https://hg.mozilla.org/mozilla-central/rev/119d46e5cec3
https://hg.mozilla.org/mozilla-central/rev/1b1567b456a5

Comment 82

[Candice Serran (:cserran)]

Candice Serran changed story state to finished in Pivotal Tracker

Comment 83

[Candice Serran (:cserran)]

Candice Serran changed story state to accepted in Pivotal Tracker

Comment 84

[Candice Serran (:cserran)]

Candice Serran changed story state to accepted in Pivotal Tracker

Comment 85

[Jason Smith [:jsmith]]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2515

Comment 86

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Problems with current implementation from IRC:

<annevk> nsm: yo
<nsm> annevk: Notification.get() needs to be better defined if Notifications are going to be in workers
<annevk> nsm: hmm okay
<annevk> nsm: file a bug?
<nsm> ok
<nsm> on w3c?
<annevk> https://github.com/whatwg/notifications/issues/new
<annevk> nsm: replied
<nsm> annevk: to clarify, the spec assumes no persistence, is that correct? That's why there is no mention of storage mutex or similar
<annevk> nsm: persistence?
<annevk> nsm: I would assume if we add some kind of notification.data thing we'd storage clone it for each global
<nsm> annevk: for example in b2g, the notifications stay around in the tray even after the app quits, so they are 'stored' somewhere
<annevk> nsm: ah, notifications can stay around for a while per spec
<annevk> nsm: Notification objects can go away
<annevk> per normal GC
<nsm> annevk: on a related note https://bugzil.la/932929
<nsm> annevk: if main thread creates a Notification, now owrker calls Notification.get() and gets the notification, obviously the two JS objects are different, but if one of them calls close, its the same underlying notification for the user, so does the main thread also receive onclose ?
<annevk> yes
<annevk> you close the notification
<annevk> so all its Notification objects will then get a queued close event
<annevk> that's what the spec says anyway
<nsm> annevk: and the same will happen for two pages right. in that case our get() is very broken
<nsm> is anyone else planning to implement get()?
<annevk> nsm: yes
<annevk> nsm: dunno
<annevk> I think people will want it eventually
<nsm> reuben: ping?
<nsm> mhenretty: ping?
<-- jhford ([e-mail]) has quit (Ping timeout)
<reuben> nsm: hi
<nsm> Notification.get() we have is completely broken based on me and annevk's discussion above
<mhenretty> nsm: pong
<nsm> it is based too much on a app model, where there is only one page running and so on
<nsm> it breaks down with multiple same origin tabs or workers

Comment 87

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #86)
> Problems with current implementation from IRC:

Based on Nikil's comment, file bug 965632.