978031 - [B2G][Dialer] Incorrect message after trying to use dialer in Airplane mode

Status: RESOLVED FIXED

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

Description

[[:AndreiH]]

Attachment: Screenshot.png

STR:

1. Go to Settings and enable Airplane mode.
2. Go to dialer and dial a number and press call.

Expected results:
A message should appear like: Airplane mode activated.

Actual results:
Current message that appears is: Unable to make a phone call now. (see screenshot)

Build:
Gaia      3a9fdec737cdfd41206a332970fea833ec4ca13a                         
Gecko     https://hg.mozilla.org/mozilla-central/rev/4cfb6c61b137          
BuildID   20140227160204                                                   
Version   30.0a1

Comment 1

[Jason Smith [:jsmith]]

QA blocker - dialer automation is blocked due to this.

Why isn't there in tree tests to catch a regression like this?

Comment 2

[Zac C (:zac)]

(In reply to Jason Smith [:jsmith] from comment #1)
> QA blocker - dialer automation is blocked due to this.
> 
> Why isn't there in tree tests to catch a regression like this?

You can't do this flow on desktopb2g. I'm not sure if a unittest could catch it. It probably needs the RIL.

Comment 3

[J Zimbrick]

Regression Window:

Last Working Environmental Variables:
Device: Buri
BuildID: 20140227054712
Gaia: 22d48b62df7901ad45044f66e15e7d8943884a06
Gecko: 0b930fe64a1e
Version: 30.0a1
Base Image: V1.2-device.cfg

First Broken Envrionmental Variables:
Device: Buri
BuildID: 20140227062009
Gaia: 3a9fdec737cdfd41206a332970fea833ec4ca13a
Gecko: f8e801b991bc
Version: 30.0a1
Base Image: V1.2-device.cfg

Last Working Gaia/First Broken Gecko: Issue DOES reproduce.
Gaia: 22d48b62df7901ad45044f66e15e7d8943884a06
Gecko: f8e801b991bc

First Broken Gaia/Last Working Gecko: Issue does NOT reproduce.
Gaia: 3a9fdec737cdfd41206a332970fea833ec4ca13a
Gecko: 0b930fe64a1e

Gecko Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b930fe64a1e&tochange=f8e801b991bc

Comment 4

[Jason Smith [:jsmith]]

Regression from bug 969218.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Note that could also be Bug 969280 as the new code was not used until bug 969218 landed.

Comment 6

[Ken Chang[:kenkai]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Note that could also be Bug 969280 as the new code was not used until bug
> 969218 landed.

Zac, I wonder if you could help to verify this bug with latest Gaia, again. Thanks.

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Hi rik,

Could you help do some analysis for this issue.
I quickly go through the modification in Bug 969280

+      if (promiseOrCall && promiseOrCall.then) {
+        promiseOrCall.then(function(call) {
+          installHandlers(call, emergencyOnly, oncall, onconnected,
+                          ondisconnected, onerror);
+        }).catch(function(errorName) {
+          displayMessage('UnableToCall');
+        });
       }

It seems that it always display 'UnableToCall' error when got the promise rejection. Based on the STR, it should received 'RadioNotAvailable' as the errorName. Gaia should show the specific error based on different errorName.

Comment 8

[Anthony Ricaud (:rik)]

(In reply to Jason Smith [:jsmith] from comment #1)
> QA blocker - dialer automation is blocked due to this.
How is that a QA blocker? From what I'm reading, it is displaying the wrong error message, not changing a flow. Maybe I'm not sure about the meaning of QA blocker.

> Why isn't there in tree tests to catch a regression like this?
We have tests, it was caught by some. We just don't have Travis/TBPL tests because we don't have an emulator to test anything related to radios. We have unit tests but I wrote it wrongly.

Comment 9

[Anthony Ricaud (:rik)]

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> It seems that it always display 'UnableToCall' error when got the promise
> rejection. Based on the STR, it should received 'RadioNotAvailable' as the
> errorName. Gaia should show the specific error based on different errorName.
Could you list all the error names that we can receive when we have a promise rejected?

In the version before promises, when we received a RadioNotAvailable error, it was an error event on an existing call. That's why I assumed we would still get a call in that case.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Anthony Ricaud (:rik) from comment #8)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > QA blocker - dialer automation is blocked due to this.
> How is that a QA blocker? From what I'm reading, it is displaying the wrong
> error message, not changing a flow. Maybe I'm not sure about the meaning of
> QA blocker.

Because it caused an automated UI test to be shut down that used to work with the Dialer in airplane mode.

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #9)
> (In reply to Szu-Yu Chen [:aknow] from comment #7)
> > It seems that it always display 'UnableToCall' error when got the promise
> > rejection. Based on the STR, it should received 'RadioNotAvailable' as the
> > errorName. Gaia should show the specific error based on different errorName.
> Could you list all the error names that we can receive when we have a
> promise rejected?

In promise version, the returned promise will only be resolved as a call if gecko successfully dial it out. Otherwise, it will be rejected. Possible errorName are:

"BadNumberError"
"RadioNotAvailable"
"GenericFailure"       => ex: modem dial fail
"InvalidAccessError"   => ex: serviceId param error
"InvalidStateError"    => ex: there is already a dialing call

> In the version before promises, when we received a RadioNotAvailable error,
> it was an error event on an existing call. That's why I assumed we would
> still get a call in that case.

Sorry that we didn't clearly describe the modification.

Comment 12

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

I suppose this work is on GAIA::Dialer. Feel free to correct me.

Comment 13

[Anthony Ricaud (:rik)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/16916

Szu-Yu: Could you confirm the unit tests part match the Gecko behaviour? https://github.com/Rik/gaia/blob/promise-error-978031/apps/communications/dialer/test/unit/telephony_helper_test.js#L368-L477

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8386270 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16916

Comment on github, I think we still need to trigger the error callback passed to the TelephonyHelper.

Comment 15

[Anthony Ricaud (:rik)]

Comment on attachment 8386270 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16916

Comment addressed.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #13)
> Created attachment 8386270 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/16916
> 
> Szu-Yu: Could you confirm the unit tests part match the Gecko behaviour?
> https://github.com/Rik/gaia/blob/promise-error-978031/apps/communications/
> dialer/test/unit/telephony_helper_test.js#L368-L477

In promise version, it is also possible to receive "BadNumberError" from call.onerror. Suggest to add an unit test for this scenario.

The case could be happened when the number format is correct and our phone successfully dial it out. Now we get a call object. However after it reach the network side. network think it doesn't match a existed destination. Then it might disconnect the call by "BadNumberError".

Except this, I think other cases are proper handled. Thank you.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8386270 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16916

See Comment 16.

Comment 18

[Etienne Segonzac (:etienne)]

Comment on attachment 8386270 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16916

Cool, r=me with Aknow's suggested test added.

Comment 19

[Anthony Ricaud (:rik)]

I copied the BadNumberError tests into the onerror tests and added some comments explaining why we could receive it both from Gecko and the network.

https://github.com/mozilla-b2g/gaia/commit/a351fe62c11737c722ad33aaff438f6ccd00bd4a