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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Nivi]

Attachment: Proposed patch to fix this issue. Tested and this works.

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.

Comment 1

[Nivi]

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 2

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

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.

Comment 3

[Nivi]

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] ([e-mail]) 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.

Comment 4

[Nivi]

Attachment: Patch to fix the issue.

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.

Comment 5

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

(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. :)

Comment 6

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

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;
"

Comment 7

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

Attachment: Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.

Comment 8

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

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.

Comment 9

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

(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. :)

Comment 10

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

Attachment: Patch v1: Do QueryInterface before accessing iccInfo from nsIRilContext.

Update |Username| to Nivi.

Comment 11

[Nivi]

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.

Comment 12

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

(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.

Comment 13

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

Attachment: 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

Comment 14

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

try server is green:
https://tbpl.mozilla.org/?tree=Try&rev=ae7e2eb9a905

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/06074b86c480

Comment 16

[Nivi]

Thanks Bevis.


(In reply to Bevis Tseng [:bevistseng] ([e-mail]) 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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06074b86c480

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7ba7fa18515
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4be310c15d02

Comment 19

[Brogan Zumwalt [Inactive]]

No STR is present to create test case to address bug.