CDMA MO MMS still not working as phone number is not correctly retrieved from iccInfo

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nsarkar, Assigned: bevis)

Tracking

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

MO MMS is not working on verizon network on 1.4 as phone number sent in the payload is not being obtained correctly from the iccInfo. We need to QueryInterface for the correct Gsm/Cdma icc info to get the phone number correctly.
Attachment #8439540 - Flags: feedback?(btseng)
blocking-b2g: --- → 1.4?
Blocks: 984663
Might need to modify the patch for sending null if phone number is not present.
Didn't cover that part. I don't know if sending headers["from"] = {address: null, type: "PLMN"} will work in that case. Probably not.
Comment on attachment 8439540 [details] [diff] [review]
Proposed patch to fix this issue. Tested and this works.

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

Thanks for catching this bug because this is not reproducible when RadioInterfaceLayer is implemented in JS code, but
we should always QI the object defined in the .idl before using it.

Please see my comment inline for your patch.
There is still some problem to be addressed. :)

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +318,5 @@
> +      let baseIccInfo = this.radioInterface.rilContext.iccInfo;
> +      if (baseIccInfo.iccType === 'ruim' || baseIccInfo.iccType === 'csim') {
> +        iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozCdmaIccInfo);
> +      } else if (baseIccInfo.iccType === 'sim' ||
> +          baseIccInfo.iccType === 'usim') {

'else {' shall be enough to try to convert the baseIccInfo to GsmIccInfo.

@@ +329,3 @@
>  
>      if (!iccInfo) {
>        return null;

We could remove this line:
"
let number = (iccInfo instanceof Ci.nsIDOMMozGsmIccInfo)
               ? iccInfo.msisdn : iccInfo.mdn;
"
And get the number right after QI nsIDOMMozCdmaIccInfo/nsIDOMMozGsmIccInfo

@@ +342,3 @@
>      return number;
>    },
>  

Hence, the getPhoneNumber() can be rewritten in this way:

  getPhoneNumber: function() {
    let number;
    // Get the proper IccInfo based on the current card type.
    try {
      let iccInfo = null;
      let baseIccInfo = this.radioInterface.rilContext.iccInfo;
      if (baseIccInfo.iccType === 'ruim' || baseIccInfo.iccType === 'csim') {
        iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozCdmaIccInfo);
        number = iccInfo.mdn;
      } else {
        iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozGsmIccInfo);
        number = iccInfo.msisdn;
      }
    } catch (e) {
      if (DEBUG) {
       debug("Exception - QueryInterface failed on iccinfo for GSM/CDMA info");
      }
      return null;
    }

    // Workaround an xpconnect issue with undefined string objects.
    // See bug 808220
    if (number === undefined || number === "undefined") {
      return null;
    }

    return number;
  },

@@ +1099,5 @@
>    // Insert Phone number if available.
>    // Otherwise, Let MMS Proxy Relay insert from address automatically for us.
>    let phoneNumber = mmsConnection.getPhoneNumber();
> +  let from = {address: phoneNumber, type: "PLMN"};
> +  msg.headers["from"] = from;

We should keep this unchanged because msg.header["from"] = null is meaningful to MmsPduHelper to encode |From| with |Insert-address-token| when |phoneNumber| is invalid.

See http://hg.mozilla.org/mozilla-central/file/aab3362f97e9/dom/mobilemessage/src/gonk/MmsPduHelper.jsm#l889

@@ +1394,5 @@
>    // Insert Phone number if available.
>    // Otherwise, Let MMS Proxy Relay insert from address automatically for us.
>    let phoneNumber = mmsConnection.getPhoneNumber();
> +  let from = {address: phoneNumber, type: "PLMN"};
> +  headers["from"] = from;

ditto.
Attachment #8439540 - Flags: feedback?(btseng)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Hi Bevis,

I agree with your changes for the else and removing the block -

>  
>      if (!iccInfo) {
>        return null;

Please note my patch was a suggestion of how to fix this.

However, the lines -

@@ +1099,5 @@
>    // Insert Phone number if available.
>    // Otherwise, Let MMS Proxy Relay insert from address automatically for us.
>    let phoneNumber = mmsConnection.getPhoneNumber();
> +  let from = {address: phoneNumber, type: "PLMN"};
> +  msg.headers["from"] = from;

if you do this directly - 
> +  msg.headers["from"] = {address: phoneNumber, type: "PLMN"} : null;
This doesn't work. I have tested this. Please note that the same is done for msg.headers["to"].

So the thing I suggested is - 

> +  let from = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} : null;
> +  msg.headers["from"] = from;

I will send you an updated patch with your comments addressed.

Thanks,
Nivi.


(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #2)
> Comment on attachment 8439540 [details] [diff] [review]
> Proposed patch to fix this issue. Tested and this works.
> 
> Review of attachment 8439540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for catching this bug because this is not reproducible when
> RadioInterfaceLayer is implemented in JS code, but
> we should always QI the object defined in the .idl before using it.
> 
> Please see my comment inline for your patch.
> There is still some problem to be addressed. :)
> 
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +318,5 @@
> > +      let baseIccInfo = this.radioInterface.rilContext.iccInfo;
> > +      if (baseIccInfo.iccType === 'ruim' || baseIccInfo.iccType === 'csim') {
> > +        iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozCdmaIccInfo);
> > +      } else if (baseIccInfo.iccType === 'sim' ||
> > +          baseIccInfo.iccType === 'usim') {
> 
> 'else {' shall be enough to try to convert the baseIccInfo to GsmIccInfo.
> 
> @@ +329,3 @@
> >  
> >      if (!iccInfo) {
> >        return null;
> 
> We could remove this line:
> "
> let number = (iccInfo instanceof Ci.nsIDOMMozGsmIccInfo)
>                ? iccInfo.msisdn : iccInfo.mdn;
> "
> And get the number right after QI nsIDOMMozCdmaIccInfo/nsIDOMMozGsmIccInfo
> 
> @@ +342,3 @@
> >      return number;
> >    },
> >  
> 
> Hence, the getPhoneNumber() can be rewritten in this way:
> 
>   getPhoneNumber: function() {
>     let number;
>     // Get the proper IccInfo based on the current card type.
>     try {
>       let iccInfo = null;
>       let baseIccInfo = this.radioInterface.rilContext.iccInfo;
>       if (baseIccInfo.iccType === 'ruim' || baseIccInfo.iccType === 'csim') {
>         iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozCdmaIccInfo);
>         number = iccInfo.mdn;
>       } else {
>         iccInfo = baseIccInfo.QueryInterface(Ci.nsIDOMMozGsmIccInfo);
>         number = iccInfo.msisdn;
>       }
>     } catch (e) {
>       if (DEBUG) {
>        debug("Exception - QueryInterface failed on iccinfo for GSM/CDMA
> info");
>       }
>       return null;
>     }
> 
>     // Workaround an xpconnect issue with undefined string objects.
>     // See bug 808220
>     if (number === undefined || number === "undefined") {
>       return null;
>     }
> 
>     return number;
>   },
> 
> @@ +1099,5 @@
> >    // Insert Phone number if available.
> >    // Otherwise, Let MMS Proxy Relay insert from address automatically for us.
> >    let phoneNumber = mmsConnection.getPhoneNumber();
> > +  let from = {address: phoneNumber, type: "PLMN"};
> > +  msg.headers["from"] = from;
> 
> We should keep this unchanged because msg.header["from"] = null is
> meaningful to MmsPduHelper to encode |From| with |Insert-address-token| when
> |phoneNumber| is invalid.
> 
> See
> http://hg.mozilla.org/mozilla-central/file/aab3362f97e9/dom/mobilemessage/
> src/gonk/MmsPduHelper.jsm#l889
> 
> @@ +1394,5 @@
> >    // Insert Phone number if available.
> >    // Otherwise, Let MMS Proxy Relay insert from address automatically for us.
> >    let phoneNumber = mmsConnection.getPhoneNumber();
> > +  let from = {address: phoneNumber, type: "PLMN"};
> > +  headers["from"] = from;
> 
> ditto.
Posted patch Patch to fix the issue. (obsolete) — Splinter Review
Tested both cases where phoneNumber is valid and phonenumber is undefined.
Works correctly for both cases. I see an <insert address> token for the header["from"] = null case which is the correct behavior.
Bevis, If you are ok with this patch, please land it.

Thanks,
Nivi.
Attachment #8440069 - Flags: feedback?(btseng)
Flags: needinfo?(btseng)
Attachment #8439540 - Attachment is obsolete: true
(In reply to Nivi from comment #3)
> Hi Bevis,

> if you do this directly - 
> > +  msg.headers["from"] = {address: phoneNumber, type: "PLMN"} : null;
> This doesn't work. I have tested this. Please note that the same is done for
> msg.headers["to"].
> 
> So the thing I suggested is - 
> 
> > +  let from = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} : null;
> > +  msg.headers["from"] = from;
>
I am not pretty sure if you misunderstood what I meant.
I think the original code before patching has the same logic as your, that's why I asked you to keep it unchanged. :)
  msg.headers["from"] = (phoneNumber) ?
                          { address: phoneNumber, type: "PLMN" } : null;

BTW, thanks for your feedback of this patch.
I'll revise your patch again and have the peer to review before landing. :)
Flags: needinfo?(btseng)
Attachment #8440069 - Flags: feedback?(btseng)
I'd like to double confirm the original logic to set msg.headers["from"] shall be workable without change since it has been tested in bug 1007542:
"
  msg.headers["from"] = (phoneNumber) ?
                          { address: phoneNumber, type: "PLMN" } : null;
"
Flags: needinfo?(nsarkar)
Assignee: nobody → btseng
Attachment #8440069 - Attachment is obsolete: true
Attachment #8441132 - Flags: review?(vyang)
Attachment #8441132 - Attachment description: Patch: Do QueryInterface before accessing iccInfo from nsIRilContext. → Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.
Comment on attachment 8441132 [details] [diff] [review]
Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.

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

I'd r+ this patch if that username is set back to Nivi. I appreciate the clarification you made the this patch very much, but the authorship means a lot to me, especially when there is little, simple removal here, changes made in comparison to the original one.  Thank you.
Attachment #8441132 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #8)
> Comment on attachment 8441132 [details] [diff] [review]
> Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.
> 
> Review of attachment 8441132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd r+ this patch if that username is set back to Nivi. I appreciate the
> clarification you made the this patch very much, but the authorship means a
> lot to me, especially when there is little, simple removal here, changes
> made in comparison to the original one.  Thank you.

Sure, I am glad to do this for Nivi.
I'll update new patch to address this. :)
Update |Username| to Nivi.
Attachment #8441132 - Attachment is obsolete: true
Attachment #8441230 - Flags: review?(vyang)
Attachment #8441230 - Flags: review?(vyang) → review+
Comment on attachment 8441230 [details] [diff] [review]
Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.

Hi Bevis,

As I mentioned before, we also need the lines -

1.
let from = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} : null;
msg.headers["from"] = from;

for some reason - 
2.
msg.headers["from"] = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} : null;

does not work. Ideally both should work but somehow 1 seems to work fine but 2 doesn't. It's weird. :(

I have tried both 1 and 2 and for some reason, only 1 works. Please let me know if you have any further questions.

Thanks,
Nivi.
Attachment #8441230 - Flags: feedback-
Flags: needinfo?(nsarkar)
blocking-b2g: 1.4? → 1.4+
(In reply to Nivi from comment #11)
> Comment on attachment 8441230 [details] [diff] [review]
> Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.
> 
> Hi Bevis,
> 
> As I mentioned before, we also need the lines -
> 
> 1.
> let from = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} : null;
> msg.headers["from"] = from;
> 
> for some reason - 
> 2.
> msg.headers["from"] = (phoneNumber) ? {address: phoneNumber, type: "PLMN"} :
> null;
> 
> does not work. Ideally both should work but somehow 1 seems to work fine but
> 2 doesn't. It's weird. :(
> 
> I have tried both 1 and 2 and for some reason, only 1 works. Please let me
> know if you have any further questions.
> 
> Thanks,
> Nivi.

Thanks, I'll apply this part into the patch as well.
apply the suggestion in comment 11 and has been verified fixed locally.

Wait for try server result:
https://tbpl.mozilla.org/?tree=Try&rev=ae7e2eb9a905
Attachment #8441230 - Attachment is obsolete: true
Attachment #8441863 - Flags: review+
Thanks Bevis.


(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #13)
> Created attachment 8441863 [details] [diff] [review]
> Patch v3: Do QueryInterface before accessing iccInfo from nsIRilContext.
> r=vyang, a=1.4+
> 
> apply the suggestion in comment 11 and has been verified fixed locally.
> 
> Wait for try server result:
> https://tbpl.mozilla.org/?tree=Try&rev=ae7e2eb9a905
https://hg.mozilla.org/mozilla-central/rev/06074b86c480
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
Flags: in-moztrap?(bzumwalt)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.