958733 - [cdma][supplementary services] GSM SS are being initiated when acquired on CDMA network

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Michael Schwartz [:m4]]

Steps:

# Acquire on a CDMA network
# Dial a GSM SS eg. *#43#

Actual:
# Attempt to initiate a GSM supplementary service which cannot succeed on a CDMA network

Expected:
# Initiate a CDMA call to given dial string

Comment 1

[Ken Chang[:kenkai]]

Hsinyi, are we going to move the decision rule of supplementary service to Gaia?

Comment 2

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

(In reply to Ken Chang[:ken] from comment #1)
> Hsinyi, are we going to move the decision rule of supplementary service to
> Gaia?

No, instead, we are going to move the determination algorithm to gecko. That's what bug 889737 tries to do.

Right now, gaia/dialer checks if the dialing string ends with #. If so, dialer calls 'mobileConnection.sendMMI()' that causes some problem on CDMA networks as this bug reports. If we want to have a quick workaround for this bug on gaia instead of waiting for the full solution on bug 889737 landed, dialer should have a careful check on the network type as well.

Comment 3

[[:fabrice] Fabrice Desré]

Gabriele, is that something you could do?

Comment 4

[Ken Chang[:kenkai]]

Joe, this bug depends on bug 889737. However, Bug 889737 depends on bug 969218 and bug 969280. I wonder if you could find someone to take the bug 969280.

Comment 5

[Ken Chang[:kenkai]]

(In reply to Ken Chang[:ken] from comment #4)
> Joe, this bug depends on bug 889737. However, Bug 889737 depends on bug
> 969218 and bug 969280. I wonder if you could find someone to take the bug
> 969280.
Anthony takes bug 969280. Thanks

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Gabriele, is that something you could do?

Yes, it should be a fairly simple job once the dependencies are fixed.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

features should not block. remove blocking flag

Comment 8

[Kai-Chih Hu [:khu]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > Gabriele, is that something you could do?
> 
> Yes, it should be a fairly simple job once the dependencies are fixed.

Thanks! So, could you be the owner for this bug? Then, we could know that once the dependencies are all fixed, we will have you to help this bug. Thank you.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

hi Gabriele, do you mind taking the bug? if you will be working on this once the depending bug is fixed. thanks

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Joe Cheng [:jcheng] from comment #9)
> hi Gabriele, do you mind taking the bug? if you will be working on this once
> the depending bug is fixed. thanks

Taken :)

Comment 11

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

As the work of Bug 889737 is huge, we don't think we are able to finish that by Feb. 28. I'd like to propose a gaia workaround for this issue.

My proposal:
In dialer/js/mmi.js, isMMI() considers two conditions:
1) return false if the network is CDMA.
2) return (number.charAt(number.length - 1) === '#') // The existing criterion

How do you think, Gabriele?

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #11)
> As the work of Bug 889737 is huge, we don't think we are able to finish that
> by Feb. 28. I'd like to propose a gaia workaround for this issue.
> 
> My proposal:
> In dialer/js/mmi.js, isMMI() considers two conditions:
> 1) return false if the network is CDMA.
> 2) return (number.charAt(number.length - 1) === '#') // The existing
> criterion
> 
> How do you think, Gabriele?

I'll let Gabriele comment on the pertinence.
But it looks simple/testable enough :)

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #11)
> My proposal:
> In dialer/js/mmi.js, isMMI() considers two conditions:
> 1) return false if the network is CDMA.
> 2) return (number.charAt(number.length - 1) === '#') // The existing
> criterion
> 
> How do you think, Gabriele?

Pretty simple as Etienne said, and definitely testable. Shall we go for this solution and then open a follow up for when mobileConnection.sendMMI() will be gone?

Comment 14

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

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #11)
> > My proposal:
> > In dialer/js/mmi.js, isMMI() considers two conditions:
> > 1) return false if the network is CDMA.
> > 2) return (number.charAt(number.length - 1) === '#') // The existing
> > criterion
> > 
> > How do you think, Gabriele?
> 
> Pretty simple as Etienne said, and definitely testable. Shall we go for this
> solution and then open a follow up for when mobileConnection.sendMMI() will
> be gone?

Sounds really good. +1!

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Never validate MMI codes in CDMA networks

Here's a patch that modifies MmiManager.isMMI() to never validate MMI codes when a CDMA network is detected; comes with unit-tests. The only thing I'm not sure about is if the type of network is right: I'm currently checking just for 'cdma' but there's apparently plenty of variations (wcdma, evdo, etc...). If this test needs to behave similarly for them I'd need a list to be sure not to miss any.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Never validate MMI codes in CDMA networks

Comment 17

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

Comment on attachment 8380732 [details] [diff] [review]
[PATCH] Never validate MMI codes in CDMA networks

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

There's no type "cdma" delivered. Sorry that the MobileConnection API didn't explain clear enough. :(

::: apps/communications/dialer/js/mmi.js
@@ +281,5 @@
> +               window.navigator.mozMobileConnections &&
> +               window.navigator.mozMobileConnections[0];
> +
> +    if (conn.voice.type === 'cdma') {
> +      return false;

Here's my suggestion:

var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
var conn = window.navigator.mozMobileConnection ||
           window.navigator.mozMobileConnections &&
           window.navigator.mozMobileConnections[0];

if (cdmaTypes.indexOf(conn.voice.type) !== -1) {
  return false;
}

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #17)
> There's no type "cdma" delivered. Sorry that the MobileConnection API didn't
> explain clear enough. :(

Yeah, that was precisely my doubt :) Let me fix the patch.

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v2] Never validate MMI codes in CDMA networks

Updated patch with the list of CDMA networks baked in.

Comment 20

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2] Cleanup linter errors in the MMI-related code

... and jshint cleanup of the affected files for good measure.

Comment 21

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

Comment on attachment 8381229 [details] [diff] [review]
[PATCH 1/2 v2] Never validate MMI codes in CDMA networks

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

Looks good and works fine on cdma devices. Thank you!

Comment 22

[Anthony Ricaud (:rik)]

Comment on attachment 8381229 [details] [diff] [review]
[PATCH 1/2 v2] Never validate MMI codes in CDMA networks

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

::: apps/communications/dialer/js/mmi.js
@@ +275,5 @@
>      window.postMessage(data, this.COMMS_APP_ORIGIN);
>    },
>  
>    isMMI: function mm_isMMI(number) {
> +    // XXX: workaround until bug 889737 gets fixed and we can drop this function

Please move the comment above the if.

@@ +277,5 @@
>  
>    isMMI: function mm_isMMI(number) {
> +    // XXX: workaround until bug 889737 gets fixed and we can drop this function
> +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> +    var conn = window.navigator.mozMobileConnection ||

I think mozMobileConnection is history. We shouldn't use it in new code.

Comment 23

[Anthony Ricaud (:rik)]

Hsin-Yi: Can you confirm that navigator.mozMobileConnection is history?

Gabriele: You can land this with only one commit (after Travis is green of course ;) ).

Comment 24

[Kai-Chih Hu [:khu]]

Is this bug in 'checkin-needed' stage? 
Can we land the fix because we already got review+? :-)

Comment 25

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

(In reply to Anthony Ricaud (:rik) from comment #23)
> Hsin-Yi: Can you confirm that navigator.mozMobileConnection is history?
> 
Confirmed: indeed history!

> Gabriele: You can land this with only one commit (after Travis is green of
> course ;) ).

Comment 26

[Enpei]

Test case: https://moztrap.mozilla.org/manage/case/11710/

Comment 27

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> (In reply to Anthony Ricaud (:rik) from comment #23)
> > Hsin-Yi: Can you confirm that navigator.mozMobileConnection is history?
> > 
> Confirmed: indeed history!
> 
> > Gabriele: You can land this with only one commit (after Travis is green of
> > course ;) ).

Hi guys,

Sorry that please HOLD the check-in for a while.
There's a very tricky case  -- *#06#. *#06# is a very special case and the string is checked in keypad.js. Right now, we call sendMMI(*#06#) to get IMEI. Actually *#06# (IMEI) is only useful and meaningful in gsm/world phones. Wasabi is a world phone so that we can get IMEI via *#06# even the network camped on is cdma. This patch checks the camped network type to enforce dial() for cdma networks. This is nothing wrong I'd say. However, as *#06# requires special handling and to not break the behaviour we have on wasabi before this patch, I'd suggest we do one more thing: we simply use sendMMI() for *#06#.

Or a more concrete solution:
We do special handle for *#06# if mobileconnecitons[0].supportedNetwokTypes contain 'gsm' or 'wcdma' or 'lte'. But I guess this work might huge?!

Comment 28

[Gabriele Svelto [:gsvelto] (PTO)]

OK, so let's do two things here: first I'll get rid of the use of mozMobileConnection, then I'll whitelist the *#06# code as Hsin-Yi has suggested on 'gsm', 'wcdma', and 'lte' phones. IMHO this is no big deal as this is really a workaround. Once the real fix will be in place this code will go away anyway.

Comment 29

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

(In reply to Gabriele Svelto [:gsvelto] from comment #28)
> OK, so let's do two things here: first I'll get rid of the use of
> mozMobileConnection, then I'll whitelist the *#06# code as Hsin-Yi has
> suggested on 'gsm', 'wcdma', and 'lte' phones. IMHO this is no big deal as
> this is really a workaround. Once the real fix will be in place this code
> will go away anyway.

Agree this is not a big deal. Either way is okay. :)

Comment 30

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v3] Never validate MMI codes in CDMA networks

Updated patch with the IMEI requested code appropriately whitelisted as per Hsin-Yi's description in comment 27; an additional unit-test has been added for this. I have not removed the use of |window.navigator.mozMobileConnection| as it would break the tests and as discussed we should move the fixes for the tests in a follow-up bug we'll use to clean up all uses of that obsolete field.

Requesting review again as the changes are significant.

Comment 31

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

(In reply to Gabriele Svelto [:gsvelto] from comment #30)
> Created attachment 8382027 [details] [diff] [review]
> [PATCH 1/2 v3] Never validate MMI codes in CDMA networks
> 
> Updated patch with the IMEI requested code appropriately whitelisted as per
> Hsin-Yi's description in comment 27; an additional unit-test has been added
> for this. I have not removed the use of
> |window.navigator.mozMobileConnection| as it would break the tests and as
> discussed we should move the fixes for the tests in a follow-up bug we'll
> use to clean up all uses of that obsolete field.
> 
> Requesting review again as the changes are significant.

Thanks Gabriele. This works fine!

Comment 32

[Anthony Ricaud (:rik)]

Comment on attachment 8382027 [details] [diff] [review]
[PATCH 1/2 v3] Never validate MMI codes in CDMA networks

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

There's a difference in the code and the comments so this is confusing. No r+ because of that.

We can discuss on IRC. In any case, ping me on IRC when you put this in review so I can re-review quickly.

::: apps/communications/dialer/js/mmi.js
@@ +277,5 @@
>  
>    isMMI: function mm_isMMI(number) {
> +    // XXX: workaround until bug 889737 gets fixed and we can drop this function
> +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> +    var conn = window.navigator.mozMobileConnection ||

No mozMobileConnection.

@@ +281,5 @@
> +    var conn = window.navigator.mozMobileConnection ||
> +               window.navigator.mozMobileConnections &&
> +               window.navigator.mozMobileConnections[0];
> +    var voiceType = conn.voice ? conn.voice.type : null;
> +    var networkTypes = conn.supportedNetworkTypes;

var supportedNetworkTypes

@@ +282,5 @@
> +               window.navigator.mozMobileConnections &&
> +               window.navigator.mozMobileConnections[0];
> +    var voiceType = conn.voice ? conn.voice.type : null;
> +    var networkTypes = conn.supportedNetworkTypes;
> +    var imeiWhitelist = function mm_imeiWhitelist(element, index, array) {

Drop index and array if we're not using them.

@@ +287,5 @@
> +      return (['gsm', 'lte', 'wcdma'].indexOf(element) !== -1);
> +    };
> +
> +    if ((number === '*#06#') && networkTypes.some(imeiWhitelist)) {
> +      // Requesting the IMEI code works on GSM networks and certain CDMA ones

Should read // Requesting IMEI works on phones supporting a GSM network

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +62,5 @@
> +
> +      delete MockMozMobileConnection.voice.type;
> +    });
> +
> +    test('Requesting the IMEI is allowed on some CDMA networks', function() {

Requesting the IMEI is allowed on phones supporting GSM networks

@@ +72,5 @@
> +      MockMozMobileConnection.voice.type = 'evdoa';
> +      assert.isFalse(MmiManager.isMMI('*#06#'));
> +
> +      delete MockMozMobileConnection.voice.type;
> +      delete MockMozMobileConnection.supportedNetworkTypes;

You can move those in a teardown(). Same for the test above.

Comment 33

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v4] Never validate MMI codes in CDMA networks

Updated patch, just a couple of quick comments on it:

(In reply to Anthony Ricaud (:rik) from comment #32)
> Review of attachment 8382027 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: apps/communications/dialer/js/mmi.js
> @@ +277,5 @@
> >  
> >    isMMI: function mm_isMMI(number) {
> > +    // XXX: workaround until bug 889737 gets fixed and we can drop this function
> > +    var cdmaTypes = ['evdo0', 'evdoa', 'evdob', '1xrtt', 'is95a', 'is95b'];
> > +    var conn = window.navigator.mozMobileConnection ||
> 
> No mozMobileConnection.

Removing mozMobileConnection requires me to change the tests and this touches other parts of the code so I'd rather not do it here. As we discussed during this morning stand-up meeting I'll open follow-ups to remove all uses of mozMobileConnection and update all the affected unit-tests.

> @@ +281,5 @@
> > +    var conn = window.navigator.mozMobileConnection ||
> > +               window.navigator.mozMobileConnections &&
> > +               window.navigator.mozMobileConnections[0];
> > +    var voiceType = conn.voice ? conn.voice.type : null;
> > +    var networkTypes = conn.supportedNetworkTypes;
> 
> var supportedNetworkTypes

Done.

> @@ +282,5 @@
> > +               window.navigator.mozMobileConnections &&
> > +               window.navigator.mozMobileConnections[0];
> > +    var voiceType = conn.voice ? conn.voice.type : null;
> > +    var networkTypes = conn.supportedNetworkTypes;
> > +    var imeiWhitelist = function mm_imeiWhitelist(element, index, array) {
> 
> Drop index and array if we're not using them.

Done, I wasn't sure if I could do it safely. Coming from a long C coding background makes you very shaky in doing this kind of stuff :-p

> @@ +287,5 @@
> > +      return (['gsm', 'lte', 'wcdma'].indexOf(element) !== -1);
> > +    };
> > +
> > +    if ((number === '*#06#') && networkTypes.some(imeiWhitelist)) {
> > +      // Requesting the IMEI code works on GSM networks and certain CDMA ones
> 
> Should read // Requesting IMEI works on phones supporting a GSM network

Done.

> ::: apps/communications/dialer/test/unit/mmi_test.js
> @@ +62,5 @@
> > +
> > +      delete MockMozMobileConnection.voice.type;
> > +    });
> > +
> > +    test('Requesting the IMEI is allowed on some CDMA networks', function() {
> 
> Requesting the IMEI is allowed on phones supporting GSM networks

Done.

> @@ +72,5 @@
> > +      MockMozMobileConnection.voice.type = 'evdoa';
> > +      assert.isFalse(MmiManager.isMMI('*#06#'));
> > +
> > +      delete MockMozMobileConnection.voice.type;
> > +      delete MockMozMobileConnection.supportedNetworkTypes;
> 
> You can move those in a teardown(). Same for the test above.

Done.

Comment 34

[Anthony Ricaud (:rik)]

Comment on attachment 8382222 [details] [diff] [review]
[PATCH 1/2 v4] Never validate MMI codes in CDMA networks

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

Ship it! (on a green travis)
http://shipitsquirrel.github.io/images/ship%20it%20squirrel.png

Comment 35

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Anthony Ricaud (:rik) from comment #34)
> Review of attachment 8382222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ship it! (on a green travis)
> http://shipitsquirrel.github.io/images/ship%20it%20squirrel.png

Thanks! Squashed the patches into one and pushed to gaia/master bd4b8f0eba16b7f767a48ed33cef16d61b0a59fd

Comment 36

[Enpei]

Verified on CDMA build. It will dial GSM USSD code as a voice call, not send it as a message.

Wasabi
Gaia      22d48b62df7901ad45044f66e15e7d8943884a06
Gecko     e8b0013422e7cdc7d0d23d3ed99c47bd0fe88d72
BuildID   20140227094728
Version   30.0a1