1043250 - [B2G][SMS] Update getSmscAddress API to support variant SMSC address formats on different devices.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[:gerard-majax]

On my Nexus S running master, the SMSC number visible in Settings is not a valid one. This is reproducing at least with my Free Mobile SIM card.

SMSC is expected to be [phone], but the value exposed is 07913396050096f5.

Comment 1

[:gerard-majax]

Attachment: Read proper SMSC value on Nexus S

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S
returns an invalid SMSC value (07913396050096f5). Hacking a bit the way
we read the GET_SMSC reply I get the proper value being read. This does
not reproduce on other devices I could test with (Flame, namely).

Comment 2

[:gerard-majax]

Comment on attachment 8461441 [details] [diff] [review]
Read proper SMSC value on Nexus S

Hsin, do you mind having a look at this ? Maybe you can find a proper reason that explains why it does not work in the first place, and help me produce a nicer patch :)

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8461441 [details] [diff] [review]
Read proper SMSC value on Nexus S

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

Sorry I don't have enough bandwidth. :(

Bevis, would you mind helping this?

Comment 4

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

Comment on attachment 8461441 [details] [diff] [review]
Read proper SMSC value on Nexus S

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

Sorry to give you feedback-. 

My reasons are 
1. It seems that you are trying to interpret the response as the RP-Destination address Centre Address defined in TS 24.011.
2. However, according to the RIL in AOSP[1][2] and the corresponding usage of the SMSC in RadioInfo Activity of AOSP[3][4],
   the response of RIL_REQUEST_GET_SMSC_ADDRESS is more likely to be a plain text of the smsc address.
   If you have other Android Phone, you can double confirm this by the following steps:
   a. Type *#*#4636#*#* in the dialer.
   b. Go into "Phone Information"
   c. Press the "Refresh" button of the SMSC to read the SMSC address.
3. Other devices than Nexus S will get into trouble if we apply this patch.

To me, it's more likely to be a Nexus S specific issue instead.

[1]http://androidxref.com/4.4.4_r1/xref/hardware/ril/include/telephony/ril.h#3320
[2]http://androidxref.com/4.4.4_r1/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/RIL.java#2423
[3]http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/RadioInfo.java#227
[4]http://androidxref.com/4.4.4_r1/xref/packages/apps/Settings/src/com/android/settings/RadioInfo.java#1056

Comment 5

[:gerard-majax]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #4)
> Comment on attachment 8461441 [details] [diff] [review]
> Read proper SMSC value on Nexus S
> 
> Review of attachment 8461441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry to give you feedback-. 

That's no problem, I asked feedback to know how to do it properly.

> 
> My reasons are 
> 1. It seems that you are trying to interpret the response as the
> RP-Destination address Centre Address defined in TS 24.011.

Then we should implement this parsing, shouldn't we?

> 2. However, according to the RIL in AOSP[1][2] and the corresponding usage
> of the SMSC in RadioInfo Activity of AOSP[3][4],

I would not consider AOSP as being the reference for specifications.

>    the response of RIL_REQUEST_GET_SMSC_ADDRESS is more likely to be a plain
> text of the smsc address.

*More likely*

>    If you have other Android Phone, you can double confirm this by the
> following steps:
>    a. Type *#*#4636#*#* in the dialer.
>    b. Go into "Phone Information"
>    c. Press the "Refresh" button of the SMSC to read the SMSC address.
> 3. Other devices than Nexus S will get into trouble if we apply this patch.
> 
> To me, it's more likely to be a Nexus S specific issue instead.

How can you judge so ? Specifically when your links are referring to KK codebase while Nexus S is ICS. I'll look into the specs for the proper encoding.

Comment 6

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

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> > To me, it's more likely to be a Nexus S specific issue instead.
> 
> How can you judge so ? Specifically when your links are referring to KK
> codebase while Nexus S is ICS. I'll look into the specs for the proper
> encoding.

Actually, the implementation is the same even in the 1st AOSP release.
(I've involved in it. :p)

After more comparison, it's not Nexus S issue but a bad API design:
I've tried *#*#4636#*#* on both HTC One & Nexus 5 and found something interesting:
1. In HTC device, the response is readable phone number "[phone]".
2. In Nexus 5, the response is in form of '"[phone]",145' which is formed in +CSCA response defined TS 27.005.
If we take your Nexus S (07913396050096f5) into account, that means there are 3 different format of this response.

Hence, treat it as a plain text will be more flexible to present this field with various modem solution.
However, it becomes difficult to the user to read and update this field. :(

Comment 7

[:gerard-majax]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #6)
> (In reply to Alexandre LISSY :gerard-majax from comment #5)
> > > To me, it's more likely to be a Nexus S specific issue instead.
> > 
> > How can you judge so ? Specifically when your links are referring to KK
> > codebase while Nexus S is ICS. I'll look into the specs for the proper
> > encoding.
> 
> Actually, the implementation is the same even in the 1st AOSP release.
> (I've involved in it. :p)
> 
> After more comparison, it's not Nexus S issue but a bad API design:
> I've tried *#*#4636#*#* on both HTC One & Nexus 5 and found something
> interesting:
> 1. In HTC device, the response is readable phone number "[phone]".
> 2. In Nexus 5, the response is in form of '"[phone]",145' which is
> formed in +CSCA response defined TS 27.005.
> If we take your Nexus S (07913396050096f5) into account, that means there
> are 3 different format of this response.
> 
> Hence, treat it as a plain text will be more flexible to present this field
> with various modem solution.
> However, it becomes difficult to the user to read and update this field. :(

The values for the HTC One and Nexus 5 seems to be proper strings, while the Nexus S one clearly involves some BCD string. Could it be that we are just missing some specs implementation ? I already got similar things around ICCIO when hacking on HTC Desire Z, so I would not be surprised :)

Comment 8

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

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> The values for the HTC One and Nexus 5 seems to be proper strings, while the
> Nexus S one clearly involves some BCD string. Could it be that we are just
> missing some specs implementation ? I already got similar things around
No, so far as I know, according to TS 27.005 and TS 24.011, SMSC was formed in either text or pdu format as what Nexus 5 & Nexus S presents, and HTC One just represents decoded one instead. :)
> ICCIO when hacking on HTC Desire Z, so I would not be surprised :)

Comment 9

[:gerard-majax]

Attachment: Read proper SMSC value on Nexus S

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S
returns an invalid SMSC value (07913396050096f5). Hacking a bit the way
we read the GET_SMSC reply I get the proper value being read. This does
not reproduce on other devices I could test with (Flame, namely).

Comment 10

[:gerard-majax]

Attachment: Read proper SMSC value on Nexus S

For some reason, Buf.readString() on my Free Mobile SIM card in Nexus S
returns an invalid SMSC value (07913396050096f5). Hacking a bit the way
we read the GET_SMSC reply I get the proper value being read. This does
not reproduce on other devices I could test with (Flame, namely).

Comment 11

[Samael Wang [:freesamael] (away for now)]

As mentioned in the meeting yesterday, I recommend to have a per-device build time configuration to let Gecko know what underlying representation format is in use on the running device, so we can ensure that Gecko always converts and returns the same format (which I assume readable number / ITU E.164 format is preferred) to Gaia apps.

I haven't figured out where to put those configs, though. Certainly we're not going to hard-code it in the source.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Samael Wang [:freesamael][:sawang] from comment #11)
> As mentioned in the meeting yesterday, I recommend to have a per-device
> build time configuration to let Gecko know what underlying representation
> format is in use on the running device, so we can ensure that Gecko always
> converts and returns the same format (which I assume readable number / ITU
> E.164 format is preferred) to Gaia apps.
> 
> I haven't figured out where to put those configs, though. Certainly we're
> not going to hard-code it in the source.

You may refer to [1] which is a config for only shinaro devices [2].


[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#51
[2] https://github.com/mozilla-b2g/device-shinano-common

Comment 13

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

[Tracking Requested - why for this release]:

Comment 14

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Update ril_worker and xpcshell test

ril_worker.js:
- Remove SMSC attribute.
- Rewrite REQUEST_GET_SMSC_ADDRESS function.
- Remove the usage of rilRequestError since it's removed in Bug 991582.

test_ril_worker_smsc_address.js:
- Add a test case for getSmscAddress.

Comment 15

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Update MobileMessageCallback and SmsService. r=btseng

nsIMobileMessageCallback.idl
- Update the signature of NotifyGetSmscAddress and uuid.

MobileMessageCallback.cpp:
- Update NotifyGetSmscAddress to reflect the signature change and to
  adapt Promise.
- Update NotifyGetSmscAddressFailed to adapt Promise.

nsISmsService.idl:
- Update the comment to emphasize NUMBER_PLAN_IDENTIFICATION constants
  reflects enumeration values.

SmsService.js:
- Update the invocation of notifySmscAddress as the signature changes.

Comment 16

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng

MobileMessageManager / MozMobileMessageManager.webidl:
- Adapt Promise in GetSmscAddress function instead of DOMRequest.

Comment 17

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 4: Update SMS IPC implementation. r=btseng

PSmsRequest.ipdl:
- Update ReplyGetSmscAddress to include extra parameters.

SmsChild:
- Update Recv__delete__ to reflect the change of PSmsRequest.

SmsParent:
- Update NotifyGetSmscAddress to reflect the change of PSmsRequest.

Comment 18

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 5: Update marionette test cases. r=btseng

test_set_smsc_address / test_smsc_address:
- Update the test cases to reflect the interface change.

Comment 19

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633325 [details] [diff] [review]
Part 1: Update ril_worker and xpcshell test

Hi Bevis,

Could you help to review this patch? 

BTW I've also removed the usage of rilRequestError of REQUEST_SET_SMSC_ADDRESS in this patch since all rilRequestError usages in ril_worker are cleaned up in Bug 991582.

Comment 20

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633326 [details] [diff] [review]
Part 2: Update MobileMessageCallback and SmsService. r=btseng

Hi Bevis,

Could you help to review this patch?

Comment 21

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633327 [details] [diff] [review]
Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng

Hi Bevis,

Could you help to review this patch?

Comment 22

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633328 [details] [diff] [review]
Part 4: Update SMS IPC implementation. r=btseng

Hi Bevis,

Could you help to review this patch?

Comment 23

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633329 [details] [diff] [review]
Part 5: Update marionette test cases. r=btseng

Hi Bevis,

Could you help to review this patch?

Comment 24

[Samael Wang [:freesamael] (away for now)]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f34eeb7fca26

Comment 25

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

Comment on attachment 8633325 [details] [diff] [review]
Part 1: Update ril_worker and xpcshell test

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

Per offline discussion, please help to ensure that additional user cases are supported:
1. In PDU mode, we have to identify the scenario that the value of this setting is empty.
2. In Text mode, 
   - Empty.
   - What if modem doesn't reply with ",<tosca>"
     <tosca> for setting is optional but is required when retrieving.
     (It's handy for us to identify this if modem doesn't support this well.)

Thanks!

Comment 26

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

Comment on attachment 8633326 [details] [diff] [review]
Part 2: Update MobileMessageCallback and SmsService. r=btseng

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

Looks good to me!

Thanks!

Comment 27

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

Comment on attachment 8633327 [details] [diff] [review]
Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng

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

Looks good to me.
Rember to invite hsinyi to review the webidl change.

Please also set dependency to a new Gaia bug in which 
the implementation adopts DOMRequest.then() to support both 
the legacy one and this new API.

Thanks!

Comment 28

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

Comment on attachment 8633328 [details] [diff] [review]
Part 4: Update SMS IPC implementation. r=btseng

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

LGTM, thanks!

Comment 29

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

Comment on attachment 8633329 [details] [diff] [review]
Part 5: Update marionette test cases. r=btseng

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

LGTM, thanks! :)

Comment 30

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Update ril_worker and xpcshell test

ril_worker.js:
- Remove SMSC attribute.
- Rewrite REQUEST_GET_SMSC_ADDRESS function.
- Remove the usage of rilRequestError since it's removed in Bug 991582.

test_ril_worker_smsc_address.js:
- Add a test case for getSmscAddress.

Comment 31

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1(v2): Update ril_worker and xpcshell test

ril_worker.js:
- Remove SMSC attribute.
- Rewrite REQUEST_GET_SMSC_ADDRESS function.
- Remove the usage of rilRequestError since it's removed in Bug 991582.

test_ril_worker_smsc_address.js:
- Add a test case for getSmscAddress.

Comment 32

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8635158 [details] [diff] [review]
Part 1(v2): Update ril_worker and xpcshell test

Hi Bevis, could you help to review the updated patch?

Comment 33

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8633327 [details] [diff] [review]
Part 3: Update MozMobileMessageManager WebIDL interface and implementation. r=htsai, btseng

Hi Hsinyi, could you help to review the Web IDL interface change?

Comment 34

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

Comment on attachment 8635158 [details] [diff] [review]
Part 1(v2): Update ril_worker and xpcshell test

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

Please see my comments inline.

Thanks!

::: dom/system/gonk/ril_worker.js
@@ +5514,5 @@
> +  const TON_UNKNOWN = (TOA_UNKNOWN & 0x70) >> 4;
> +  let tonValue = (tosca & 0x70) >> 4;
> +  let ton = TON_UNKNOWN;
> +  for (let i = 0; i < CALLED_PARTY_BCD_NPI.length; i++) {
> +    if (CALLED_PARTY_BCD_NPI[i] === tonValue) {

Oops!
It seems that you incorrectly mapped NPI with tonValue instead of the npi one.

::: dom/system/gonk/tests/test_ril_worker_smsc_address.js
@@ +28,2 @@
>      equal(this.readString(), expected);
> +

nits: please remove this line.

@@ +97,5 @@
> +  getSmsc(worker, context, SMSC_O2_TEXT, SMSC_O2, 1, 1);
> +  getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT, SMSC_TON_UNKNOWN, 0, 1);
> +  getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT_NO_TOA, SMSC_TON_UNKNOWN, 0, 1);
> +  getSmsc(worker, context, SMSC_TON_UNKNOWN_TEXT_INVALID_TOA, SMSC_TON_UNKNOWN,
> +    0, 1);

Per offline discuss, please help to add test coverage of empty sms for text mode.

Thanks!

Comment 35

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

Comment on attachment 8635158 [details] [diff] [review]
Part 1(v2): Update ril_worker and xpcshell test

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

::: dom/system/gonk/ril_worker.js
@@ +5502,5 @@
> +    if (segments.length === 2) {
> +      tosca = this.parseInt(segments[1], TOA_UNKNOWN, 10);
> +    }
> +
> +    smsc = segments[0].replace(/\"/ig, "");

nits: no need to ignore case:
smsc = segments[0].replace(/\"/g, "");

Comment 36

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1(v3): Update ril_worker and xpcshell test

ril_worker.js:
- Remove SMSC attribute.
- Rewrite REQUEST_GET_SMSC_ADDRESS function.
- Remove the usage of rilRequestError since it's removed in Bug 991582.

test_ril_worker_smsc_address.js:
- Add a test case for getSmscAddress.

Comment 37

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8635224 [details] [diff] [review]
Part 1(v3): Update ril_worker and xpcshell test

Hi Bevis,

Would you help to review the updated patch?

Comment 38

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

Comment on attachment 8635224 [details] [diff] [review]
Part 1(v3): Update ril_worker and xpcshell test

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

r=me after the comments addressed.

Thanks!

::: dom/system/gonk/ril_worker.js
@@ +5509,5 @@
> +  // Convert the NPI value to the corresponding index of CALLED_PARTY_BCD_NPI
> +  // array. If the value does not present in the array, use
> +  // CALLED_PARTY_BCD_NPI_ISDN.
> +  let npi = CALLED_PARTY_BCD_NPI.indexOf(tosca & 0xf);
> +  if (npi === -1)

nit: Please always have {} closure protected even there is only one statement in the if-statement.

In addition, please add a brief note at:
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#1414
that the indexed value shall be consistent to nsISmsService::NUMBER_PLAN_IDENTIFICATION_*.

Comment 39

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1(v4): Update ril_worker and xpcshell test. r=btseng

ril_worker.js:
- Remove SMSC attribute.
- Rewrite REQUEST_GET_SMSC_ADDRESS function.
- Remove the usage of rilRequestError since it's removed in Bug 991582.

test_ril_worker_smsc_address.js:
- Add a test case for getSmscAddress.

ril_const.js:
- Add more clear comment for CALLED_PARTY_BCD_NPI array.

Comment 40

[Samael Wang [:freesamael] (away for now)]

try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=497d872454f5

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/281c89d5582f
https://hg.mozilla.org/integration/b2g-inbound/rev/8288be78c5f5
https://hg.mozilla.org/integration/b2g-inbound/rev/dd174c137868
https://hg.mozilla.org/integration/b2g-inbound/rev/a254def80fe6
https://hg.mozilla.org/integration/b2g-inbound/rev/d5f6df9fc4a7

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/281c89d5582f
https://hg.mozilla.org/mozilla-central/rev/8288be78c5f5
https://hg.mozilla.org/mozilla-central/rev/dd174c137868
https://hg.mozilla.org/mozilla-central/rev/a254def80fe6
https://hg.mozilla.org/mozilla-central/rev/d5f6df9fc4a7