1008974 - 'No network connection' error message displayed when calling a contact from Contacts app

Status: RESOLVED FIXED

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

Description

[Viorela Ioia [:viorela]]

Attachment: screenshot.png

test_call_contact.py is failing in the latest master build because an error message is displayed when trying to make a call. 

Stacktrace:
Traceback (most recent call last):
  File "/home/viorelaioia/.virtualenvs/gaia/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette_test.py", line 163, in run
    testMethod()
  File "/home/viorelaioia/WebQA/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/test_call_contact.py", line 37, in test_call_contact
    call_screen.wait_for_outgoing_call()
  File "/home/viorelaioia/WebQA/gaia/tests/python/gaia-ui-tests/gaiatest/apps/phone/regions/call_screen.py", line 43, in wait_for_outgoing_call
    outgoing_call = self.marionette.find_element(*self._outgoing_call_locator)
  File "/home/viorelaioia/.virtualenvs/gaia/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette.py", line 1204, in find_element
    response = self._send_message('findElement', 'value', **kwargs)
  File "/home/viorelaioia/.virtualenvs/gaia/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/decorators.py", line 35, in _
    return func(*args, **kwargs)
  File "/home/viorelaioia/.virtualenvs/gaia/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette.py", line 624, in _send_message
    self._handle_error(response)
  File "/home/viorelaioia/.virtualenvs/gaia/local/lib/python2.7/site-packages/marionette_client-0.7.7-py2.7.egg/marionette/marionette.py", line 655, in _handle_error
    raise NoSuchElementException(message=message, status=status, stacktrace=stacktrace)
NoSuchElementException: NoSuchElementException: Unable to locate element: .handled-call.outgoing

#Env:
Gaia      c1a8cbaac1d921cfb50e3a2600720b75cf5afabd
Gecko     https://hg.mozilla.org/mozilla-central/rev/6f5597d4d3e3
BuildID   20140512040203
Version   32.0a1
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013

#STR from test:
1. Launch contacts app
2. Add a contact
3. Tap the contact you created to open the details page
4. Tap the phone number to call the contact

#Expected results:
A phone call is launched, call screen window is displayed

#Actual results: 
1."No network connection" error message is displayed on the screen (see screenshot attached)

I was able to reproduce the failure locally, by running the test. 
It fails intermittently. 
I was not able to reproduce it manually, as I tried several times. 

This is not reproducible on v1.4.

Comment 1

[Jason Smith [:jsmith]]

So are we classifying this as a test problem or a bug?

Comment 2

[Viorela Ioia [:viorela]]

This is happening in today's build too. I was able to reproduce the failure manually, by repeating steps 3 and 4 from STR really quick, several times. 
After that error message is displayed, I tapped ok and I went back to contact details page.  
Then I tried to call the contact again, but that button became unresponsive.
I'll attach a video of the issue I described.

Comment 3

[Viorela Ioia [:viorela]]

Attachment: VID_0003.3gp

Comment 4

[Jason Smith [:jsmith]]

Ok - that confirms this is a gaia bug & causing one of our tests to go down, which makes this a QA blocker.

Comment 5

[Jason Smith [:jsmith]]

Johan thinks this is a timing issue related to the test, so moving back to UI Tests.

Comment 6

[Johan Lorenzo [:jlorenzo]]

Attachment: Screenshot after entring tap_phone_number()

I reproduced the issue with the same build as Viorela. When I run the base test, it fails every time. When I add a time.sleep(0.1) just before tapping the phone number button, the test always pass (tried 10 times in a row).

I checked if we could wait for a transition or an icon beeing rendered but I don't think there is any graphical element on which we may wait for. See attached screenshots.

Comment 7

[Johan Lorenzo [:jlorenzo]]

Attachment: Screenshot just after wait_for_element_displayed(*self._call_phone_number_button_locator)

Comment 8

[Johan Lorenzo [:jlorenzo]]

Attachment: Screenshot just after find_element(*self._call_phone_number_button_locator).tap()

Comment 9

[Viorela Ioia [:viorela]]

This is failing in today's build too: http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.non-smoketest/354/HTML_Report/ 

Gaia      3a1d67246a79e3632c3b3f2460a25291e7e2714c
Gecko     https://hg.mozilla.org/mozilla-central/rev/e4843f4f08a7
BuildID   20140515040207
Version   32.0a1
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013


I tried to wait for several elements on the contact details page, but I still got the failure. The test failed even with time.sleep(0.1).
I'll still look for a suitable wait before proposing a time.sleep, which should be the last option.

Comment 10

[Johan Lorenzo [:jlorenzo]]

Regarding the screenshots taken, I don't think there is any element in the DOM which we can wait for. What do you think?

Comment 11

[Florin Strugariu [:Bebe]]

I can easily reproduce this issue manually on the latest Hamachi build

Gaia      7973e06dc278f67b4109ac3c33020ed086f0d042
Gecko     https://hg.mozilla.org/mozilla-central/rev/58c5a3427997
BuildID   20140515160202
Version   32.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013

Make sure you have a contact and just "double tap on it" one to open the contact details and one to tap the phone number on the next screen. (Make sure you don't have the phone app opened to reproduce the bug)

I see this as a "normal" use of the contacts app when you want to call someone you know what you need to do and do it in a hurry not waiting for the full screen to load.

I can reproduce this on my Keon too

What's more annoying is that you are prompted with a "No network connection" message.
and after you dismiss the message you still can't call the contact. Tapping on the phone number has no effect. 

As a workaround you have to close the contacts app and start the flow again.

See the video https://bugzilla.mozilla.org/attachment.cgi?id=8421756 

I'm adding  qawanted   here. Can someone else take a look over this.

Comment 12

[Jason Smith [:jsmith]]

This only affects 2.0, not 1.4, right?

comment 4 appears to be applicable again.

Comment 13

[Viorela Ioia [:viorela]]

I was able to reproduce the failure manually on v1.4 too.

Device: Hamachi
Gaia      32fca83da31b9a0f9a5a88f96c913a25accdc14b
Gecko     https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/a1e455367fa6
BuildID   20140516000201
Version   30.0
ro.build.version.incremental=eng.tclxa.20131223.163538
ro.build.date=Mon Dec 23 16:36:04 CST 2013

Comment 14

[Jason Smith [:jsmith]]

Can we get an analysis here of the reproduction rate on 1.4 vs. 2.0?

Comment 15

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #14)
> Can we get an analysis here of the reproduction rate on 1.4 vs. 2.0?

Zac - Can you help clarify this? We're seeing far more often on 2.0 instead of 1.4, right? 2.0's case is actually blocking automation, where as 1.4 isn't?

Comment 16

[Viorela Ioia [:viorela]]

Jsmith, I tested manually on both v1.4 and v2.0 and I noticed that the test is failing on v1.4 more often than it is failing on v2.0. It depends a lot on how quick can we tap the call button from contact details page. 
As for the automated tests, I guess that on v1.4 we just didn't had the luck to see it failing, as it is intermittent. 
I'll run the automated test on both v1.4 and v2.0 around 10 times and I'll let you know the results, so that you can have a more clear reproduction rate.

Comment 17

[Jason Smith [:jsmith]]

Ok - I'm going to operate under the belief on what we are seeing in our reports on 2.0, not 1.4, so I'm pushing for this as a test blocker for 2.0.

Comment 18

[Pi Wei Cheng [:piwei] (inactive)]

Able to repro this manually on today's Buri master build with the help tip from comment 11. Buri master channel will be used to find the regression window.

Comment 19

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Buri MOZ
BuildID: 20140321163215
Gaia: a970a22cceab5537dae680b9a172468602d901c7
Gecko: 3dbd42a3977d
Version: 31.0a1
Firmware Version: v1.2-device.cfg

First Broken Environmental Variables:
Device: Buri MOZ
BuildID: 20140321164315
Gaia: b2545bf6eb976704b54d82cfc06b24f31fedee6a
Gecko: 5251c32761cb
Version: 31.0a1
Firmware Version: v1.2-device.cfg

Last Working Gaia / First Broken Gecko: Issue Does NOT reproduce
Gaia: a970a22cceab5537dae680b9a172468602d901c7
Gecko: 5251c32761cb

Last Working Gecko / First Broken Gaia: Issue DOES reproduce
Gaia: b2545bf6eb976704b54d82cfc06b24f31fedee6a
Gecko: 3dbd42a3977d

Gaia Pushlog:
https://github.com/mozilla-b2g/gaia/compare/a970a22cceab5537dae680b9a172468602d901c7...b2545bf6eb976704b54d82cfc06b24f31fedee6a

Comment 20

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #17)
> Ok - I'm going to operate under the belief on what we are seeing in our
> reports on 2.0, not 1.4, so I'm pushing for this as a test blocker for 2.0.

Or maybe not. The range points to a DSDS change in bug 980982, so we'll need this on 1.4 as well then.

Comment 21

[Jason Smith [:jsmith]]

Doug - Can you take a look? The range is pointing to bug 980982 as the cause.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #17)
> > Ok - I'm going to operate under the belief on what we are seeing in our
> > reports on 2.0, not 1.4, so I'm pushing for this as a test blocker for 2.0.
> 
> Or maybe not. The range points to a DSDS change in bug 980982, so we'll need
> this on 1.4 as well then.

Actually re-reading the above comments indicates that the automation failure is only present on 2.0, even though 1.4 does have the bug (but automation does not seem to hit it).

Comment 23

[Doug Sherk (:drs) (inactive)]

Attachment: Ignore (long) taps on MSAB until settings have been loaded.

The problem was that we had a race condition on a setting being loaded but we didn't guard against it not being loaded yet in the click/contextmenu event handlers.

PR: https://github.com/mozilla-b2g/gaia/pull/19443

Comment 24

[Doug Sherk (:drs) (inactive)]

This isn't very risky, BTW. I think we should uplift it to 1.4. I'll ask for approval once it gets through review.

Comment 25

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

triage: regression

Comment 26

[Anthony Ricaud (:rik)]

Comment on attachment 8425489 [details] [diff] [review]
Ignore (long) taps on MSAB until settings have been loaded.

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

We shouldn't ignore user inputs, this is disappointing from a UX point of view. I'd rather make sure that _getCardIndex always returns a valid cardIndex.

What do you think about making _getCardIndex a promise and inside do:
LazyLoader.load(['/shared/js/settings_listener.js'], function() {
  promise.resolve(this._defaultCardIndex);
}

Comment 27

[Doug Sherk (:drs) (inactive)]

Attachment: Ignore (long) taps on MSAB until settings have been loaded.

(In reply to Anthony Ricaud (:rik) from comment #26)
> Comment on attachment 8425489 [details] [diff] [review]
> Ignore (long) taps on MSAB until settings have been loaded.
> 
> Review of attachment 8425489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We shouldn't ignore user inputs, this is disappointing from a UX point of
> view. I'd rather make sure that _getCardIndex always returns a valid
> cardIndex.
> 
> What do you think about making _getCardIndex a promise and inside do:
> LazyLoader.load(['/shared/js/settings_listener.js'], function() {
>   promise.resolve(this._defaultCardIndex);
> }

Thanks for the suggestion. Supporting promises here would require significant rearchitecting, so I took a poor man's approach to this where I just store whether or not a click has been queued up. You can see it in the updated PR as a new commit and as part of this patch.

Comment 28

[Anthony Ricaud (:rik)]

Attachment: Alternate promise version

The poor man's approach was making me uncomfortable because it was not handling all cases. So I tried doing a promise version and it seems to work.

What do you think?

Comment 29

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #28)
> Created attachment 8429389 [details] [review]
> Alternate promise version
> 
> The poor man's approach was making me uncomfortable because it was not
> handling all cases. So I tried doing a promise version and it seems to work.
> 
> What do you think?

I might be missing something so I'd like to hear your thoughts on why this was making you uncomfortable. But on the surface, I don't think this was either worth the time you put into this or necessary. This also adds a great amount of complexity to the code. The cases I can see happening here are, an app using an MSAB is loading a view containing an MSAB and:

a) the setting hasn't been loaded yet but will finish at some point, and the user is tapping on it, or
b) the setting being loaded gets stalled (i.e. never completes) and the user is tapping/long pressing on it, or
c) the setting hasn't been loaded yet but will finish at some point, and the user is long pressing on it

I don't see a) or b) as really being significantly different in either implementation. In the case of b), if the setting stalls, then neither one will do a better job than the other. The only case I can see being different is c), but I made a decision that I would understand if you disagreed with to not try to defer long taps because I thought that they would so rarely occur before settings are loaded.

And here are what I see as the benefits of using promises for this:

1) every tap/long press gets captured and handled once the promise is resolved
2) we're less tied to an internal implementation detail since promises abstract away most of the mechanics for us

I don't think 1) applies here. It's not really useful to know if the user pressed the call button twice as opposed to once, and it would likely lead to unexpected behavior such as loading the most recently dialed phone number and calling it if the user taps the button repeatedly. Maybe you would see this as desirable?

Just a note that I'm not defending my patch. :) I just want to do what is actually best in the long run. So maybe you can explain why you disagree.

Comment 30

[Anthony Ricaud (:rik)]

It was making me uncomfortable because I'd like _getCardIndex always returning something correct. It feels weird that a getter might return undefined. I prefer my version in that respect but I don't like how ugly the unit tests look like with my patch.

On the end result, I agree that both patches cover the useful use cases that a user will encounter.

Let's bring Etienne in to see what he thinks.

Comment 31

[Etienne Segonzac (:etienne)]

Maybe my opinion will change with time since they are pretty new to me. But, mainly for testing reasons, I still find it awkward to use promises partially/internally when you're not working on code that would naturally expose methods returning promises. Hence the _getCardIndex access in the tests of the promise-based patch...

So, right now and given the 1.4+-ness, I vote for Doug's simpler patch.

I agree that |_getCardIndex| being racy is weird, thankfully we do not expose it. 
Maybe we can rename it to something like |_getSettingValueIfLoaded|?

Comment 32

[Anthony Ricaud (:rik)]

Comment on attachment 8428468 [details] [diff] [review]
Ignore (long) taps on MSAB until settings have been loaded.

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

I agree with Etienne's proposition, let's go with the simplest patch. I still have a few comments to make this easier to understand but r+ otherwise.

::: apps/communications/dialer/test/unit/multi_sim_action_button_test.js
@@ +165,5 @@
> +
> +        this.sinon.spy(MockSimPicker, 'getOrPick');
> +      });
> +
> +      test('should ignore taps until settings are loaded', function() {

'should queue one tap until settings are loaded'

::: shared/js/multi_sim_action_button.js
@@ +45,3 @@
>  };
>  
>  MultiSimActionButton.prototype._getCardIndex = function() {

Let's put a big comment here stating that this could return undefined.

Also, I like Etienne's proposition to rename. _getCardIndexIfLoaded looks good to me.

@@ +122,5 @@
>        phoneNumber === '' ||
>        event.target.disabled ||
>        (window.TelephonyHelper &&
> +       window.TelephonyHelper.getInUseSim() !== null) ||
> +      this._getCardIndex() === undefined) {

Let's put this in another if statement, explaining that we expect the setting to be loaded by then but just in case, we bail out.

Comment 33

[Anthony Ricaud (:rik)]

Comment on attachment 8428468 [details] [diff] [review]
Ignore (long) taps on MSAB until settings have been loaded.

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

I agree with Etienne's proposition, let's go with the simplest patch. I still have a few comments to make this easier to understand but r+ otherwise.

::: apps/communications/dialer/test/unit/multi_sim_action_button_test.js
@@ +165,5 @@
> +
> +        this.sinon.spy(MockSimPicker, 'getOrPick');
> +      });
> +
> +      test('should ignore taps until settings are loaded', function() {

'should queue one tap until settings are loaded'

::: shared/js/multi_sim_action_button.js
@@ +45,3 @@
>  };
>  
>  MultiSimActionButton.prototype._getCardIndex = function() {

Let's put a big comment here stating that this could return undefined.

Also, I like Etienne's proposition to rename. _getCardIndexIfLoaded looks good to me.

@@ +122,5 @@
>        phoneNumber === '' ||
>        event.target.disabled ||
>        (window.TelephonyHelper &&
> +       window.TelephonyHelper.getInUseSim() !== null) ||
> +      this._getCardIndex() === undefined) {

Let's put this in another if statement, explaining that we expect the setting to be loaded by then but just in case, we bail out.

Comment 34

[Doug Sherk (:drs) (inactive)]

Attachment: Queue taps and ignore long presses on MSAB until settings have been loaded.

Thanks, I addressed your review comments and I'm carrying r+. I'll land when green.

Comment 35

[Doug Sherk (:drs) (inactive)]

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

Comment 36

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8432953 [details] [diff] [review]
Queue taps and ignore long presses on MSAB until settings have been loaded.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Probably something like bug 946866
[User impact] if declined: Users will get a 'no network connection' message if they try to place a call very quickly after a multi SIM button has been loaded (approx. 1 second after a view loads).
[Testing completed]: Rik and I tested this.
[Risk to taking this patch] (and alternatives if risky): Low. Though this should be fixed in conjunction with bug 1005823.
[String changes made]: None.

Comment 37

[Preeti Raghunath(:Preeti)]

Comment on attachment 8432953 [details] [diff] [review]
Queue taps and ignore long presses on MSAB until settings have been loaded.

Taking in 1.4

Comment 38

[Doug Sherk (:drs) (inactive)]

v1.4: https://github.com/mozilla-b2g/gaia/commit/0c16adced7c51f795ef250aebe184f60b6a9b987