Closed Bug 733266 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: philikon, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 5 obsolete files)

pass MSISDN to SMS DB ,v5
4.34 KB, patch
philikon
: review+
Details | Diff | Splinter Review 
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.
Summary: B2G SMS: Use MSISDN from RIL as own phone number → B2G SMS DB: Use MSISDN from RIL as own phone number
Assignee: nobody → yhuang
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
(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.
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~ :)
Attached patch pass MSISDN to SMS DB (obsolete) — — Splinter Review 
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.
Attachment #605314 - Flags: review?(philipp)
(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 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!
Attachment #605314 - Flags: review?(philipp) → review+
sure, I'll do it
thanks
(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
(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.
Attached patch pass MSISDN to SMS DB v2 (obsolete) — — Splinter Review 
revised according to philikon's comments,
MSISDN remains null if cannot get it.
Attachment #605314 - Attachment is obsolete: true
Attachment #605663 - Flags: review?(philipp)
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;
Attachment #605663 - Flags: review?(philipp) → review-
(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
Attached patch [WIP] pass MSISDN to SMS DB v3 (obsolete) — — Splinter Review 
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
Attachment #605663 - Attachment is obsolete: true
(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 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.
Attachment #606142 - Flags: review+
(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.
(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 :)
(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
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?
Attached patch [WIP] pass MSISDN to SMS DB v4 (obsolete) — — Splinter Review 
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; )
Attachment #606142 - Attachment is obsolete: true
(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.
Accommodate to philikon's comments.
Thanks for showing this cool lazy getter pattern to me, philikon. :)
Attachment #606510 - Attachment is obsolete: true
Attachment #606868 - Flags: review?(philipp)
Attachment #606868 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/cec9443fa8eb
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attached patch Patch (obsolete) — — Splinter Review 
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.
Attachment #636062 - Flags: review?(philipp)
(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 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.)
Attachment #636062 - Attachment is obsolete: true
Attachment #636062 - Flags: review?(philipp)
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
(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?
(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.
(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?
(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 ;))
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
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: