963130 - NotificationDB throws with missing this.byTag[origin]

Status: RESOLVED FIXED

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

Comment 1

[:gerard-majax]

From bug 962977, we learned that:
 - start emulator
 - send a wappush message:
   - telnet localhost 5554
   - sms pdu 07911326040000F0400B911346610089F6000420806291731408350605040B8423F0010603AEAF8203056A0045C60D036F7265696C6C7900850103436865636B20746869732077656273697465000101

Will in some case trigger an exception in NotificationDB.jsm:
I/Gecko   (  308): *************************
I/Gecko   (  308): A coding exception was thrown in a Promise resolution callback.
I/Gecko   (  308): 
I/Gecko   (  308): Full message: TypeError: this.byTag[origin] is undefined
I/Gecko   (  308): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
I/Gecko   (  308): Full stack: NotificationDB.taskSave@resource://gre/modules/NotificationDB.jsm:255
I/Gecko   (  308): NotificationDB.runNextTask/<@resource://gre/modules/NotificationDB.jsm:220
I/Gecko   (  308): onSuccess@resource://gre/modules/NotificationDB.jsm:61
I/Gecko   (  308): Handler.prototype.process@resource://gre/modules/Promise.jsm:767
I/Gecko   (  308): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
I/Gecko   (  308): 
I/Gecko   (  308): *************************

To my understanding, this happens when:
 - you have sent a previous notification to the same app, that has not been closed
 - restart b2g
 - send a new notification to this application

As far as I understand, the this.byTag map is not populated when there is already some notificationstore.json

Comment 2

[:gerard-majax]

First run, fresh profile:
I/Gecko   (   45): -*- NotificationDB component: Task, saving
I/Gecko   (   45): -*- NotificationDB component: Task: notifications={}
I/Gecko   (   45): -*- NotificationDB component: Task: origin="app://wappush.gaiamobile.org/manifest.webapp"
I/Gecko   (   45): -*- NotificationDB component: Task: notifications[origin]=undefined
I/Gecko   (   45): -*- NotificationDB component: Task: this.byTag[origin]={}

Then, stop and start b2g (keeping the profile, not closing the notification):
I/Gecko   (  285): -*- NotificationDB component: Task, saving
I/Gecko   (  285): -*- NotificationDB component: Task: notifications={"app://wappush.gaiamobile.org/manifest.webapp":{"{84e795b0-7134-4bcb-81cc-9f90fc6105b5}":{"id":"{84e795b0-7134-4bcb-81cc-9f90fc6105b5}","title":"[phone]","dir":"auto","lang":"","body":"Check this website http://www.oreilly.com/","tag":"1390493982425","icon":"app://wappush.gaiamobile.org/style/icons/wappush.png"}}}
I/Gecko   (  285): -*- NotificationDB component: Task: origin="app://wappush.gaiamobile.org/manifest.webapp"
I/Gecko   (  285): -*- NotificationDB component: Task: notifications[origin]={"{84e795b0-7134-4bcb-81cc-9f90fc6105b5}":{"id":"{84e795b0-7134-4bcb-81cc-9f90fc6105b5}","title":"[phone]","dir":"auto","lang":"","body":"Check this website http://www.oreilly.com/","tag":"1390493982425","icon":"app://wappush.gaiamobile.org/style/icons/wappush.png"}}
I/Gecko   (  285): -*- NotificationDB component: Task: this.byTag[origin]=undefined
I/Gecko   (  285): *************************
I/Gecko   (  285): A coding exception was thrown in a Promise resolution callback.
I/Gecko   (  285): 
I/Gecko   (  285): Full message: TypeError: this.byTag[origin] is undefined
I/Gecko   (  285): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
I/Gecko   (  285): Full stack: NotificationDB.taskSave@resource://gre/modules/NotificationDB.jsm:261
I/Gecko   (  285): NotificationDB.runNextTask/<@resource://gre/modules/NotificationDB.jsm:225
I/Gecko   (  285): onSuccess@resource://gre/modules/NotificationDB.jsm:61
I/Gecko   (  285): Handler.prototype.process@resource://gre/modules/Promise.jsm:767
I/Gecko   (  285): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
I/Gecko   (  285): 
I/Gecko   (  285): *************************

Comment 4

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

Attachment: Populate byTag on DB reload

This seems to do the trick. I pushed to try, and am going to bang on it for a while using the QA page:

http://mozilla.github.io/qa-testcase-data/webapi/notifications/

https://tbpl.mozilla.org/?tree=Try&rev=4a4db178bb4f

Comment 5

[:gerard-majax]

I do confirm that this patch fixes the exposed issue when testing in emulator :)

Comment 6

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

Comment on attachment 8364630 [details] [diff] [review]
Populate byTag on DB reload

Try looks good. Alex, care to review?

Comment 7

[Gregor Wagner [:gwagner]]

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

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b8e730733ce8

Comment 9

[:gerard-majax]

Now that this has landed, it also fixes SMS notification issues :)

Comment 10

[:gerard-majax]

This is missing from the Gecko 28 of 1.3

Comment 11

[:gerard-majax]

Comment on attachment 8364630 [details] [diff] [review]
Populate byTag on DB reload

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: huge breakage of the new notification API
Testing completed: this patch has been on master since early february and helped fixing a lot of notifications issues related to the new API
Risk to taking this patch (and alternatives if risky): low, the fix is quite short and self contained
String or UUID changes made by this patch: none

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 8364630 [details] [diff] [review]
Populate byTag on DB reload

a- - see blocking bug for more details

Comment 13

[:gerard-majax]

Comment on attachment 8364630 [details] [diff] [review]
Populate byTag on DB reload

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: New Notification API broken, NotificationDB will throw and break at use.
Testing completed: in master since end of january, fixes a lot of bugs
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

I'm renominating since without this patch, the notification API is not usable on gecko 28 for b2g

Comment 14

[Jason Smith [:jsmith]]

If you are asking uplift here, then you need a 1.3+. We don't grant blanket approval anymore without 1.3+.

Comment 15

[:gerard-majax]

requesting 1.3? as it breaks Notification API.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8364630 [details] [diff] [review]
Populate byTag on DB reload

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

::: dom/src/notification/NotificationDB.jsm
@@ +58,5 @@
>            if (DEBUG) { debug("Unable to parse file data " + e); }
>          }
> +        // populate the list of notifications by tag
> +        if (this.notifications) {
> +          for (var origin in this.notifications) {

s/var/let

@@ +61,5 @@
> +        if (this.notifications) {
> +          for (var origin in this.notifications) {
> +            this.byTag[origin] = {};
> +            for (var id in this.notifications[origin]) {
> +              var curNotification = this.notifications[origin][id];

s/var/let

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/57f27d290aea