1022575 - Received SMS store (without text) on check balance.

Status: VERIFIED FIXED

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

Description

[Loli (:lolimartinezcr)]

Attachment: 2014-06-09-11-41-40.png

Tested
2.0
Hamachi
Reproducible 100%
Platform versión: 32.0a1
Build ID: 20140609083901
Git commit: deed49c5

Pre-requisites: Sim card with billing support.

STRs:
1. Tap usage application.
2. Tap "Update" button.
3. Tap message application.

Actual result:
You can see thread with messages about cost control application and when user taps in a message, it is empty (without text). See attached image.

Expected result:
Any messages about cost control is received.

Comment 1

[Loli (:lolimartinezcr)]

Attachment: 2014-06-09-11-42-01.png

Comment 2

[Marina Rodríguez [:mai]]

These threads disappear when the sms app is closed and reopened. I don't know how to force a refresh of the thread after a sms deletion.

Julien, can you help us with this?
Regards

Comment 3

[Julien Wajsberg [:julienw]]

I can think of several things:

* ignore the blacklisted SMS when loading the messages at all
* have a "ondeleted" event in [1] so that we can react on it

I'd do the first option, seems easier.

But first, Loli, I'd like to know if the issue happens on previous versions as well?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl

Comment 4

[Loli (:lolimartinezcr)]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> I can think of several things:
> 
> * ignore the blacklisted SMS when loading the messages at all
> * have a "ondeleted" event in [1] so that we can react on it
> 
> I'd do the first option, seems easier.
> 
> But first, Loli, I'd like to know if the issue happens on previous versions
> as well?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/
> nsIDOMMobileMessageManager.idl

Im not sure about what you want to know, because it is new feature.

Comment 5

[Loli (:lolimartinezcr)]

(In reply to Loli from comment #4)
> (In reply to Julien Wajsberg [:julienw] from comment #3)
> > I can think of several things:
> > 
> > * ignore the blacklisted SMS when loading the messages at all
> > * have a "ondeleted" event in [1] so that we can react on it
> > 
> > I'd do the first option, seems easier.
> > 
> > But first, Loli, I'd like to know if the issue happens on previous versions
> > as well?
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/
> > nsIDOMMobileMessageManager.idl
> 
> Im not sure about what you want to know, because it is new feature. This bug has been created when I tested Bug 987156 - [User Story] Cost Control: Do not store SMS sent or received on check balance and top-up

Comment 6

[Julien Wajsberg [:julienw]]

Hey Bevis,

see comment 3.

Is it possible to have a "deleted" event for v2.0?

All other solutions we discussed with Marina feels both complicated and hacky...

Comment 7

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

(In reply to Julien Wajsberg [:julienw] from comment #6)
> Hey Bevis,
> 
> see comment 3.
> 
> Is it possible to have a "deleted" event for v2.0?
> 
> All other solutions we discussed with Marina feels both complicated and
> hacky...

Hi Julien,

I'd like to double confirm if the received messages are the messages sent from SilentNumber. [1]
If yes, we didn't see any deleting-related operations from gecko because the message is actually created and notify directly to gaia without access to DB (save and delete).

Is there anything abnormal in SMS app instead?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#2955

Comment 8

[Julien Wajsberg [:julienw]]

Bevis, how a "silent message" is created ? I don't know much about this to be honest.

I don't think that's what's currently used, but maybe we should.

Comment 9

[Marina Rodríguez [:mai]]

Hi,
the sms are deleted by usage application. The problem is not related with the silent mode.

The str is the following:
 1. SMS app is opened in background
 2. Usage app deletes the sms
 3. Open the sms app. Then the threads of the deleted sms are showed with the last text received, because the sms app doesn't know that this sms are deleted. When the user click on the sms, the message is empty, because was deleted by usage app.

For this reason, if a sms-deleted event is launched, the sms app can detect when it's necessary a ui refresh to avoid the current behavior

If the sms app is closed after the deleting this behaviour does not occurs.

Comment 10

[Julien Wajsberg [:julienw]]

Marina: but maybe the SMS to update the balance should be sent using sendSilentSms instead of sendSms ? (note: I don't know the actual name of the method)

Comment 11

[Marina Rodríguez [:mai]]

AFAIK this method of the API is not available from gaia, only can used from gecko for security reasons.

Comment 12

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

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Bevis, how a "silent message" is created ? I don't know much about this to
> be honest.
> 
> I don't think that's what's currently used, but maybe we should.

After further study, as marina mentioned, this API can only be used from gecko in b2g/chrome/content/payment.js for security reasons.
Instead of defining new event in a rush, for the scenario in comment 9, 
is it possible to refresh the UI of the SMS app when it resumed from background to foreground?

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Marina: but maybe the SMS to update the balance should be sent using
> sendSilentSms instead of sendSms ? (note: I don't know the actual name of
> the method)

As Marina mentioned, the silent SMS flag is not exposed to web content. It is only used internally for payments [1] and the new MobileID API [2]. We had some discussions about this in the past and we figured that all the use cases could be fulfilled with the current WebSMS API and a collaboration with the Gaia SMS app.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=816564
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=988469

(In reply to marina rodríguez [:mai] from comment #9)

> Hi,
> the sms are deleted by usage application. The problem is not related with
> the silent mode.
> 
> The str is the following:
>  1. SMS app is opened in background
>  2. Usage app deletes the sms
>  3. Open the sms app. Then the threads of the deleted sms are showed with
> the last text received, because the sms app doesn't know that this sms are
> deleted. When the user click on the sms, the message is empty, because was
> deleted by usage app.
> 
> For this reason, if a sms-deleted event is launched, the sms app can detect
> when it's necessary a ui refresh to avoid the current behavior
> 
> If the sms app is closed after the deleting this behaviour does not occurs.

I am not saying that an event like this is not needed in the SMS API, but a way to workaround this is to notify from Cost Control to the SMS app via IAC about the SMS deletion.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #12)
> (In reply to Julien Wajsberg [:julienw] from comment #8)
> > Bevis, how a "silent message" is created ? I don't know much about this to
> > be honest.
> > 
> > I don't think that's what's currently used, but maybe we should.
> 
> After further study, as marina mentioned, this API can only be used from
> gecko in b2g/chrome/content/payment.js for security reasons.
> Instead of defining new event in a rush, for the scenario in comment 9, 
> is it possible to refresh the UI of the SMS app when it resumed from
> background to foreground?

I would prefer to avoid wasting any resources refreshing and repainting if nothing changed in the SMS DB. We need a way to tell the SMS app that something changed. We can do that from the API if we expect 3rd party consumers of the WebSMS API (something like the oncontactchange in the Contacts API) or from the known responsibles of these changes in the SMS DB, which if I am not wrong is only the Cost Control app so far.

Another alternative to the WebSMS SMS deletion event or the IAC workaround mentioned in comment 13 would be to expose a property with the version of the MobileMessage DB. With this property, the SMS app would be able to know if something changed since its last DB query and so it would be able to react accordingly. Note that this property would have the extra benefit of allowing the SMS app to handle local caches if needed, but that's another topic.

Comment 15

[Julien Wajsberg [:julienw]]

> Instead of defining new event in a rush, for the scenario in comment 9, 

Really, it's not that much, is it?

> is it possible to refresh the UI of the SMS app when it resumed from background to foreground?

That would mean fetching all messages and rendering again. Then no :)

> Another alternative would be to expose a property with the version of the MobileMessage DB. 

Not sure it's simpler :p
I don't think we should do this, especially in light of future Datastore work that will give this for free.


The easiest is to exactly know which message has been deleted so that we can update the ui. IAC would work (could be an argument to the "disable" operation) but looks overkill.

Comment 16

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

Filed bug 1022575 to address the possibility to provide a sms-deleted event for multiple apps to be aware of the delete operations in MobileMessageDB.

Comment 17

[bhavana bajaj [:bajaj]]

Can QA find out if this is a regression?

Comment 18

[Julien Wajsberg [:julienw]]

Unneeded, this is bug 1007588.

Comment 19

[Andreas Gal :gal]

Comment 18 says this is a dup of 1007588. Why was this set to blocking without dup-ing it. Bhavana?

Comment 20

[David Scravaglieri [:scravag]]

This is a duplicate so marking

Comment 21

[Marcelino Veiga Tuimil [:sonmarce]]

This is not a duplicate, we need it to fix threading problem described in comment 0. It is owed to the case of Cost Control app deleting an SMS while SMS app is running. This bug would be to fix gaia part of SMS app, and gecko part would be implemented in bug 1023695, creating an event to be handled by SMS app.

Comment 22

[Maria Angeles Oteo (:oteo)]

Changing the component to SMS application

Comment 23

[Julien Wajsberg [:julienw]]

bug 1007588 was the regressing bug, not a duplicate; I answered the previous request from Jason and marked it accordingly in the "blocks" line. Sorry for the confusion.

The solution should be quite easy once we have the Gecko event.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Andreas Gal :gal from comment #19)
> Comment 18 says this is a dup of 1007588. Why was this set to blocking
> without dup-ing it. Bhavana?

It was indeed discussed that 1007588 was the blocker bug and not a DUP, which Julien clarifies per comment #23.

Comment 25

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

ETA of the Gecko dependency (bug 1023695) will be 7/11. 
It's under review so please wait a few days.

Comment 26

[Julien Wajsberg [:julienw]]

Some of the work done in Bug 1024835 will be used here.

Comment 27

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Hi Marina,

Could you please let us know what SMS exactly should be received so that Cost control\Usage app can recognize it correctly?

Also any other pointers on how to correctly configure SIM card to work with Cost control\Usage app would be appreciated :)

Thanks!

Comment 28

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Hi Vicamo and Marina,

Once we fix bug described in the comment 0 using "navigator.mozMobileMessage.ondeleted" we'll still have the following issue: 

If cost control incoming SMS (eg. with balance info) is received when SMS inbox is opened, user will notice that new thread appears and then quickly disappears (when cost control app deletes it). Probably it's not very frequent case, but still it doesn't feel like good UX. It depends on how long it can take for Cost Control app to process and delete that SMS + how long it can take for SMS app to create and then delete new thread.

When we have messages datastore probably other apps that are going to use it will also suffer from such thing. Also what will happen if Cost Control\SMS app is crashed\killed in the middle of it. 

So, we probably can think of different workarounds on SMS side, but I'd like to know whether Gecko\System can take care of it in a cleaner and more scalable way (ideally SMS app shouldn't know about such service SMS at all).

What do you guys think?

Thanks!

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Hi,

We have silent SMS for payment, but that's not available for apps other than System app. They are delivered to neither SMS database nor mozMobileMessage::onreceived event target but the payment method itself. If we agree to ask System app to send that SMS for you with the silent flag set, I think that would fit the need perfectly. However, this follows some complicated communication between Cost Control and System app.

Comment 30

[Julien Wajsberg [:julienw]]

Hey Vicamo,

here is the requirements from the Cost Control side, as I understood it:

* needs to send a message silently
* needs to receive a message silently
* that message can come from different numbers (not necessarily the same one that the initial destination)
* the said numbers can also send messages that we don't want to silence
* CostControl parses the input message and then delete it and disables the silencing only if it was a valid answer

That's why the current solution uses a quite complex solution involving IAC. I don't really like it so much, but it sounded like the simplest solution to fulfill these requirements. I don't know if the current Silent SMS feature works for this?

Comment 31

[Marina Rodríguez [:mai]]

Hi, 
the process to update the balance depends on the minimun_delay variable of the config. Currently all the countries have 3 hours as value.

When the user interacts with the widget or directly with the App, the app checks when the last balance request was made, if the mínimun_delay passed then the app proceed to send the message.

For this reason, IMHO the usual case is that the user is not working with sms when these messages are been processed, although no way to ensure this.

Regards

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #30)
> Hey Vicamo,
> 
> here is the requirements from the Cost Control side, as I understood it:
> 
> * needs to send a message silently
> * needs to receive a message silently
> * that message can come from different numbers (not necessarily the same one
> that the initial destination)
> * the said numbers can also send messages that we don't want to silence

There are three related APIs for silent SMS.  {add,remove}SilentNumber to add/remove interested silent SMS.  Received messages with its sender attribute set to one of the registered silent message numbers are only broadcasted inside Gecko chrome process via "sms-received" observer event. For outgoing messages, use that aSilent argument of nsISmsService::Send(). [1]  I think this meets all above conditions unless we have an additional one:

  * messages from a certain address may be silent or non-silent in a certain period of time. Totally unpredictable.

> * CostControl parses the input message and then delete it and disables the
> silencing only if it was a valid answer

As said, a silent message will never be stored into database. So even that's an unexpected answer, the message just can't be put back to database at present.

> That's why the current solution uses a quite complex solution involving IAC.
> I don't really like it so much, but it sounded like the simplest solution to
> fulfill these requirements. I don't know if the current Silent SMS feature
> works for this?

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsISmsService.idl#24

Comment 33

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: GitHub pull request URL (v2.0)

Comment 34

[Julien Wajsberg [:julienw]]

Ok thanks Vicamo.

For v2.0, unless we have partner blockers, I think it's safer and good enough to keep the current way. For later versions we can revisit if we have requests to do it from UX. I personally think it's actually not that bad to see the threads appearing/disappearing for more transparency to the user.

Comment 35

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Attachment: GitHub pull request URL (master)

Comment 36

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

Hey Julien,

Here is a small patch (v2.0 only) that fixes issue in comment 0, other refactoring and improvements will be in master patch only.

Thanks

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

I think we should land this patch in both master and v2.0 and file a separate bug for the more invasive changes we want to do in master.

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

I keep my r for another round for unit tests only. The code is fine.

Thanks !

Comment 39

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

(In reply to Julien Wajsberg [:julienw] from comment #38)
> Comment on attachment 8456311 [details] [review]
> GitHub pull request URL (v2.0)
> 
> I keep my r for another round for unit tests only. The code is fine.
> 
> Thanks !
Thanks for review! Updated unit test with your suggestion.

> I think we should land this patch in both master and v2.0 and file a
> separate bug for the more invasive changes we want to do in master.
Yep, moreover I'll probably have more than one follow-up bug for master...

Comment 40

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Just for the record, here are steps that allow to simulate "SIM card with billing support" provided by Marina:

1. Make "getConfigFilePath" function to always return "'js/config/vivo/config.js'" in [1];
2. In [2] add your mobile number to the array of "senders" like "['4850', '[phone]']";
3. (Optional) In [3] set the same mobile number as "destination" like "[phone]";
3. Open Cost Control (Usage) app and configure SIM as "Prepaid";
4. At the balance screen tap on "Update" (now Cost Control sends a message to the number defined in the [3], this message is removed by Cost Control once it's sent);
5. From the number defined in [2] send the following message "2014071108523301110010215.67;15/06/2014" where "15.67" part is actual balance (once it's parsed by Cost Control app and determined as valid it will be automatically removed).

[1] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/config_manager.js#L104
[2] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/vivo/config.js#L108
[3] https://github.com/mozilla-b2g/gaia/blob/1246714f168d23d624244e352f8ba5d151a6da9a/apps/costcontrol/js/config/vivo/config.js#L104

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 8456311 [details] [review]
GitHub pull request URL (v2.0)

r=me

Use insertMockMarkup to insert the mock DOM if you think it's better, otherwise you can leave it like this.

I haven't tried to exercise the whole code with CostControl but I did try to delete threads and at least this doesn't regress :) I trust that you tested the case with CostControl ;)

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 8456686 [details] [review]
GitHub pull request URL (master)

as discussed, this bigger patch won't land here.

Comment 43

[Oleg Zasypkin [:azasypkin][⏰UTC+1]]

Master: https://github.com/mozilla-b2g/gaia/commit/6f335ef8865e597d83d47621261e4ba844d8bbdf

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Loli (:lolimartinezcr)]

Tested and working
Hamachi
2.0
Gecko-15afb7d
Gaia-c7b190a

Hamachi
2.1
Gecko-c431020
Gaia-1c9eb3d