Closed Bug 958733 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
1.4 S2 (28feb)

People

(Reporter: mschwart, Assigned: gsvelto)

References

Details

Attachments

(3 files, 3 obsolete files)

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
blocking-b2g: --- → 1.4?
Depends on: 889737
Blocks: b2g-ril-cdma
Hsinyi, are we going to move the decision rule of supplementary service to Gaia?
Flags: needinfo?(htsai)
(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.
Flags: needinfo?(htsai)
Blocks: 960372
Gabriele, is that something you could do?
Flags: needinfo?(gsvelto)
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.
Flags: needinfo?(jcheng)
(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
Flags: needinfo?(jcheng)
blocking-b2g: 1.4? → 1.4+
(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.
Flags: needinfo?(gsvelto)
features should not block. remove blocking flag
blocking-b2g: 1.4+ → ---
(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.
hi Gabriele, do you mind taking the bug? if you will be working on this once the depending bug is fixed. thanks
Flags: needinfo?(gsvelto)
(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 :)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto)
Flags: in-moztrap?(echu)
Target Milestone: --- → 1.4 S2 (28feb)
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?
(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 :)
(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?
(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!
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.
Attachment #8380732 - Flags: feedback?(htsai)
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;
}
Attachment #8380732 - Flags: feedback?(htsai) → feedback-
Blocks: 976427
No longer depends on: 889737
Depends on: 969218
(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.
Updated patch with the list of CDMA networks baked in.
Attachment #8380732 - Attachment is obsolete: true
Attachment #8381229 - Flags: review?(anthony)
Attachment #8381229 - Flags: feedback?(htsai)
... and jshint cleanup of the affected files for good measure.
Attachment #8381230 - Flags: review?(anthony)
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!
Attachment #8381229 - Flags: feedback?(htsai) → feedback+
Attachment #8381230 - Flags: review?(anthony) → review+
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.
Attachment #8381229 - Flags: review?(anthony) → review+
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 ;) ).
Flags: needinfo?(htsai)
Is this bug in 'checkin-needed' stage? 
Can we land the fix because we already got review+? :-)
Flags: needinfo?(gsvelto)
(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 ;) ).
Flags: needinfo?(htsai)
Test case: https://moztrap.mozilla.org/manage/case/11710/
Flags: in-moztrap?(echu) → in-moztrap+
(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?!
Flags: in-moztrap+ → in-moztrap?(echu)
Flags: in-moztrap?(echu) → in-moztrap+
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.
Flags: needinfo?(gsvelto)
(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. :)
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.
Attachment #8381229 - Attachment is obsolete: true
Attachment #8382027 - Flags: review?(anthony)
(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 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.
Attachment #8382027 - Flags: review?(anthony)
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.
Attachment #8382027 - Attachment is obsolete: true
Attachment #8382222 - Flags: review?(anthony)
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
Attachment #8382222 - Flags: review?(anthony) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.