1017821 - [B2G][System] Voicemail notification remains in the "Notification Menu" after removing SIM card from device.

Status: RESOLVED FIXED

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

Description

[Mike Livingston]

Attachment: voicemail notification

Description:
A device that has a SIM card that contains voicemails, and the "voicemail notification" appears in the "Notification Menu". When the device is turned off and the SIM card removed, power the device back on. In the "Notification Menu" the old "voicemail notification" remains even though there is no SIM card in the device.

Pre-Requisites: Make sure your device has a SIM card that contains at least one(1) voicemail.

Repro Steps:
1) Update a Flame to BuildID: 20140529040201
2) From the homescreen, pull down the "Notifications Menu"
3) Observe the voicemail notification. 
4) Take out the battery, remove the SIM card, replace battery and turn on device.
5) From the homescreen, pull down the "Notifications Menu"
6) Observe that the voicemail notification is still there, along with the "No SIM inserted" notification at the bottom of the "Notifications Menu"

Actual:
The voicemail notification appears in the "Notifications menu" when no SIM card is inserted in the device.

Expected:
No voicemail notification should appear in the "Notifications menu" when there is no SIM card inserted in the device.

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140529040201
Gaia: 4142df90c71abdc1e3a87cd37dff1a22d5e38b34
Gecko: 1e712b724d17
Version: 32.0a1
Firmware Version: V10G-2

Repro frequency: 100%
See attached: Screenshot

Notes: Unable to add logcat due to repro steps.

Comment 1

[Mike Livingston]

This issue reproduces on Buri 2.0. Voicemail notification remains when no SIM inserted.

2.0 Environmental Variables:
Device: Buri 2.0 MOZ
BuildID: 20140527040202
Gaia: 6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko: cbe4f69c2e9c
Version: 32.0a1
Firmware Version: v1.2-device.cfg
User Agent: Mozilla 5.0 Gecko 32.0 Firefox 32.0

-----------------------------------------------

This issue does not reproduce on Buri 1.4

1.4 Environmental Variables:
Device: Flame 1.4 MOZ
BuildID: 20140529000204
Gaia: 7bc1c15c67661a0b8e35f18f15a9d03d1d2cfcd5
Gecko: 2181cac4d0fc
Version: 30.0
Firmware Version: V10G-2
User Agent: Mozilla 5.0 Gecko 30.0 Firefox 30.0

Comment 2

[:gerard-majax]

That's probably because the notification was saved and relaunched on next reboot.

Comment 3

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

Yup, this was caused by 874364. Voicemail notifications were always resent on reboot using information obtained from the carrier. But now we actually store all notification data on the phone and resend it upon reboot. So even when there is no SIM to get voicemail info, we resend the stored notification object. I think we will need special logic here for preventing the resending of stored voicemail notifications on reboot. In other words, we want to send voicemail notifications using the SIM, not using the notification DB.

Comment 4

[:gerard-majax]

(In reply to Michael Henretty [:mhenretty] from comment #3)
> Yup, this was caused by 874364. Voicemail notifications were always resent
> on reboot using information obtained from the carrier. But now we actually
> store all notification data on the phone and resend it upon reboot. So even
> when there is no SIM to get voicemail info, we resend the stored
> notification object. I think we will need special logic here for preventing
> the resending of stored voicemail notifications on reboot. In other words,
> we want to send voicemail notifications using the SIM, not using the
> notification DB.

Or maybe we can check in voicemail.js if the SIM card changed/has been removed, then we should close the restored notifications.

Comment 5

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

(In reply to Alexandre LISSY :gerard-majax from comment #4)
> (In reply to Michael Henretty [:mhenretty] from comment #3)
> > Yup, this was caused by 874364. Voicemail notifications were always resent
> > on reboot using information obtained from the carrier. But now we actually
> > store all notification data on the phone and resend it upon reboot. So even
> > when there is no SIM to get voicemail info, we resend the stored
> > notification object. I think we will need special logic here for preventing
> > the resending of stored voicemail notifications on reboot. In other words,
> > we want to send voicemail notifications using the SIM, not using the
> > notification DB.
> 
> Or maybe we can check in voicemail.js if the SIM card changed/has been
> removed, then we should close the restored notifications.

Even better.

Comment 6

[:gerard-majax]

Hm. The current code does not have any link between the voicemail notifications and the SIMs other than the "serviceId" which is an int being 0 for the first SIM, 1 for the second ...

Tim or Alive, do you see any other way than having to use the SIM's ICCID in the voicemail notification tag ?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

No. I don't think there is any other way.

(Not the that I really familiar with this part of the code either ...)

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

I have same opinion, and this sounds a platform question.
Yoshi could you find someone to answer?

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Forward ni to Hsinyi.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #9)
> Forward ni to Hsinyi.

When the API was designed, it was designed to use the 'serviceId' to get the right mobileconnection object. Then acquire the corresponding iccId if there's any, i.e. mozMobileConnections[serviceId].iccId.

Comment 11

[:gerard-majax]

Hsin, I'm wondering about the behavior of the Voicemail API itself. What is the expected behavior, on Gecko side, when you have a voicemail and that your device boots ?

I remember reading code stating that we were not saving the voicemail bit when receiving the SMS that notifies us of the voicemail status. Is it still the case ? Do we perform some hand check when bringing up the RIL stack ? Does it depends on the provider who may resend us the voicemail sms again when we are getting back online?

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Hsin, I'm wondering about the behavior of the Voicemail API itself. What is
> the expected behavior, on Gecko side, when you have a voicemail and that
> your device boots ?
> 
> I remember reading code stating that we were not saving the voicemail bit
> when receiving the SMS that notifies us of the voicemail status. Is it still
> the case ? 

You are right. We don't save the voicemail. We check what we get from operator. Per my understanding, some operators don't update the voicemail status correctly even after user taps the voicemail indication, makes a call to voicemail number and there's indeed no new voicemail message. That's why we always see a voicemail indication.

> Do we perform some hand check when bringing up the RIL stack ?
> Does it depends on the provider who may resend us the voicemail sms again
> when we are getting back online?

Comment 13

[:gerard-majax]

Hsin, you did not answered the others questions, it's very blocking :(

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > Hsin, I'm wondering about the behavior of the Voicemail API itself. What is
> > the expected behavior, on Gecko side, when you have a voicemail and that
> > your device boots ?
> > 
> > I remember reading code stating that we were not saving the voicemail bit
> > when receiving the SMS that notifies us of the voicemail status. Is it still
> > the case ? 
> 
> You are right. We don't save the voicemail. We check what we get from
> operator. Per my understanding, some operators don't update the voicemail
> status correctly even after user taps the voicemail indication, makes a call
> to voicemail number and there's indeed no new voicemail message. That's why
> we always see a voicemail indication.
> 

I thought my previous (above) comment answered the below already. But if they weren't clear enough, then please see below:

> > Do we perform some hand check when bringing up the RIL stack ?

RIL updates the voicemail status based on the event from modem/carrier. If RIL doesn't receive anything, default value is exposed.

> > Does it depends on the provider who may resend us the voicemail sms again
> > when we are getting back online?

Yes, it depends on the provider. AFAIK some operators don't update voicemail status correctly. So, user always receives a notification even there's obviously no voicemail messages.

Did I answer all your questions this time?

Comment 15

[:gerard-majax]

Perfect :)

Comment 16

[:gerard-majax]

Attachment: prepopulated notificationstore.json with serviceId in tag

Comment 17

[:gerard-majax]

Attachment: prepopulated notificationstore.json with two valids iccId in tag

Comment 18

[:gerard-majax]

Attachment: prepopulated notificationstore.json with two iccId, one valid on in tag

Comment 19

[:gerard-majax]

attachment 8442796 [details], attachment 8442797 [details] and attachment 8442799 [details] are intended for testing with fake sim card from bug 989926

Comment 20

[:gerard-majax]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20757

This already works on B2G Desktop with fake libril and the previously attached notification store.

Previous unit tests have been updated to check for the use of iccId, new function to get SIM cards iccId is also tested.

Proper behavior with the notifications themselves is still to test.

Comment 21

[:gerard-majax]

PR updated, all tests should be good.

Comment 22

[:gerard-majax]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

Etienne, would you mind reviewing this?

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

Comments on github, probably worth a quick second review round.

Comment 24

[:gerard-majax]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

All nits addressed, tested on B2G Desktop with libril.js but so far I still haven't been able to get a SIM card with Voicemail support to make sure.

Meanwhile, I also removed most of the jshint failures except three that would pollute too much the patch.

Comment 25

[:gerard-majax]

It does not work on my Flame when I have no SIM at all, since there is no simslot-iccinfochange event triggered :(

Comment 26

[:gerard-majax]

SIMSlotManager.noSIMCardConnectedToNetwork() may help.

Comment 27

[:gerard-majax]

So I propose a change of plan:
 (1) upon start of Voicemail in System app, we retrieve all pending voicemail notifications
 (2) we store them
 (3) we close them
 (4) we wait for iccinfochange

Upon each iccinfochange, we can them cross check with the saved data from step (2) and see if we had any stored notification matching. If yes, then we can send a new one.

If the number of non null iccId we finally get is matching the number of SIM card, we can delete all remaining notifications.

Comment 28

[:gerard-majax]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> (In reply to Alexandre LISSY :gerard-majax from comment #11)
> > Hsin, I'm wondering about the behavior of the Voicemail API itself. What is
> > the expected behavior, on Gecko side, when you have a voicemail and that
> > your device boots ?
> > 
> > I remember reading code stating that we were not saving the voicemail bit
> > when receiving the SMS that notifies us of the voicemail status. Is it still
> > the case ? 
> 
> You are right. We don't save the voicemail. We check what we get from
> operator. Per my understanding, some operators don't update the voicemail
> status correctly even after user taps the voicemail indication, makes a call
> to voicemail number and there's indeed no new voicemail message. That's why
> we always see a voicemail indication.

I'm sorry, but I just had a look to the Gecko RIL, and I do see a lot of code handling with MWIS, reading and storing it. This has landed only a couple of weeks/months ago, so I would say it's now functionnal.

Specifically, I see bug 768441 part one [http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=9abce82c28b49b618378f5c8b98b587223e1ccad] which seems to precisely implement this, and remove the old code I had memory of.

On light of this, since we seem to be properly storing the information on the SIM card itself, I see no point in hacking around with notification, and we should just discard the saved ones, and wait for the statuschanged event to get triggered.

Comment 29

[:gerard-majax]

Using my hack from bug 989926, I added proper support for the MWIS bits, and I can confirm it's properly set/read:

> +ICC.prototype.files[ICC_EF_MWIS] = {};
> +ICC.prototype.files[ICC_EF_MWIS][ICC_COMMAND_GET_RESPONSE] = function(output, pathId, p1, p2, p3) {
> +  output.writeString("000000196fca040000000005020105");
> +};
> +ICC.prototype.files[ICC_EF_MWIS][ICC_COMMAND_READ_RECORD] = function(output, pathId, p1, p2, p3) {
> +  // One voicemail
> +  output.writeString("ff0100000000000000000000000000");
> +};

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Alexandre LISSY :gerard-majax from comment #29)
> Using my hack from bug 989926, I added proper support for the MWIS bits, and
> I can confirm it's properly set/read:
> 
> > +ICC.prototype.files[ICC_EF_MWIS] = {};
> > +ICC.prototype.files[ICC_EF_MWIS][ICC_COMMAND_GET_RESPONSE] = function(output, pathId, p1, p2, p3) {
> > +  output.writeString("000000196fca040000000005020105");
> > +};
> > +ICC.prototype.files[ICC_EF_MWIS][ICC_COMMAND_READ_RECORD] = function(output, pathId, p1, p2, p3) {
> > +  // One voicemail
> > +  output.writeString("ff0100000000000000000000000000");
> > +};

Sorry I didn't notice the change earlier.

Hi Bevis,
Could you help take a look at Alexandre's question? Thank you.

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Alexandre LISSY :gerard-majax from comment #28)
> I'm sorry, but I just had a look to the Gecko RIL, and I do see a lot of
> code handling with MWIS, reading and storing it. This has landed only a
> couple of weeks/months ago, so I would say it's now functionnal.
> 
> Specifically, I see bug 768441 part one
> [http://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=9abce82c28b49b618378f5c8b98b587223e1ccad] which seems to precisely
> implement this, and remove the old code I had memory of.
> 
> On light of this, since we seem to be properly storing the information on
> the SIM card itself, I see no point in hacking around with notification, and
> we should just discard the saved ones, and wait for the statuschanged event
> to get triggered.

Hi Alexandre,

Yes, in bug 768441, we've added the support to
1. Save received MWI into EF_MWIS if present in UICC.
2. Notify MWI to Gaia when reading EF_MWIS from UICC. (For example, when device boots up.)

Comment 32

[Anshul]

Trying to work around the problem where the notifications are saved for Voicemail and displayed when there is no SIM seems like a trap for future bugs. User really won't be able to make a call when the voicemail icon will be clicked. So as long as an appropriate message is displayed (like network not available), shouldn't that be good enough?

Comment 34

[:gerard-majax]

(In reply to Anshul from comment #32)
> Trying to work around the problem where the notifications are saved for
> Voicemail and displayed when there is no SIM seems like a trap for future
> bugs. User really won't be able to make a call when the voicemail icon will
> be clicked. So as long as an appropriate message is displayed (like network
> not available), shouldn't that be good enough?

Since Gecko now does properly store mwi and retriggers it, then there is no need for any workaround whatsoever: we should just close the previous notification on reboot. Then install the statuschanged handler on voicemail, which will get retriggered by MWIS.

FYI this is working fine on my B2G Desktop/Mulet build with hacked libriljs to emulate MWIS behavior. Previously sent notifications are closed at startup, and as soon as the SIM is available then we get notified and properly send new notifications.

I suspect you commented because of bug 1027870: what happens in this case is that when tapping the resent notification, a system message is sent. And we have no handling for those for voicemail notification. That's why nothing was happening.

Comment 35

[Anshul]

(In reply to Alexandre LISSY :gerard-majax from comment #34)
> (In reply to Anshul from comment #32)
> > Trying to work around the problem where the notifications are saved for
> > Voicemail and displayed when there is no SIM seems like a trap for future
> > bugs. User really won't be able to make a call when the voicemail icon will
> > be clicked. So as long as an appropriate message is displayed (like network
> > not available), shouldn't that be good enough?
> 
> Since Gecko now does properly store mwi and retriggers it, then there is no
> need for any workaround whatsoever: we should just close the previous
> notification on reboot. Then install the statuschanged handler on voicemail,
> which will get retriggered by MWIS.
> 
> FYI this is working fine on my B2G Desktop/Mulet build with hacked libriljs
> to emulate MWIS behavior. Previously sent notifications are closed at
> startup,
Is the code to close the notification for MWIS already merged on 2.0 or moz central?
> and as soon as the SIM is available then we get notified and
> properly send new notifications.
> 
> I suspect you commented because of bug 1027870: what happens in this case is
> that when tapping the resent notification, a system message is sent. And we
> have no handling for those for voicemail notification. That's why nothing
> was happening.
What do you mean you there is no handling for those for voicemail notifications? SIM had correctly already sent the MWI notification with message count and also a voicemailinfochanged with the MBDN (voicemail number) info.

Alexander, I am seeing the same behavior with email notifications where if you got an email notification and you rebooted the phone without clicking on the notification and after the reboot if you click on the notification nothing happens.

Comment 36

[:gerard-majax]

(In reply to Anshul from comment #35)

[...]

> Is the code to close the notification for MWIS already merged on 2.0 or moz
> central?

Not yet. I was going for the workaround at first, got informed that we were not yet okay regarding MWIS. But this week-end I checked gecko code, and thus yesterday I rewrote the patch. It's more or less ready. Needs more testing on device, though.

[...]

> What do you mean you there is no handling for those for voicemail
> notifications? SIM had correctly already sent the MWI notification with
> message count and also a voicemailinfochanged with the MBDN (voicemail
> number) info.

We have no system message handler. It has nothing to do at all with voicemail specifically.

> 
> Alexander, I am seeing the same behavior with email notifications where if
> you got an email notification and you rebooted the phone without clicking on
> the notification and after the reboot if you click on the notification
> nothing happens.

Well, that does work for me.

Comment 37

[Etienne Segonzac (:etienne)]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

Comments on github, I think we can simplify the patch a bit more thanks to your new approach.

Comment 38

[:gerard-majax]

(In reply to Etienne Segonzac (:etienne) from comment #37)
> Comment on attachment 8442838 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/20757
> 
> Comments on github, I think we can simplify the patch a bit more thanks to
> your new approach.

Thanks! I could not find any justification for keeping the iccId in the voicemail tag given the current assumptions. This makes the code simpler.

Comment 39

[:gerard-majax]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

Simplified as suggested.

Comment 40

[Etienne Segonzac (:etienne)]

Comment on attachment 8442838 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20757

all good!

Comment 41

[:gerard-majax]

Try is green: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=bd0a6f570688426b55a99ba4ef9de00808d04434

Comment 42

[:gerard-majax]

https://github.com/mozilla-b2g/gaia/commit/05174650872cc733462cd4fcfb04f70c0215bba7

Comment 43

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/ae8c3876b78de6f1a0b9899ce05826601a42d438