874364 - [system] Notifications should persist at reboot

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Juan Perez-Bedmar [:juanpbf]]

Seen in  Ikura
QC RIL version:
"ro.build.firmware_revision=V1.01.00.01.019.107"
gaia commit:
e980247 Merge pull request #9591 from crdlc/bug-868258
gecko commit:
85cfe31 Backed out changeset 63dfdb7a83c6 (bug 868932) for bustage. a=backout


"New SMS" notifications disappear if you reboot the handset

Steps to reproduce:
1. Send a SMS to the DuT
2. Verify the reception of the message and the New SMS notification on the DuT
3. Restart the DuT

Current result:
Once you restart the device, the notification has disappeared.

Expected result:
The notification must remain until the SMS is read, enven after restarting the device.

Comment 1

[Juan Perez-Bedmar [:juanpbf]]

Nominating it for leo?

Comment 2

[Rick Waldron [:rwaldron]]

Confirmed, but I don't think this is specifically controlled by the Messages app.

cc'ing Julien and Corey

Comment 3

[Julien Wajsberg [:julienw]]

We don't persist any notification (except the updates notifications). Rick, you're right when you say that this is not controlled by the Messages app.

Afaik this is by design for the current versions. Josh, please enlightens us with your wisdom:)

Comment 4

[Josh Carpenter [:jcarpenter]]

Ha, you're too kind. We did not spec this, but messages should persist after reboot. If I update Gecko, for example, I still want to see my pending notifications when the device turns back on. Having them disappear might cause me to miss and important communication. 

In other words, I agree with the expected:

> The notification must remain until the SMS is read, 
> even after restarting the device.

cc'ing Rob for needsinfo commentary to confirm, as he is doing IxD patterns review with Francis.

Comment 5

[Julien Wajsberg [:julienw]]

okay, this is non-trivial (but interesting !) work ;) Let's see if this will block leo...

Comment 6

[Rob MacDonald [:robmac]]

(In reply to Josh Carpenter [:jcarpenter] from comment #4)
> 
> In other words, I agree with the expected:
> 
> > The notification must remain until the SMS is read, 
> > even after restarting the device.
> 

Absolutely agree. This is something we'll need to include in upcoming notification specs.

Thanks for the heads-up on this!

Comment 7

[Evan Tseng [:evanxd]]

Hi all,

I found out that the notification id seemed to be inexistent after we reboot the device.

Because I used the WIP patch https://github.com/evanxd/gaia/commit/badc9fc472996128bd461260bc8413ba52a9f949 to save the notification item after reboot the device, then clicked the notification and the function registered in navigator.mozSetMessageHandler('notification', function(){}); wasn't triggered.

So I think this bug is also related with Gecko module. We also need to fix the issue in Gecko.

Comment 8

[Evan Tseng [:evanxd]]

The STP to reproduce that the function registered in navigator.mozSetMessageHandler('notification', function(){}) wasn't trigger.

1. cherry-pick the patch: https://github.com/evanxd/gaia/commit/badc9fc472996128bd461260bc8413ba52a9f949 (now you could save the notification after reboot the device)
2. Make phone call to the device for adding a notification item.
3. Reboot the device. (Dialer App didn't run in the background)
4. Click the notification item.

Expected:
The dialer was triggered.

Currently:
Nothing happen.

Comment 9

[[:fabrice] Fabrice Desré]

There is absolutely no persistence of notifications on the gecko side. Please file a bug blocking this one if we really need that.

Comment 10

[Evan Tseng [:evanxd]]

Hi Fabrice,

I think we need the feature.
This bug is a leo+ one.

So let me file a bug for Gecko side.
Thanks. :)

Comment 11

[Evan Tseng [:evanxd]]

Hi Fabrice,

The gecko side bug http://bugzil.la/881159 was created.
Please help us fix the bug.

Thanks. :)

Comment 12

[Evan Tseng [:evanxd]]

Hi Leo,

I think this bug might not be resolved in QE3.
Because the Bug 881159 is blocking it.

Could we change the Target Milestone?
Thanks. :)

Comment 13

[Leo]

(In reply to Evan Tseng [:evanxd] from comment #12)

Since this seems a new feature, let me remove leo+ for now.

Comment 14

[Wayne Chang [:wchang]]

Partner agrees to not introduce the risk for leo. Nominating for koi?

Comment 16

[Julien Wajsberg [:julienw]]

All this will be done by Gregor's system team and their current effort with notifications.

Comment 18

[lecky]

Hi,this feature will be supported in V1.2,right?

Comment 19

[Jason Smith [:jsmith]]

Triage - not blocking. This is something that should be fixed, but not a blocker for release.

Comment 21

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

I'm working on this as part of bug 899574.

Comment 22

[Gregor Wagner [:gwagner]]

(In reply to lecky from comment #18)
> Hi,this feature will be supported in V1.2,right?

This will be in 1.3.

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Noming this for 1.3 based on comment 22

Comment 24

[lecky]

hi beatriz:
Can you accept the conclusion that this feature would not implemented on V1.1HD?
Thank you very much!

Comment 25

[Beatriz Rodríguez [:brg]]

(In reply to lecky from comment #24)
> hi beatriz:
> Can you accept the conclusion that this feature would not implemented on
> V1.1HD?
> Thank you very much!
Yes, we accept current roadmap of this fuctionality --> v1.3 (thanks Gregor)

Comment 27

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

Now that bug 899574 has landed, we need to do two things to get this working:

1.) Add a special API for fetching all notifications across apps/domains in Gecko. Perhaps something like
  - navigator.mozGetAllNotifications(). This will create and re-send the notifications in such a way that things like click events will still work as expected (see bug 874364).

2.) Add functionality in the Gaia system app to fetch all open notifications when the system boots.

Comment 28

[Beatriz Rodríguez [:brg]]

This feature had been requested again and added in the certification requirements for the future. Can we add it in v1.4 ?

Comment 29

[Peter Dolanjski [:pdol]]

Gregor, with the notifications API finished (from what I can tell), what is required to finish this?

Comment 30

[:gerard-majax]

(In reply to Peter Dolanjski [:pdol] from comment #29)
> Gregor, with the notifications API finished (from what I can tell), what is
> required to finish this?

Basically, what we are lacking is an API for the system app to retrieve all the notification and show them. As far as I've played with notifications for now, we are correctly storing them so persistence is now just a matter of displaying them.

Comment 31

[:gerard-majax]

How should we handle this need ?

Comment 32

[Anne (:annevk)]

The Notifications API does not deal with the "notification center" functionality of a given platform. It's an API for sites.

Comment 33

[Julien Wajsberg [:julienw]]

I think you'd need either another privileged API or a privileged method. I believe it is possible to add a method visible only if the app holds a particular permission.

Comment 34

[Gregor Wagner [:gwagner]]

Michael already outlined the steps here in comment 27. This has some security implications.
Paul, Ehsan, any objections to this plan?

Comment 35

[(no longer active)]

(In reply to Gregor Wagner [:gwagner] from comment #34)
> Michael already outlined the steps here in comment 27. This has some
> security implications.
> Paul, Ehsan, any objections to this plan?

If you mean adding a certified only API for this purpose, that seems fine.  (We may even want to restrict it further through a permission to the system app only.)

Note that Boris has patches in bug 958667 awaiting review which help make it very easy to add certified only APIs.

Comment 36

[Gregor Wagner [:gwagner]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #35)
> (In reply to Gregor Wagner [:gwagner] from comment #34)
> > Michael already outlined the steps here in comment 27. This has some
> > security implications.
> > Paul, Ehsan, any objections to this plan?
> 
> If you mean adding a certified only API for this purpose, that seems fine. 
> (We may even want to restrict it further through a permission to the system
> app only.)
> 
> Note that Boris has patches in bug 958667 awaiting review which help make it
> very easy to add certified only APIs.

Thanks Ehsan. Seems like this is close to land. Lets wait for it.

Comment 37

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Gregor Wagner [:gwagner] from comment #34)
> Michael already outlined the steps here in comment 27. This has some
> security implications.
> Paul, Ehsan, any objections to this plan?

Comment 35 sounds fine to me, seems like an additional permission would be a good idea though. 

Just to clarify, we are talking about desktop notifications here, ie [1], the kind that are sent via the 'notification' system message? I was just a bit confused because of comment 0 being about sms messages.


[1] https://developer.mozilla.org/en-US/docs/Web/API/notification

Comment 38

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

(In reply to Paul Theriault [:pauljt] from comment #37)
> Just to clarify, we are talking about desktop notifications here, ie [1],
> the kind that are sent via the 'notification' system message? I was just a
> bit confused because of comment 0 being about sms messages.

That's right, this is about web notifications persisting in general upon device reboot. The initial context for this bug was for SMS, but now this bug pertains to all app notifications.

Comment 39

[Beatriz Rodríguez [:brg]]

Peter, this requirement is a priority reported by the carriers where we run cert process, could you please confirm if this will land in v1.4?

Comment 40

[:gerard-majax]

Stealing this, because I'm a bad guy :)

Comment 41

[Peter Dolanjski [:pdol]]

(In reply to Beatriz Rodríguez [:brg] from comment #39)
> Peter, this requirement is a priority reported by the carriers where we run
> cert process, could you please confirm if this will land in v1.4?

I think we should do our best to deliver this in 1.4.  I'm hesitant to block on it as we have API dependencies that need to be worked out and I suspect that we could ship without it if we REALLY had to.

Comment 42

[:gerard-majax]

Bug 967475 provides Gecko and Gaia patches that enables this. However, this is still preliminary work, and this lacks:
 - security, we do not want to expose this new API to everyone
 - UX, we need to discuss especially regarding the notification sound
 - testing, we have no test at all for now

I have no idea whether we can achieve those in time for v1.4.

Meanwhile, this should be working more or less not that bad, so if you want to give it a try and provide feedback on the feature itself, feel free.

Comment 43

[Beatriz Rodríguez [:brg]]

Hi Alexandre, thanks for your good work here and sorry for my delay. It seems to work fine, and i have even misbehave by removing the battery without pressing the power off button and the notifications are there when you turn the device on back. Congrats :-)

The two comments:
First, the time notified in the event after turnning on the device is compare to the time the device is turned on, not when the event happened.
Example: in the attachment, the call was made at 2:48pm. I turned on the device back at 3:05pm. It said that the last call is from 1min ago... but the real time is: 18min. However I do not know if you can get the real timing.

Second comment:, 
-if I clear all the pending notifications with the "Clear all" option in the utility tray. 
-Then receive a new missed call, the device notifies I have one pending call. 
-Restart the device
-When turning it on back, you will see the last call + the old notifications as New, when I guess that the device should only show the last one as new notifications. Am I right?

Comment 44

[Beatriz Rodríguez [:brg]]

Attachment: 2014-02-10-15-06-35.png

Comment 45

[:gerard-majax]

(In reply to Beatriz Rodríguez [:brg] from comment #43)
> Hi Alexandre, thanks for your good work here and sorry for my delay. It
> seems to work fine, and i have even misbehave by removing the battery
> without pressing the power off button and the notifications are there when
> you turn the device on back. Congrats :-)

Congrats to Michael for this :)

> 
> The two comments:
> First, the time notified in the event after turnning on the device is
> compare to the time the device is turned on, not when the event happened.
> Example: in the attachment, the call was made at 2:48pm. I turned on the
> device back at 3:05pm. It said that the last call is from 1min ago... but
> the real time is: 18min. However I do not know if you can get the real
> timing.

Yes, this is documented and fixed in bug 968955 that you should apply also.

> 
> Second comment:, 
> -if I clear all the pending notifications with the "Clear all" option in the
> utility tray. 
> -Then receive a new missed call, the device notifies I have one pending
> call. 
> -Restart the device
> -When turning it on back, you will see the last call + the old notifications
> as New, when I guess that the device should only show the last one as new
> notifications. Am I right?

It's a bug but not of this code. When you issued the "Clear all", the notification should have been removed already. If we popup it again, it's because it is still here.

Comment 46

[:gerard-majax]

UX people, the current behavior is that we ring/vibrate when showing the notification. This means that with this feature applied, then we will do it while displaying the initlog. Do we want this, or another behavior?

Comment 47

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #46)
> UX people, the current behavior is that we ring/vibrate when showing the
> notification. This means that with this feature applied, then we will do it
> while displaying the initlog. Do we want this, or another behavior?

Comment 48

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #42)
> Bug 967475 provides Gecko and Gaia patches that enables this. However, this
> is still preliminary work, and this lacks:
>  - security, we do not want to expose this new API to everyone
>  - UX, we need to discuss especially regarding the notification sound
>  - testing, we have no test at all for now
> 

I've just updated patches with some security checks:
 - limiting the scope of the new API to certified apps
 - limiting the use of mozResendAllNotifications() to the 'desktop-notification-resend' permission

Comment 49

[Rob MacDonald [:robmac]]

(In reply to Alexandre LISSY :gerard-majax from comment #47)
> (In reply to Alexandre LISSY :gerard-majax from comment #46)
> > UX people, the current behavior is that we ring/vibrate when showing the
> > notification. This means that with this feature applied, then we will do it
> > while displaying the initlog. Do we want this, or another behavior?

Hi Alexandre...

I prefer if the ring/vibrate only took place when the notifications are actually taking place as opposed to on reboot. So, if the phone reboots and there are new notifications since the phone was shut down (a new email for example), then ring/vibrate. If the phone reboots and it is showing the same notifications as when it was shut down, there should not be a notification sound or vibration.

- Rob

Comment 50

[:gerard-majax]

(In reply to Rob MacDonald [:robmac] from comment #49)
> (In reply to Alexandre LISSY :gerard-majax from comment #47)
> > (In reply to Alexandre LISSY :gerard-majax from comment #46)
> > > UX people, the current behavior is that we ring/vibrate when showing the
> > > notification. This means that with this feature applied, then we will do it
> > > while displaying the initlog. Do we want this, or another behavior?
> 
> Hi Alexandre...
> 
> I prefer if the ring/vibrate only took place when the notifications are
> actually taking place as opposed to on reboot. So, if the phone reboots and
> there are new notifications since the phone was shut down (a new email for
> example), then ring/vibrate. If the phone reboots and it is showing the same
> notifications as when it was shut down, there should not be a notification
> sound or vibration.
> 
> - Rob

Thanks, this is done.

Also, I added unit test on the gaia side now, to make sure we correctly disable and enable vibration and sound. Work is ongoing regarding tests on the Gecko side.

Comment 51

[Jason Smith [:jsmith]]

Unblocking per drivers decision - this feature isn't a QC requested feature, so this isn't allowed to be part of 1.4.

Comment 52

[Julien Wajsberg [:julienw]]

Look at the duplicate line: partners actually requested this quite a lot.

Also, I think the sentence "this isn't allowed to be part of 1.4." is a little too strong, you probably meant "it's not a blocker but can land when it's ready", right?

Comment 53

[Juan Perez-Bedmar [:juanpbf]]

We are about to start a new IOT process with several operators with our current build based on V1.3, and I'm sure they will report, again, this bug. 
By the time I reported this bug, with V1.0.1, I persuade them to accept this in a "future version", but I won't be able to play this cards much longer. 

I do think this should be fixed for 1.4

Comment 54

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #52)
> Look at the duplicate line: partners actually requested this quite a lot.
> 
> Also, I think the sentence "this isn't allowed to be part of 1.4." is a
> little too strong, you probably meant "it's not a blocker but can land when
> it's ready", right?

I meant exactly what I stated. Talk to your manager about this - he'll give you more context on what's going on here with 1.4 with feature work.

(In reply to Juan Perez-Bedmar [:juanpbf] from comment #53)
> We are about to start a new IOT process with several operators with our
> current build based on V1.3, and I'm sure they will report, again, this bug. 
> By the time I reported this bug, with V1.0.1, I persuade them to accept this
> in a "future version", but I won't be able to play this cards much longer. 
> 
> I do think this should be fixed for 1.4

Talk to your TAM about this. There's larger concerns here at play here with 1.4 that resulted in comment 51's decision due to schedule changes that have been imposed on the release.

Comment 55

[Julien Wajsberg [:julienw]]

I got more context in the mean time.

Comment 56

[:gerard-majax]

(In reply to Juan Perez-Bedmar [:juanpbf] from comment #53)
> We are about to start a new IOT process with several operators with our
> current build based on V1.3, and I'm sure they will report, again, this bug. 
> By the time I reported this bug, with V1.0.1, I persuade them to accept this
> in a "future version", but I won't be able to play this cards much longer. 
> 
> I do think this should be fixed for 1.4

Considering we want to prioritize stability and avoid regressions, I think it's safer to postpone a bit: Notification API is still brand new, and we already got a couple of regressions by its use. Implementing persistence after reboot will probably bring new ones.

Comment 57

[:gerard-majax]

Beatriz, the feature itself should be quite good now. If you want to try it again, feel free.

Comment 58

[Beatriz Rodríguez [:brg]]

(In reply to Alexandre LISSY :gerard-majax from comment #57)
> Beatriz, the feature itself should be quite good now. If you want to try it
> again, feel free.

Hi Alexandre, we had tested this feature and we had seen two strange things that are not 100% reproducible:
- After flashing the device and having a few notifications in utility tray all of them are lost after reboot; it fails the first time, many times the second time and seems to be always working from the third time (from that point pending notifications always persist after reboot)
-Sometimes having an existing conversation(never seen with a only one new message), when you receive a new message and without accessing to it then restart the device. Access the message through notification utility tray, the pending messages blue icon within messaging app is lost...

The only thing we can reproduce 100% is:
- Missed Call -> Restart -> accessing to Call log -> Missed calls --> it doesn't remove the notification from utility tray. It is necessary to access twice to missed calls menu.

Thanks for your work and sorry for the delay.

Comment 59

[:gerard-majax]

(In reply to Beatriz Rodríguez [:brg] from comment #58)
> (In reply to Alexandre LISSY :gerard-majax from comment #57)
> > Beatriz, the feature itself should be quite good now. If you want to try it
> > again, feel free.
> 
> Hi Alexandre, we had tested this feature and we had seen two strange things
> that are not 100% reproducible:
> - After flashing the device and having a few notifications in utility tray
> all of them are lost after reboot; it fails the first time, many times the
> second time and seems to be always working from the third time (from that
> point pending notifications always persist after reboot)

I also ran into this issue. From what I could check, OS.File.writeAtomic() was not doing its job correctly and not writing notifications to disk. Not my fault :)

Comment 60

[Jason Smith [:jsmith]]

This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 61

[:gerard-majax]

Fixed by landing the gecko and gaia patches in bug 967475.