Closed Bug 1003050 Opened 8 years ago Closed 8 years ago

[Notification] Use promises to make the code more solid

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
2.0 S4 (20june)

People

(Reporter: julienw, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe] [p=])

Attachments

(1 file, 4 obsolete files)

This comes from the discussion in Bug 997949.
Whiteboard: [systemsfe] [p=1 → [systemsfe] [p=]
Attached patch notification-promises-poc.patch (obsolete) — Splinter Review
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?
(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 ;)
Blocks: 1011158
Blocks: 1018879
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.
Assignee: nobody → lissyx+mozillians
Target Milestone: --- → 2.0 S3 (6june)
Okay, at least mochitests for Notification are passing :))
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
And xpcshell NotificationDB tests are also working.
Try mostly green, only one failing test which seems to be intermittent and unrelated. Let's retrigger :)
All try is green.
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. :)
Attachment #8432518 - Flags: review?(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?
Attachment #8432518 - Flags: review?(mhenretty)
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.
Attachment #8432518 - Attachment is obsolete: true
(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 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
Attachment #8433904 - Flags: review?(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.
Attachment #8433904 - Flags: review?(mhenretty)
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?
Attachment #8433904 - Attachment is obsolete: true
Attachment #8434972 - Flags: review?(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!
Attachment #8434972 - Flags: review?(mhenretty) → review+
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 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"})
Updated with all suggestions from Yoric.
Attachment #8434972 - Attachment is obsolete: true
Attachment #8436813 - Flags: review?(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.
Attachment #8436813 - Flags: review?(mhenretty) → review+
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Keywords: checkin-needed
Blocks: 1024090
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.