Closed Bug 1159128 Opened 9 years ago Closed 9 years ago

notification from browser doesn't resent while restart and newly created notification doesn't sound/vibrate

Categories

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.2+, firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S12 (15may)
blocking-b2g 2.2+
Tracking Status
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
b2g-v2.0M --- wontfix
b2g-v2.1 --- affected
b2g-v2.1S --- affected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: johnhu, Assigned: gerard-majax)

References

()

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 7 obsolete files)

If we send notification from browser and reboot, the notification doesn't show up at lockscreen and notification tray.

STR:
  1. open URL, https://developer.mozilla.org/ja/docs/Web/API/notification , with browser. (tiny version url: http://tinyurl.com/muxm4fh)
  2. press |notify me!| button multiple times
  3. reboot phone
  4. check lockscreen and notification tray  <== NG


This issue brings another side-effect. If the issue affected, we will not have sound and vibration while we create new notifications until the resend counter is reached.

Expected:
  I don't know what's the correct behavior.

Actually:
  Notifications don't show up and newly created one doesn't give us sound and vibration.

I will paste the video url once it is uploaded.

-------------------------------------------------
The following is the investigations from partner:

[Possible Cause]
After restarting, Restoration process using the storage is performed, the resend process.
   * gecko/dom/notification/ChromeNotifications.js:L56 performResend()

Counter is incremented regardless of the success or failure notification.
   * Counter: resentNotifications
   * notification: appNotifier.showAppNotification()
   
The app finding failed in gecko/b2g/components/AlertHelper.jsm:L180 registerAppListener())
As a result, "this.resendReceived" (in gaia/smart-system(system)/js/notification.js) the less number of failures.

[Possible Root Cause]

 * gecko/dom/notification/Notification.cpp: L873 Notification::GetOrigin()
 In the case of browser:      nsContentUtils::GetUTFOrigin(principal, aOrigin);
  --> app get fail in gecko/b2g/components/AlertHelper.jsm:L180 registerAppListener()
   e.g.) When user push "notify-me" button on https://developer.mozilla.org/ja/docs/Web/API/notification,
         But system fails to find app by manifestURL. 
 In the case of application:  appsService->GetManifestURLByLocalId(appId, aOrigin);
  --> app get succeed in gecko/b2g/components/AlertHelper.jsm:L180 registerAppListener()


As shown in the following code, I think the problem occurs that is similar in Firefox OS.
gaia/apps/system/js/notification.js: L134
          case 'desktop-notification':
            this.addNotification(detail);
            if (this.isResending) {
              this.resendReceived++;
              this.isResending = (this.resendReceived < this.resendExpecting);
            }
I also found that the number of notifications without sound/vibration is as same as the number of undeleted browser notifications. What's worse, the number of notifications without sound/vibration would be accumulated with the number of undeleted browser notifications. The solution to reset the number of notifications without sound/vibration is re-flash the entire (including Gaia & Gecko) phone.

For example, restart the phone with one undeleted browser notification. After restarting, we send the notifications in UITests. We found that the first notification has no sound and vibration. Until the second notification sent, the sound and vibration works.

And then, we send a browser notification again. Restart the phone without deleting the browser notification. This time, there would be two notifications (the first notification and the second notification) without sound/vibration. Until the third notification sent, the sound and vibration works finally.
Assignee: nobody → kuoe0
Attached patch Bug-1159128.patch (obsolete) — Splinter Review
I found that the bug is caused by the wrong manifest URL when the browser notification is sending. The manifest URL is getting from the origin. So, the notification from other apps, the manifest URLs are "app://<domain name of the app>/manifest.webapp". For the browser notification, the manifest URLs are "http://<domain name of the notification from>".

Before gecko sending notifications, it'll check the manifest URLs by appService. If the manifest URL is not a "app://<domain name>/manifest.webapp", it will fail and won't send the notification. More detail at showNotification() in AlertsHelper.jsm.

And, when a browser notification is sent, gecko will get the manifest URL by the appId (which app sends the notification). So, when the browser notification is create, it will be sent normally. More detail at Notification::ShowInternal().

But when gecko stores the notifications into DB, it get the manifest URL only from origin. It will get wrong manifest URLs for browser notifications and save them into DB. After we restart the phone, gecko cannot resend the browser notifications because the wrong manifest URLs in DB.

The patch I create will get the right manifest URLs from appId. So, after we restart the phone, the browser notifications will be resent.

In addition, when we click a browser notification, no any action will happen. So, do we need to resend the browser notification? Or don't even put the browser notifications into DB.

I think we should confirm the right behavior for browser notifications on phones and TVs.
Flags: needinfo?(rob.a.mcdonald)
Flags: needinfo?(mhenretty)
Flags: needinfo?(hluo)
Attachment #8600776 - Flags: feedback?(mhenretty)
Currently, the notification handler is system app. The manifest URL get from appId of browser notifications is always "app://system.gaiamobile.org/manifest.webapp".
Attached patch check manifest URL before send (obsolete) — Splinter Review
After discuss with Hunter (UX of TV), he think we don't need to resend the browser notification. Because there is no action when we click the browser notification. So when we restart the b2g, we delete the browser notification directly from DB. And, it won't affect the counter of the notifications to resend anymore.

I create a patch to do the thing I mentioned above.
Attachment #8600776 - Attachment is obsolete: true
Attachment #8600776 - Flags: feedback?(mhenretty)
Attachment #8600859 - Flags: feedback?(mhenretty)
I know this is not a regression, but I think we need to triage this for 2.2 since it is pretty bad UX.
blocking-b2g: --- → 2.2?
Flags: needinfo?(mhenretty)
Whiteboard: [systemsfe]
Comment on attachment 8600859 [details] [diff] [review]
check manifest URL before send

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

First of all, thank you Tommy for the thorough investigation and alternative patches! It's very good work.

My personal feeling is that we want to treat browser notifications just like app notifications. Why would they be different? If I have a calendar web app, I would probably want those notifications to be resent on reboot. Same with a messaging web page like irc cloud. I will wait to see how Rob and Alex feel about it, but my opinion is we need to fix this for the browser.

If people agree, I think we should fix the manifestURL for browser notifications like your initial proposal.
Attachment #8600859 - Flags: feedback?(mhenretty) → feedback?(lissyx+mozillians)
So, if I summup correctly:
 - open browser app on device
 - trigger some notification

they work fine until you reboot where:
 - they are not resent
 - the failure messes up with the counter for the sound/vibration

And all of this because the origin (a website here) we retrieve when building/storing the notification, which is not an expected installed app?

If so, then I would agree with Michael.
That being said, looking at Gecko, shouldn't we already go through this code path in this case?
https://dxr.mozilla.org/mozilla-central/source/dom/notification/Notification.cpp#900

And not https://dxr.mozilla.org/mozilla-central/source/dom/notification/Notification.cpp#908
Flags: needinfo?(mhenretty)
(In reply to Alexandre LISSY :gerard-majax from comment #8)
> That being said, looking at Gecko, shouldn't we already go through this code
> path in this case?
> https://dxr.mozilla.org/mozilla-central/source/dom/notification/Notification.
> cpp#900
> 
> And not
> https://dxr.mozilla.org/mozilla-central/source/dom/notification/Notification.
> cpp#908

Yeah, that looks like what sound happen, but I haven't traced it myself. Tommy, can you answer this question?
Flags: needinfo?(mhenretty) → needinfo?(kuoe0)
After discussion. Since the TV browser is mainly for short term media/information browsing. If there are any uncleared notifications during the process of power-off. Then the notifications will be cleared automatically.
Flags: needinfo?(hluo)
(In reply to Alexandre LISSY :gerard-majax from comment #8)
> That being said, looking at Gecko, shouldn't we already go through this code
> path in this case?
> https://dxr.mozilla.org/mozilla-central/source/dom/notification/Notification.
> cpp#900

Yes. When notifications are created, they go through line 900. A browser notification will get a http protocol origin, not an app protocol origin. And then, Gecko stores the http protocol origin into DB. When Gecko want to resend the browser notifications, it would be failed. Because we pass the http protocol origin as the parameter to appsService.getManifestFor(), it returns null object. And then, the variable `app` is also a null object. Javascript would stop when the code want to access a null object.

See https://dxr.mozilla.org/mozilla-central/source/b2g/components/AlertsHelper.jsm?from=AlertsHelper.jsm#262.

As a result, all of the browser notifications won't be sent.

---

So we have different behaviors on phones and TVs?
Flags: needinfo?(kuoe0)
blocking-b2g: 2.2? → 2.2+
Right. I think we should rather fix this on AlertsHelper's and friends side: this code, as documented by the comment, makes sure we avoid getting spoof for apps. Hence I have several questions that maybe you can answer faster than I can dig into this issue:
 - should we even get a http origin as manifestURL ?
 - if having a http origin as "manifestURL" (which will obviously not be a manifest, according to what I'm understanding) is legal, should we make use of it to query appsService ?

My point being that maybe it makes no sense to go through this code in case of http origin if it's not pointing to a manifest.
Flags: needinfo?(mhenretty)
Flags: needinfo?(kuoe0)
Comment on attachment 8600859 [details] [diff] [review]
check manifest URL before send

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

::: dom/notification/ChromeNotifications.js
@@ +80,5 @@
>        debug("notification.body: " + notification.body);
>        debug("notification.data: " + notification.data);
>        debug("notification.mozbehavior: " + notification.mozbehavior);
>  
> +      app = appsService.getAppByManifestURL(notification.origin);

I'm not sure we should be doing this on ChromeNotifications side but rather not even bother to save notifications for which there is no valid manifest maybe: being able to handle resent notifications implies that we have a way to register a system message for notifications.

If this later condition cannot be fullfilled I think we should not even bother saving the notification in the database :)
Attachment #8600859 - Flags: feedback?(lissyx+mozillians)
Well it looks like I'm outnumbered here in thinking browser notifications should be persistent across reboots. That being the case, I agree with Alex that we should just avoid storing browser (ie. non-app) notifications in the DB in the first place. But we also need to deal with the case where a user will have browser notifications in the DB when performing an upgrade, and so I think we still need to perform the check in ChromeNotifications.js in attachment 8600859 [details] [diff] [review].

Reading recent changes to the spec [1], Notification.get has been deprecated, and browser notifications not tied to a service worker are no longer considered persistent. So this change makes sense from a spec perspective. The next step would be to make Notification.get a privileged API, but that is obviously out of the scope of this bug.

1.) https://notifications.spec.whatwg.org/
Flags: needinfo?(mhenretty)
Hi all,

I would like to un-assign Tommy from connected device team from this bug since 
  1. Tommy already provided a patch which follows UX from Stingray project.
  2. And there is no conclusion here yet which is not controlled by Tommy.

Once there is a conclusion I think the 2.2 triage meeting will find out the resource to help it.
By the way, Tommy can still be consulted for his implementation now.
Assignee: kuoe0 → nobody
(In reply to Michael Henretty [:mhenretty] from comment #14)
> Well it looks like I'm outnumbered here in thinking browser notifications
> should be persistent across reboots. 


My 2 cents here is that I have mixed feelings.

On one side I agree with you, because there is no reason we should handle them differently.

On the other side, what should happen when the user taps the notification? We have no system messages for the browser... should we open the Browser page? And then what happens? How the web application knows about the notification that was clicked? We need to answer these questions to move forward :)
Alex, can you take this one?
Assignee: nobody → lissyx+mozillians
Yep. We need to decide, though. I don't have a strong opinion except what I stated earlier: if we have no way for the resent notification to be useful to the webpage which made use of it, let's not even keep them. And we will have to handle the migration case of course :(
(In reply to Julien Wajsberg [:julienw] (PTO May 8th -> May 17th) from comment #16)
> On the other side, what should happen when the user taps the notification?
> We have no system messages for the browser... should we open the Browser
> page? And then what happens? How the web application knows about the
> notification that was clicked? We need to answer these questions to move
> forward :)

My first thought was that the system app handles the system message, and then opens up the origin the notification was tied to. But after seeing that WHATWG deprecated persistent notifications not tied to a service worker, I think the best way forward here is simply not storing browser notifications. This means that until we get service workers, Notification.get and resending notifications on reboot will be Firefox OS propriety functionality.
Component: Gaia::System → Gaia::System::Status bar, Utility tray, Notification
Right. Michael, what should we consider a notification sent from the Browser ? One for which we have no manifestURL and a http/https origin?
Flags: needinfo?(mhenretty)
(In reply to Alexandre LISSY :gerard-majax from comment #12)

Sorry for the late reply. I think we get a http origin is valid. But if we use it to query appsService, I think it is invalid. In my view, appsService would only accept an app origin.

In addition, I think the function 'getManifestFor' in appsService.js should continue with the Promise.reject function rather than return a null object.

See: https://dxr.mozilla.org/mozilla-central/source/dom/apps/AppsService.js#64-72
Flags: needinfo?(kuoe0)
Given Tommy, Michael and myself do agree on this, we'll fix it like exposed in comment 21:
 - consider a manifest url for an origin http/https being illegal, so make it reject the promise
 - on promise reject, do not store the notification
 - for notifications already stored with invalid origin, just silently delete from the database
Flags: needinfo?(mhenretty)
Attached file notificationstore.json
Patch in attachment 8604107 [details] [diff] [review] is WIP but it should already be good enough to cross check. Tommy, would you mind giving a try to this one?
Flags: needinfo?(kuoe0)
(In reply to Alexandre LISSY :gerard-majax from comment #26)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a58a80e14acb

Failures everywhere for test_notification_storage.html \o/
Comment on attachment 8604107 [details] [diff] [review]
Only save valid app notifications r=mhenretty

Michael, if you agree with this, I'll finish the tests :)
Attachment #8604107 - Flags: feedback?(mhenretty)
Depends on: 1163997
Comment on attachment 8604107 [details] [diff] [review]
Only save valid app notifications r=mhenretty

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

NotificationStorage being the gatekeeper for the NotificationDB sounds good to me. That being the case, I think it makes sense to have NotificationStorage itself check for canPut before saving to NotificationDB as well.

::: dom/interfaces/notification/nsINotificationStorage.idl
@@ +95,5 @@
>    void delete(in DOMString origin,
>                in DOMString id);
> +
> +  /**
> +   * Check if it makes sense to store this notification

nit: let's be more explicit in the description about which notifications we care about saving, rather than just in the @param field.

@@ +98,5 @@
> +  /**
> +   * Check if it makes sense to store this notification
> +   *
> +   * @param origin: Origin from which the notification is sent. Per spec, it
> +   *                only makes sense to save the notifications coming from apps

To be clear, the spec says we shouldn't store notifications at all (until later when we get service workers). This line makes it seem like the spec tells us to only save app notifications, but really saving them is just something we do for resend ability in b2g. I'm probably splitting hairs though.

::: dom/notification/NotificationDB.jsm
@@ +80,5 @@
>        this.unregisterListeners();
>      }
>    },
>  
> +  preProcess: function(notifications) {

maybe we should call this `filterNonAppNotifications` or something
Attachment #8604107 - Flags: feedback?(mhenretty) → feedback+
Using a pref for the test do not makes me happy, but I have not been able to
replace the already running appsService.
Attachment #8600859 - Attachment is obsolete: true
Attachment #8604107 - Attachment is obsolete: true
(In reply to Michael Henretty [:mhenretty] from comment #29)

[...]

> NotificationStorage being the gatekeeper for the NotificationDB sounds good
> to me. That being the case, I think it makes sense to have
> NotificationStorage itself check for canPut before saving to NotificationDB
> as well.

But checking like this did broke Notification.get() (and this was caught by test_notification_storage :)).
(In reply to Alexandre LISSY :gerard-majax from comment #30)
> Created attachment 8604761 [details] [diff] [review]
> Only save valid app notifications
> 
> Using a pref for the test do not makes me happy, but I have not been able to
> replace the already running appsService.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dec0a768a1e
Attachment #8604761 - Attachment is obsolete: true
Attachment #8604776 - Flags: review?(mhenretty)
Flags: needinfo?(rob.a.mcdonald)
(In reply to Alexandre LISSY :gerard-majax from comment #33)
> Created attachment 8604776 [details] [diff] [review]
> Only save valid app notifications

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ae1b82bad8
this one should check all platforms are okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4225936a030b
Flags: needinfo?(kuoe0)
Comment on attachment 8604776 [details] [diff] [review]
Only save valid app notifications

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

Looks good! I have a small question below, but overall this seems good to go.

::: dom/interfaces/notification/nsINotificationStorage.idl
@@ +100,5 @@
> +   * least for now. But we want to be able to have this behavior on B2G. Thus,
> +   * this method will check if the origin sending the notifications is a valid
> +   * registered app with a manifest or not. Hence, a webpage that has none
> +   * will have its notification sent and available (via Notification.get())
> +   * during the life time of the page.

:)

::: dom/notification/NotificationDB.jsm
@@ +116,5 @@
>              }
>            }
>          }
> +
> +        this.save();

Do we really need to save here? My point is, this will be run every time a new tab loads and most of the time we won't be filtering anything. In the cases where we are filtering, couldn't we just defer this save until that webpage sends a new notification? It seems like this save is adding a lot of writes to disk when most times we don't need it.
Attachment #8604776 - Flags: review?(mhenretty) → review+
This does address previous review comments, and drops the need for a pref.
Hopefully this will be green on B2G ICS Emulator too. The way we fake the app
might not be the nicest, but it works.
Attachment #8604776 - Attachment is obsolete: true
Attachment #8605213 - Flags: review?(mhenretty)
This should hopefully be all green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18cf7d5b4eb1
Comment on attachment 8605213 [details] [diff] [review]
Only save valid app notifications

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

Nice job not having to use test-only pref!

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +400,5 @@
> +	      }
> +
> +              delete Webapps.DOMApplicationRegistry.webapps[aAppId];
> +	      return true;
> +	    }

clever!
Attachment #8605213 - Flags: review?(mhenretty) → review+
So B2G ICS Emulator failures might simply be because:
 - on those, running mozAppps.getSelf() inside mochitest do returns something
 - so the mochitest noresend page does have a valid app manifest, so it passes canPut() in NotificationStorage

For the Browser e10s failures, all I could see is that the faked app do not gets into the registry :(. Given we indeed really need this behavior to be checked only on Gonk and I have already investigated a lot unsuccessfully, I might be tempted to just skip this on e10s for now.
Attachment #8605213 - Attachment is obsolete: true
Attachment #8605272 - Flags: review+
(In reply to Alexandre LISSY :gerard-majax from comment #42)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2230c04639fa

Cancelled by error
(In reply to Alexandre LISSY :gerard-majax from comment #45)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ef7a6b807eb

e10s green and b2g ics emulator green
Flags: needinfo?(mhenretty)
Michael, the needinfo is so that you can say if you agree with those latest changes :)
Michael, how should we handle this case:
 - have "invalid" notifications pre-existing in the db (notification from a webpage)
 - on boot, NotificationDB loads notificationstore.json and removes those invalid notification
 - NotificationDB.notifications reflects the expected state at that moment
 - use the device with no new notification being triggered from valid app
 - reboot

Since notificationstore.json will be populated by taskSave(), so by NotificationStorage.put(), we will have removed the notification from the db for nothing and upon next boot we will have to do it again. User will never see it though, so we should be ok spec-wise. 

Now, we can either:
 - consider it's not a big deal, and that during real usage there will always be some notification being sent at some point
 - revive the this.save() call at the end of NotificationDB.load(), with the risks you suggested (is it really a problem? we are supposed to call load() only when NotificationDB starts, so not that much)
 - force a sync during shutdown (maybe earlier than xpcom-shutdown though)

I'd like that we do not land this without making sure we thought about it, otherwise I fear we might get bug reports for this.
(In reply to Alexandre LISSY :gerard-majax from comment #48)
> Michael, how should we handle this case:
>  - have "invalid" notifications pre-existing in the db (notification from a
> webpage)
>  - on boot, NotificationDB loads notificationstore.json and removes those
> invalid notification
>  - NotificationDB.notifications reflects the expected state at that moment
>  - use the device with no new notification being triggered from valid app
>  - reboot
> 
> Since notificationstore.json will be populated by taskSave(), so by
> NotificationStorage.put(), we will have removed the notification from the db
> for nothing and upon next boot we will have to do it again. User will never
> see it though, so we should be ok spec-wise.

My thinking is that if a web page already has notifications in the DB, then the likelihood of that page sending another notification is rather high. In the case where it doesn't, you are right we will keep these notifications in the store and run them through the filter logic each time we boot the NotificationsDB. In the end, it's a question of which case we are optimizing for. Do we want to always write to disk on every NotificationDB load, or re-run the filter logic for the web-pages a user visited in the past that sent notifications but now no longer does. I feel like the first case happens more often, and so should be optimized there.

Whatever we do, I don't think we should save before xpcom-shutdown.
Flags: needinfo?(mhenretty)
Comment on attachment 8605272 [details] [diff] [review]
Only save valid app notifications

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

::: dom/notification/NotificationDB.jsm
@@ +100,5 @@
>        function onSuccess(data) {
>          if (data.length > 0) {
> +	  // Preprocessing phase intends to cleanly separate any migration-related
> +          // tasks.
> +	  this.notifications = this.preProcess(JSON.parse(data));

I still think we should either call this `filterNonAppNotifications`, or make this comment clearer that we are filtering non-app notifications that might still be around from before an upgrade.
Attachment #8605272 - Attachment is obsolete: true
Attachment #8605913 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54e9eb7818ba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S12 (15may)
Flags: needinfo?(hcheng)
Blocks: 960762
Could you please uplift to v2.2 since it is a blocker?
Flags: needinfo?(hcheng) → needinfo?(lissyx+mozillians)
Flags: needinfo?(lissyx+mozillians)
Comment on attachment 8605913 [details] [diff] [review]
Only save valid app notifications

NOTE: 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 #): N/A
User impact if declined: breaks some notification handling code: device vibrates and sounds when it should not
Testing completed: on device and added mochitests
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #8605913 - Flags: approval-mozilla-b2g37?
Hi Hermes,
Can you help to verify on m-c?
Thanks!
Flags: needinfo?(fan.luo) → needinfo?(hcheng)
I'm trying to verify this bug on master, but it's not clear of what the expected behavior is? On today's build, I can reproduce the bug where after reboot there's no notifications on lockscreen or notification tray, but I can't reproduce the bug where future notifications become silent.
Currently, the notification from webpage or test app would not be silent anymore. It would trigger vibration and sound after rebooting. All the notifications can be shown on notification tray.

However, just like comment 59, I am not so sure if it is correct when a notification trigger from a web page (https://developer.mozilla.org/ja/docs/Web/API/notification) is not shown at lockscreen.
Flags: needinfo?(hcheng) → needinfo?(lissyx+mozillians)
Can you be more specific ? The behavior described in comment 59 looks indeed correct to me: those notifications will not be stored, so they will not be presented again after a reboot, and it does imply that it fixes the fact that future notifications were silent. Behavior from this comment does confirm it is fixed.
Flags: needinfo?(pcheng)
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(hcheng)
Yes, the silent problem is fixed.

My question in comment 60 is why the notification triggered from the page or the test app (UI tests) is not shown at Lockscreen. But, I think I have found the reason which is that it have to be put in [1] to shown on lockscreen.

@Josh, maybe you can approve it to 2.2?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/manifest.webapp#L99
Flags: needinfo?(hcheng)
Flags: needinfo?(jocheng)
(In reply to Hermes Cheng[:hermescheng] from comment #62)
> Yes, the silent problem is fixed.
> 
> My question in comment 60 is why the notification triggered from the page or
> the test app (UI tests) is not shown at Lockscreen. But, I think I have
> found the reason which is that it have to be put in [1] to shown on
> lockscreen.

Your link is unrelated, it's for the system message. That being said, I just rechecked on Mulet and on Z3 Compact with a Gecko from yesterday and it's all fine. Showed this to Johan too. I'm not sure to get your point regarding "shown on lockscreen": notifications are in the tray, and will be displayed on the lockscreen only if they are triggered while your device is locked.

Since your device is obviously not locked when you tap on the "Notify me!" button on a webpage, it's expected you just see them in the notification tray.

> 
> @Josh, maybe you can approve it to 2.2?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/manifest.
> webapp#L99
Comment on attachment 8605913 [details] [diff] [review]
Only save valid app notifications

Keeping verifyme as QA can verify when patch landed on 2.2.
Flags: needinfo?(jocheng)
Attachment #8605913 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(In reply to Alexandre LISSY :gerard-majax from comment #61)
> Can you be more specific ? The behavior described in comment 59 looks indeed
> correct to me: those notifications will not be stored, so they will not be
> presented again after a reboot, and it does imply that it fixes the fact
> that future notifications were silent. Behavior from this comment does
> confirm it is fixed.

Since it was not explained what the expected behavior should be on comment 0, I had to ask to make sure what I was seeing is the expected for the fix. So this issue is verified as fixed on Flame 3.0.

Fixed behavior observed: Following STR, after reboot I didn't see notifications show up on lockscreen or notification tray. Further notifications did have sound (this part is fixed).

Device: Flame (KK, full flashed, 319MB)
BuildID: 20150519010201
Gaia: 762cbd16712484f93f485e89f5363686540a3db7
Gecko: f65cc0022a0e
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Leaving verifyme for 2.2 verification after uplift.
Status: RESOLVED → VERIFIED
Flags: needinfo?(pcheng) → needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e0c74bd3e489

I fixed up the whitespace for the uplift, but we should probably still get a follow-up patch landed on trunk as well.
Flags: in-testsuite+
@Pi Wei,
Could you help verify 2.2 once the patch is included in pvt build?
Depends on: 1167056
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(pcheng)
(In reply to Hermes Cheng[:hermescheng] from comment #68)
> @Pi Wei,
> Could you help verify 2.2 once the patch is included in pvt build?

Sure.

This issue has been verified fixed on Flame 2.2. Same behavior is observed as comment 65.

Device: Flame (KK, full flashed, 319MB)
BuildID: 20150601002502
Gaia: b4582cc394e0919623263997c0cdb0b4751a1403
Gecko: 78d8b0a4303d
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(pcheng) → needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.