733266 - B2G SMS DB: Use MSISDN from RIL as own phone number

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

Bug 720638 will get us the MSISDN from the RIL. If it's available we should use it to add sender/receiver to stored messages.

Comment 1

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

Hi.philikon :
  I plan to use sendDOMMessage to send MSISDN back to RadioInterfaceLayer.js, but what I am not sure is what the new message.type should be named, for example, 'siminfoavailable'?? Or should I use the original 'cardstatechange'? And should we also send other information like IMEI, IMSI back to RadioInterfaceLayer?    
  If you can kindly give me some suggestions that will be appreciated.

Thanks

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #1)
> I plan to use sendDOMMessage to send MSISDN back to
> RadioInterfaceLayer.js, but what I am not sure is what the new message.type
> should be named, for example, 'siminfoavailable'??

If we include IMEI, IMSI, etc. like you suggested above, I suggest something like 'deviceinfo'. Because the IMEI is the device identity, whereas IMSI, phone number, etc. are SIM card specific, and 'deviceinfo' sounds vague enough to me that it can include both ;).

> Or should I use the original 'cardstatechange'?

We can't really use the 'cardstatechange' message because the response from 'getMSISDN' will be available asynchronously.

> And should we also send other information like
> IMEI, IMSI back to RadioInterfaceLayer?    

I suppose we could do that. Right now we don't expose it anywhere to content, but we always could (e.g. as read-only settings in the Settings API?).

You're going to have figure out to wait for all these calls to return before sending the 'deviceinfo' message. Should be easy enough, though.

Comment 3

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

Hi Philikon
Thanks for your suggestions. but I still got some questions.

1. After we have used the phone for a while, if we pull out the simcard, 
   I think two result can happen.
   case a: 
     we notify the user should reboot the phone, and block/ignore all the ril requests after.
     (but this is not related to this bug, so I don't discuss it here)
   case b:
     I suppose 'cardstatechange' will be called with SIM_ABSENT,
     in that case, 
     should we also notify the deviceinfo event? Since at lease MSISDN won't be available now.
     If this answer is 'yes', 
       should we still need to send 'deviceinfo' event with all infos like you suggest in the last part in your last comment (wait for all these calls to return before sending the 'deviceinfo' message)? 
    Since your comment suggests that if the deviceinfo event arrives, all info should be available. But in this case(a), it might not.I am wondering will the scenario be inconsistent in some cases. 
   
2. If we put IMEI, IMEISV in 'deviceinfo' event, but in current implementation, these infos will only be available when simcard is READY, but I think these info should also be available even when simcard is ABSENT or NOTREADY, is that right? 
  Or should we separate them out with deviceinfo and siminfo event?

sorry for my long questions....
I just want to make clean about the definition about those events.
thank you very much~ :)

Comment 4

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

Attachment: pass MSISDN to SMS DB

I think I can just add the MSISDN into message object, 
so we don't have to add another message type, 
sorry my last comment should be useless.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #4)
> I think I can just add the MSISDN into message object, 
> so we don't have to add another message type, 
> sorry my last comment should be useless.

Sorry I took so long to respond to comment 3. I think your patch is a good idea!

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 605314 [details] [diff] [review]
pass MSISDN to SMS DB

The only question I have is this: If the SIM card doesn't have an MSISDN, the ICC_EF_MSISDN file just contains 'fff...ff', right? So after we read this value and evaluate it as swapped nibble BCD, what will Phone.MSISDN be? I *think* it will be the string "0". Can you verify that? I think in that case we should continue to use `null` everywhere instead of the string "0".

Basically, replace

  let number = GsmPDUHelper.readStringAsBCD().toString().substr(4);
  if (DEBUG) debug("MSISDN: " + number);
  this.MSISDN = number;

with something like

  let number = GsmPDUHelper.readStringAsBCD();
  if (!number) {
    if (DEBUG) debug("ICC does not have MSISDN information.");
    return;
  }
  this.MSISDN = number.toString().substr(4);
  if (DEBUG) debug("MSISDN: " + this.MSISDN);

Would be great if you could test this and -- if it works -- incorporate it into the patch. r=me with that. Thanks!

Comment 7

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

sure, I'll do it
thanks

Comment 8

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

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 605314 [details] [diff] [review]
> pass MSISDN to SMS DB
> 
> The only question I have is this: If the SIM card doesn't have an MSISDN,
> the ICC_EF_MSISDN file just contains 'fff...ff', right? So after we read
> this value and evaluate it as swapped nibble BCD, what will Phone.MSISDN be?
> I *think* it will be the string "0".
Hi Philikon, it will be "". The reason is the result of substr(4).
So should I still need to change it to null?

thanks

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #8)
> Hi Philikon, it will be "". The reason is the result of substr(4).

Oh right, that makes sense.

> So should I still need to change it to null?

Yes, I think that would be clearer.

Comment 10

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

Attachment: pass MSISDN to SMS DB v2

revised according to philikon's comments,
MSISDN remains null if cannot get it.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 605663 [details] [diff] [review]
pass MSISDN to SMS DB v2

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +309,5 @@
>    /**
>     * nsISmsDatabaseService API
>     */
>  
> +  saveReceivedMessage: function saveReceivedMessage(sender, receiver, body, date) {

I just realized, this won't work! These methods are part of the nsISmsDatabaseService contract, so you can't simply add a parameter here! (In fact, I'm surprised this works... did you test this at all?)

So I think we need to get back to our original plan and notify RadioInterfaceLayer of the MSISDN once we retrieve it from the ICC. We can expose it through the 'radioState' object. SmsDatabaseService would have to acquire the nsIRadioInterfaceLayer object, e.g. via this lazy getter:

  get mRIL() {
    delete this.mRIL;
    return this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
                         .getService(Ci.nsIInterfaceRequestor)
                         .getInterface(Ci.nsIRadioInterfaceLayer);
  },

and then use this.mRIL.radioState.MSISDN.

::: dom/system/b2g/ril_worker.js
@@ +1816,5 @@
>  
>        case ICC_COMMAND_READ_RECORD:
>          // Ignore 2 bytes prefix, which is 4 chars
>          let number = GsmPDUHelper.readStringAsBCD().toString().substr(4); 
> +        this.MSISDN = number ? number : null;

Simpler:

  this.MSISDN = number || null;

Comment 12

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

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> Comment on attachment 605663 [details] [diff] [review]
> pass MSISDN to SMS DB v2
> (In fact, I'm surprised this works... did you test this at all?)
Oops, sorry, I didn't test that far. 
What I test is to make sure MSISDN can be passed to SMSDatabase when sending/receiving sms, 
I didn't test the functionalities of SMSDatabase. Next time I'll check SMSDatabase as well.

thanks

Comment 13

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

Attachment: [WIP] pass MSISDN to SMS DB v3

revised according philikon's comments.
But SMSDatabase will throw exception in latest m-c build(Not related to my patch).

03-15 16:19:22.175: E/GeckoConsole(2610): [JavaScript Error: "uncaught exception: [Exception... "This error occurred because an operation was not allowed on an object. A retry of the same operation would fail unless the cause of the error is corrected."  code: "2" nsresult: "0x80660002 (NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR)"  location: "jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js Line: 180"]"]

file new Bug 736009 for this

Comment 14

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

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> Comment on attachment 605663 [details] [diff] [review]
> pass MSISDN to SMS DB v2

>   get mRIL() {
>     delete this.mRIL;
>     return this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
>                          .getService(Ci.nsIInterfaceRequestor)
>                          .getInterface(Ci.nsIRadioInterfaceLayer);
>   },


Hi, philikon 
Can you kindly explain these code a little bit? 
I know this sample code comes from RILNetworkInterface, and it works. But I just don't know how.
Why 'delete' at first?
Then how does it assign value to this.mRIL? I though mRIL is readonly.

Thanks.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 606142 [details] [diff] [review]
[WIP] pass MSISDN to SMS DB v3

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

This looks good except for the lazy getter, see below. Please rebase this on top of bug 725002 (sorry for the inconvenience!).

r=me with that addressed.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +105,5 @@
> +  // Helper
> +  get mRIL() {
> +    return this._mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> +                        .getService(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIRadioInterfaceLayer);

You're not overriding the getter with the value (because you're setting this._mRIL, not this.mRIL), so this getter is not lazy. It will make the service lookup every single time.

Also, in ES5 strict mode you need to delete the getter first before you can override it with a value. This file is in strict mode, so you will have to do this.

See comment 11 for what it should look like.

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #14)
> >   get mRIL() {
> >     delete this.mRIL;
> >     return this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> >                          .getService(Ci.nsIInterfaceRequestor)
> >                          .getInterface(Ci.nsIRadioInterfaceLayer);
> >   },
> 
> Can you kindly explain these code a little bit? 

Sure! This is a lazy getter. The first time you access this.mRIL, the code gets run. Then the getter replaces itself with the returned value. The 'delete' is needed in ES5 strict mode. Otherwise you would get an exception for trying to overwrite a read-only attribute. The 'delete' will remove the getter, that way we can set the attribute to the returned value. It's a common pattern throughout Gecko.

Comment 17

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

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> Comment on attachment 606142 [details] [diff] [review]
> [WIP] pass MSISDN to SMS DB v3
> 
> Review of attachment 606142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good except for the lazy getter, see below. Please rebase this on
> top of bug 725002 (sorry for the inconvenience!).
> 
> r=me with that addressed.
> 
> ::: dom/sms/src/ril/SmsDatabaseService.js
> @@ +105,5 @@
> > +  // Helper
> > +  get mRIL() {
> > +    return this._mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> > +                        .getService(Ci.nsIInterfaceRequestor)
> > +                        .getInterface(Ci.nsIRadioInterfaceLayer);
> 
> You're not overriding the getter with the value (because you're setting
> this._mRIL, not this.mRIL), so this getter is not lazy. It will make the
> service lookup every single time.
> See comment 11 for what it should look like.

Hi, philikon 
Yes, I use the original you paste in comment 11 
I put the code in SmsDatabaseService.prototype.
But when I test it, I found it will throw exception, sorry I forgot the error message, 
But it's something like 'setting on a property(mRIL) which doens't have a setter.'
That's why I wrote the code in my patch.
I'll check more detailed about this.

Thanks for your comment and explanation :)

Comment 18

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

(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> (In reply to Yoshi Huang[:yoshi] from comment #14)
> > >   get mRIL() {
> > >     delete this.mRIL;
> > >     return this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> > >                          .getService(Ci.nsIInterfaceRequestor)
> > >                          .getInterface(Ci.nsIRadioInterfaceLayer);
> > >   },
> > 
> > Can you kindly explain these code a little bit? 
> 
> Sure! This is a lazy getter. The first time you access this.mRIL, the code
> gets run. Then the getter replaces itself with the returned value. The
> 'delete' is needed in ES5 strict mode. Otherwise you would get an exception
> for trying to overwrite a read-only attribute. The 'delete' will remove the
> getter, that way we can set the attribute to the returned value. It's a
> common pattern throughout Gecko.

Hi philikon, 
But now in SmsDatabaseService, it uses phototype and constructor to define its members,
not using object initializers.
But I check the delete operator on MDN [1]
"You cannot delete a property on an object that it inherits from a prototype "
so in phototype, that means the line 'delete this.mRIL' doesn't work, right?
also I don't know how to do ' delete the property of prototype inside prototype"...
delete this.prototype.mRIL seems cannot work.

Also I got another question about this pattern
why not just use 

get mRIL() {
  return Cc["@mozilla.org/telephony/system-worker-manager;1"]
         .getService(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIRadioInterfaceLayer);
},

??

Thanks

[1] https://developer.mozilla.org/en/JavaScript/Reference/Operators/delete

Comment 19

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

Hi philikon:
I understand the pattern now. :) Thanks for timdream's explanation.
So the current problem is that we did this in prototype object, 
and that member in prototype cannot be 'deleted'.
In that case, if I write 

get mRIL() {
  delete SmsDatabaseService.prototype.mRIL;
  return this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
                       .getService(Ci.nsIInterfaceRequestor)
                       .getInterface(Ci.nsIRadioInterfaceLayer);
},


Does this code make sense now?

Comment 20

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

Attachment: [WIP] pass MSISDN to SMS DB v4

Revised lazy getter according to philikon's comments.
I'll make it a formal review request if philikon thinks my comment is okay.
(delete SmsDatabaseService.prototype.mRIL; )

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #19)
> I understand the pattern now. :) Thanks for timdream's explanation.
> So the current problem is that we did this in prototype object, 
> and that member in prototype cannot be 'deleted'.

Oh, you're totally right! Good catch. It's fine in RILNetworkInterface because that's not a prototype.

Your solution makes sense, but I'm actually thinking we should probably just use XPCOMUtils.defineLazyGetter:

  XPCOMUtils.defineLazyGetter(SmsDatabaseService.prototype, "mRIL", function () {
    return Cc["@mozilla.org/telephony/system-worker-manager;1"]
             .getService(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIRadioInterfaceLayer);
  });

That is 100% equivalent to your solution, but uses a common module to do it.

Comment 22

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

Attachment: pass MSISDN to SMS DB ,v5

Accommodate to philikon's comments.
Thanks for showing this cool lazy getter pattern to me, philikon. :)

Comment 23

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/mozilla-central/rev/cec9443fa8eb

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Since this patch I can't receive SMS anymore on my Otoro device. The attached patch use MSISDN from RIL only if it is present.

Comment 25

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

(In reply to Vivien Nicolas (:vingtetun) from comment #24)
> Created attachment 636062 [details] [diff] [review]
> Patch
Hi Vivien,
Thanks for the patch, 
Also FYI, the root cause is from Bug 766862.
thanks

Comment 26

[Philipp von Weitershausen [:philikon]]

Comment on attachment 636062 [details] [diff] [review]
Patch

This bug has long been fixed. Please file a new bug. (Also, we should look at whatever the underlying problem is rather than plastering over the bug. E.g. the bug that Yoshi mentioned.)

Comment 27

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

Marshall told me he also found this situation on emulator, so I am trying to build emulator right now to see what happens, will file a bug if needed.

thanks

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Philipp von Weitershausen [:philikon] from comment #26)
> Comment on attachment 636062 [details] [diff] [review]
> Patch
> 
> This bug has long been fixed. Please file a new bug. (Also, we should look
> at whatever the underlying problem is rather than plastering over the bug.
> E.g. the bug that Yoshi mentioned.)

Nobody with the Otoro device can use SMS right now. Maybe we should backout this patch instead?

Comment 29

[Philipp von Weitershausen [:philikon]]

(In reply to Vivien Nicolas (:vingtetun) from comment #28)
> Nobody with the Otoro device can use SMS right now.

IIRC it worked for me at some point, and this patch landed 3 months ago. How can *this* patch be a regression?

> Maybe we should backout this patch instead?

This patch can't be the root cause. We likely have a bug in ril_worker or need a RILQUIRK or something like that. Please file a new bug.

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Philipp von Weitershausen [:philikon] from comment #29)
> (In reply to Vivien Nicolas (:vingtetun) from comment #28)
> > Nobody with the Otoro device can use SMS right now.
> 
> IIRC it worked for me at some point, and this patch landed 3 months ago. How
> can *this* patch be a regression?

I have done a mistake on the bug number. I think I arrived to this bug because of some wrong informations on the log of SmsDatabaseService.js (the last change is 'Bug 767620 - Part 2: RIL implementation. r=philikon' from the 13 june but the bug number is wrong :/).

> 
> > Maybe we should backout this patch instead?
> 
> This patch can't be the root cause. We likely have a bug in ril_worker or
> need a RILQUIRK or something like that. Please file a new bug.

So my suggestion was to backout the patch that cause the regression (but I don't know the bug number...). Maybe you do?

Comment 31

[Philipp von Weitershausen [:philikon]]

(In reply to Vivien Nicolas (:vingtetun) from comment #30)
> (In reply to Philipp von Weitershausen [:philikon] from comment #29)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #28)
> > > Nobody with the Otoro device can use SMS right now.
> > 
> > IIRC it worked for me at some point, and this patch landed 3 months ago. How
> > can *this* patch be a regression?
> 
> I have done a mistake on the bug number. I think I arrived to this bug
> because of some wrong informations on the log of SmsDatabaseService.js (the
> last change is 'Bug 767620 - Part 2: RIL implementation. r=philikon' from
> the 13 june but the bug number is wrong :/).

Sigh, it was pushed with the wrong bug number, then backed out, and then repushed again with the wrong number.

The actual bug number is bug 762760. Let's move over there. (But really, I'd prefer a fix rather than a backout. Feel free to bully Yoshi onsite ;))

Comment 32

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

Hi Philikon
The actual root cause should be Bug766862

I agree to fix, but in case Vivien needs quick fix to demo
I'll file a bug first

Comment 33

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

(In reply to Philipp von Weitershausen [:philikon] from comment #31)
 > Sigh, it was pushed with the wrong bug number, then backed out, and then
> repushed again with the wrong number.
> 
> The actual bug number is bug 762760. Let's move over there.
Sorry when I re-pushed, I forgot to replace the patch in Bugzilla
But the re-pushed patch landed in m-c is correct,
https://hg.mozilla.org/mozilla-central/rev/d509d8abf5ba

as I said in last comment, the root cause is Bug 766862
don't need to back out patch for Bug762760 again.

sorry for causing confusion again.

Comment 34

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

File a Bug 768367