1003050 - [Notification] Use promises to make the code more solid

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

This comes from the discussion in Bug 997949.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Attachment: notification-promises-poc.patch

Comment 2

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

I'm working on implementing this setup in settings for bug 900551. I think I've found why it looks like the promise that comes out the queueTask is returned into the ether (mhenretty mentioned this in his fb for bug 997949). The queueTask calls in receiveMessage never got updated, so they're still passing callbacks, which won't even work because queueTask only takes two arguments now (I realize julian never tested this so I'm not really blaming him except that this did cost me some time. Time that I will NEVER GET BACK. :) ). We need to only pass the first two arguments to queueTask then chain the callbacks as part of a .then(), which means we /do/ need that promise returned.

At least, that's my interpretation?

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #2)
> I'm working on implementing this setup in settings for bug 900551. I think
> I've found why it looks like the promise that comes out the queueTask is
> returned into the ether (mhenretty mentioned this in his fb for bug 997949).
> The queueTask calls in receiveMessage never got updated, so they're still
> passing callbacks, which won't even work because queueTask only takes two
> arguments now.
>  We need to only pass the first two arguments to queueTask then chain
> the callbacks as part of a .then(), which means we /do/ need that promise
> returned.
> 
> At least, that's my interpretation?

Oops looks like I missed this, you're perfectly right !

> (I realize julian never tested this so I'm not really blaming
> him except that this did cost me some time. Time that I will NEVER GET BACK.
> :) ).

Heh I even said on IRC that I knew it didn't work ;)

Comment 4

[:gerard-majax]

Okay, now that all the Notification stuff landed and we do not risks any conflict, let's work on this: we already have two intermittents that may be directly related to this.

Comment 5

[:gerard-majax]

Okay, at least mochitests for Notification are passing :))

Comment 6

[:gerard-majax]

Attachment: 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

Comment 7

[:gerard-majax]

Gaia Integration tests ran against Gecko with attachment 8432518 [details] [diff] [review]:


  Desktop Notifications
    ✓ should be cleared after opening app (6397ms)

  Notification.get():
    ✓ promise is fulfilled 
    ✓ promise returns a new notification (78ms)
    ✓ get works with tag option (80ms)
    ✓ should work across domains (4336ms)
    ✓ notifications should persist even after closing app (5614ms)
    ✓ bug 931307, empty title should not cause crash 

  launch an app via notification click
    - clicking notification launches app

  mozChromeNotifications:
    ✓ Checking mozResendAllNotifications API (109ms)
    ✓ Sending no notification, resends none (2919ms)
    ✓ Sending one notification, resends one (3066ms)
    ✓ Sending two notifications, resends two (2722ms)
    ✓ Sending two notifications, close one, resends one (3034ms)
    ✓ Sending one notification, remove from tray, resend (4439ms)

  notification tests
    ✓ fire notification (134ms)
    ✓ system replace notification (157ms)
    ✓ close notification (118ms)
    - email notification should not wake screen

  Notification events
    - click event starts application
    ✓ close event removes notification (4204ms)
    ✓ click event on resent notification starts application (10392ms)
    ✓ close event removes resent notification (6186ms)


  19 passing (2m)
  3 pending

Comment 8

[:gerard-majax]

And xpcshell NotificationDB tests are also working.

Comment 9

[:gerard-majax]

Try pending at https://tbpl.mozilla.org/?tree=Try&rev=72e693356b0d

Comment 10

[:gerard-majax]

Try mostly green, only one failing test which seems to be intermittent and unrelated. Let's retrigger :)

Comment 11

[:gerard-majax]

All try is green.

Comment 12

[:gerard-majax]

Comment on attachment 8432518 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

Michael, it's your turn to give your blessing. :)

Comment 13

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

Comment on attachment 8432518 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

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

Looking good Alexandre! Just a couple of questions:

::: dom/src/notification/NotificationDB.jsm
@@ +221,5 @@
>  
> +    var promise = new Promise(function(resolve, reject) {
> +      defer.resolve = resolve;
> +      defer.reject = reject;
> +     });

It's unfortunate we don't have a Promise.defer helper.

@@ +271,5 @@
> +    }.bind(this))
> +    .catch(function(err) {
> +      if (DEBUG) {
> +        debug('Error while running ' + this.runningTask.operation + ': ' + err);
> +      }

I'm not the most familiar with promises, but how does this work? I though .catch was supposed to return a value that would resolve the chained promise? Seems like here we should either reject the runningTask defer and handle the rejection it in our message handler above, or we just resolve the task here and move on.

@@ +273,5 @@
> +      if (DEBUG) {
> +        debug('Error while running ' + this.runningTask.operation + ': ' + err);
> +      }
> +    }.bind(this))
> +    .then(function(payload) {

nit: payload unused.

@@ +277,5 @@
> +    .then(function(payload) {
> +      if (DEBUG) {
> +        debug("Resolving deferred for task: " + this.runningTask.operation);
> +      }
> +      this.runningTask.defer.resolve();

Are we double resolving?

Comment 14

[:gerard-majax]

Attachment: 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

Addressing most of the remarks. I need to re-check the way we handle resolving/rejecting, since I'm not yet very familiar with this too.

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

(In reply to Michael Henretty [:mhenretty] from comment #13)
> Comment on attachment 8432518 [details] [diff] [review]
> 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch
> 
> Review of attachment 8432518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good Alexandre! Just a couple of questions:
> 
> ::: dom/src/notification/NotificationDB.jsm
> @@ +221,5 @@
> >  
> > +    var promise = new Promise(function(resolve, reject) {
> > +      defer.resolve = resolve;
> > +      defer.reject = reject;
> > +     });
> 
> It's unfortunate we don't have a Promise.defer helper.

Yeah, I so love much more the "defer" way of working...

> @@ +271,5 @@
> > +    }.bind(this))
> > +    .catch(function(err) {
> > +      if (DEBUG) {
> > +        debug('Error while running ' + this.runningTask.operation + ': ' + err);
> > +      }
> 
> I'm not the most familiar with promises, but how does this work? I though
> .catch was supposed to return a value that would resolve the chained
> promise? Seems like here we should either reject the runningTask defer and
> handle the rejection it in our message handler above, or we just resolve the
> task here and move on.

Just "catching" and not rethrowing or return a rejected promise makes it return a resolved promise.

Comment 16

[:gerard-majax]

Comment on attachment 8433904 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

This should be cleaner now: properly resolving/rejecting once, removed the unused variable.

Try is okay: https://tbpl.mozilla.org/?tree=Try&rev=32b5e4d11c3a

Comment 17

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

Comment on attachment 8433904 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

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

Thanks for the continued work here Alex. Sorry for the slow reviews, my mind is preoccupied with 2.0 stuff. One last issue here I think, mentioned below. Also note, I won't have time to test this code out this week, so I may have to leave that to you :)

::: dom/src/notification/NotificationDB.jsm
@@ +167,4 @@
>  
>      switch (message.name) {
>        case "Notification:GetAll":
> +        this.queueTask("getall", message.data).then(function(notifications) {

Since we are now rejecting promises, we probably want to attach a rejection handler. IIRC, the old code would still call the queueTast callback even if we got a onFailure case. We should preserve that behavior.

Comment 18

[:gerard-majax]

Attachment: 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

We should now have a better error reporting from NotificationDB. I'm not sure however that should would propagate to content: do we want this? If yes, is it enough with this code?

Comment 19

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

Comment on attachment 8434972 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

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

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> Created attachment 8434972 [details] [diff] [review]
> 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch
> 
> We should now have a better error reporting from NotificationDB. I'm not
> sure however that should would propagate to content: do we want this? If
> yes, is it enough with this code?

I spoke with you about this over IRC, but I'll put it here too. I think we eventually want to propogate an error message to content, but we should do it by rejecting the promise returned bet Notification.get (ie. bug 951629). Also, would be great to report all error message (ie anything after `if (DEBUG)`) whenever we are in an engineering build. But I don't think this patch should block on that either. Good work!

Comment 20

[:gerard-majax]

A new pending try is runing at https://tbpl.mozilla.org/?tree=Try&rev=05ffcac5f3a1
And I have sent a 50 jobs retrigger in https://tbpl.mozilla.org/?tree=Try&rev=fbfda42afe32 to check for intermittents.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8434972 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

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

::: dom/src/notification/NotificationDB.jsm
@@ +93,4 @@
>        function onSuccess(data) {
> +        // JSON parsing failure will get handled by a later catch in the promise
> +        // chain
> +        this.notifications = JSON.parse(gDecoder.decode(data));

You should rather do
`var promise = OS.File.read(NOTIFICATION_STORE_PATH, { encoding: "utf-8"})` and get rid of `gDecoder`. This will make sure that any decoding takes place off the main thread.

@@ +132,1 @@
>      var promise = OS.File.open(NOTIFICATION_STORE_PATH, {create: true});

Calling `OS.File.writeAtomic(NOTIFICATION_STORE_PATH, "")` would probably be more efficient.

@@ +142,2 @@
>      var data = gEncoder.encode(JSON.stringify(this.notifications));
> +    return OS.File.writeAtomic(NOTIFICATION_STORE_PATH, data);

OS.File.writeAtomic(NOTIFICATION_STORE_PATH, JSON.stringify(this.notifications), { encoding: "utf-8"})

Comment 22

[:gerard-majax]

Attachment: 0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

Updated with all suggestions from Yoric.

Comment 23

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

Comment on attachment 8436813 [details] [diff] [review]
0001-Bug-1003050-Harden-NotificationDB-code-with-Promises.patch

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

This code just gets better and better.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7b9657044f45

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b9657044f45