Closed Bug 874364 Opened 7 years ago Closed 6 years ago

[system] Notifications should persist at reboot

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S2 (23may)
tracking-b2g backlog

People

(Reporter: juanpbf, Assigned: gerard-majax)

References

Details

(Whiteboard: [ucid: comms47, 2.0, FT:COMMS][systemsfe], [p=8], [priority])

Attachments

(1 file)

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.
Nominating it for leo?
blocking-b2g: --- → leo?
Confirmed, but I don't think this is specifically controlled by the Messages app.

cc'ing Julien and Corey
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:)
Flags: needinfo?(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.
Flags: needinfo?(jcarpenter) → needinfo?(rmacdonald)
Component: Gaia::SMS → Gaia::System
okay, this is non-trivial (but interesting !) work ;) Let's see if this will block leo...
(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!
Flags: needinfo?(rmacdonald)
blocking-b2g: leo? → leo+
Target Milestone: --- → 1.1 QE3
Assignee: nobody → 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.
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.
There is absolutely no persistence of notifications on the gecko side. Please file a bug blocking this one if we really need that.
Hi Fabrice,

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

So let me file a bug for Gecko side.
Thanks. :)
Depends on: 881159
Hi Fabrice,

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

Thanks. :)
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. :)
Flags: needinfo?(leo.bugzilla.gaia)
(In reply to Evan Tseng [:evanxd] from comment #12)

Since this seems a new feature, let me remove leo+ for now.
blocking-b2g: leo+ → -
Flags: needinfo?(leo.bugzilla.gaia)
Target Milestone: 1.1 QE3 (24jun) → 1.1 QE4
Partner agrees to not introduce the risk for leo. Nominating for koi?
blocking-b2g: - → koi?
Duplicate of this bug: 898723
Assignee: evanxd → nobody
All this will be done by Gregor's system team and their current effort with notifications.
Summary: "New SMS" notification disappears after rebooting handset → [system] Notifications should persist at reboot
Duplicate of this bug: 905922
Hi,this feature will be supported in V1.2,right?
Flags: needinfo?(anygregor)
Whiteboard: [systemsfe]
Triage - not blocking. This is something that should be fixed, but not a blocker for release.
blocking-b2g: koi? → -
Duplicate of this bug: 870280
Assignee: nobody → mhenretty
Depends on: 899574
I'm working on this as part of bug 899574.
Blocks: 881159
No longer depends on: 881159
(In reply to lecky from comment #18)
> Hi,this feature will be supported in V1.2,right?

This will be in 1.3.
blocking-b2g: - → ---
Flags: needinfo?(anygregor)
Noming this for 1.3 based on comment 22
blocking-b2g: --- → 1.3?
hi beatriz:
Can you accept the conclusion that this feature would not implemented on V1.1HD?
Thank you very much!
Flags: needinfo?(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)
Flags: needinfo?(brg)
Duplicate of this bug: 911616
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.
blocking-b2g: 1.3? → -
Target Milestone: 1.1 QE4 (15jul) → ---
This feature had been requested again and added in the certification requirements for the future. Can we add it in v1.4 ?
blocking-b2g: - → 1.4?
Blocks: 943796
Gregor, with the notifications API finished (from what I can tell), what is required to finish this?
Flags: needinfo?(anygregor)
(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.
How should we handle this need ?
Flags: needinfo?(annevk)
The Notifications API does not deal with the "notification center" functionality of a given platform. It's an API for sites.
Flags: needinfo?(annevk)
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.
Michael already outlined the steps here in comment 27. This has some security implications.
Paul, Ehsan, any objections to this plan?
Flags: needinfo?(ptheriault)
Flags: needinfo?(ehsan)
Flags: needinfo?(anygregor)
(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.
Flags: needinfo?(ehsan)
(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.
Depends on: 958667
(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
Flags: needinfo?(ptheriault)
(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.
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?
Flags: needinfo?(pdolanjski)
Stealing this, because I'm a bad guy :)
Assignee: mhenretty → lissyx+mozillians
Depends on: 967475
(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.
Flags: needinfo?(pdolanjski)
Assignee: lissyx+mozillians → reuben.bmo
Assignee: reuben.bmo → lissyx+mozillians
Depends on: 968955
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.
Flags: needinfo?(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?
Flags: needinfo?(brg)
(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.
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?
Flags: needinfo?(firefoxos-ux-bugzilla)
(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?
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(rmacdonald)
(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
(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
Flags: needinfo?(rmacdonald)
(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.
blocking-b2g: 1.4? → 1.4+
Target Milestone: --- → 1.4 S2 (28feb)
Unblocking per drivers decision - this feature isn't a QC requested feature, so this isn't allowed to be part of 1.4.
blocking-b2g: 1.4+ → backlog
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?
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
Whiteboard: [systemsfe] → [systemsfe] [p=8]
(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.
I got more context in the mean time.
(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.
Beatriz, the feature itself should be quite good now. If you want to try it again, feel free.
Flags: needinfo?(brg)
Target Milestone: 1.4 S2 (28feb) → ---
Whiteboard: [systemsfe] [p=8] → [systemsfe] [p=8][priority]
Whiteboard: [systemsfe] [p=8][priority] → [systemsfe], [p=8], [priority]
(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.
Flags: needinfo?(brg)
Blocks: 984305
blocking-b2g: backlog → 1.5?
(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 :)
No longer blocks: 930794
blocking-b2g: 2.0? → 2.0+
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.
blocking-b2g: 2.0+ → backlog
Fixed by landing the gecko and gaia patches in bug 967475.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S2 (23may)
Duplicate of this bug: 823851
Whiteboard: [systemsfe], [p=8], [priority] → [ucid: comms47, 2.0, FT:COMMS][systemsfe], [p=8], [priority]
Depends on: 1046828
Duplicate of this bug: 900353
Duplicate of this bug: 859214
blocking-b2g: backlog → ---
Depends on: 1172316
Depends on: 1221019
You need to log in before you can comment on or make changes to this bug.