1006592 - While sending USSD code, last dialed number by user appears in dialer app

Status: VERIFIED FIXED

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

Description

[nulti-korisnik]

Attachment: 2014-05-06-09-30-03.png

User Agent: Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140421221237

Steps to reproduce:

I noticed this in latest v1.4 for keon device:

Build Identifier: 20140505185236
Gaia git: fccb15d6

How to produce it:

1. Dial any number;
2. After dialing it, try to send USSD code;
3. While sending USSD code, you should be able to see last dialed number in phone app.


Actual results:

The last dialed number was in phone app


Expected results:

Phone application should just send USSD code without exposing last dialed number by user.

Comment 1

[Gabriele Svelto [:gsvelto]]

Confirmed, I've been experiencing this today while working on MMI code, this looks like a regression.

Comment 2

[Jason Smith [:jsmith]]

QA Wanted to check if we can reproduce this on a production device.

Comment 3

[Cody Roesch [:croesch]]

Verified the bug exists in V1.4 Buri.

After dialing a phone number, I then dialed *#07# and the previous phone number was displayed.

Environmental Variables
Device: Buri V1.4 Moz Ril
Build ID: 20140513000208
Gecko: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c140bcd02b9b
Gaia: b40103dec34a147c9018a1af76eb21c3184f2f93
Platform Version: 30.0
Firmware Version: v1.2-device.cfg

Comment 4

[Jason Smith [:jsmith]]

Can we check to see if this happens on 1.3?

Comment 5

[Cody Roesch [:croesch]]

This bug does NOT happen on Buri V1.3. No telephone number was displayed.

Environmental Variables
Device: Buri V1.3
Build ID: 20140513024003
Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/328611bdebc6
Gaia: 26a7a59d219fc8753613b433844123e428adcd56
Platform Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 6

[Jason Smith [:jsmith]]

Can we get a video of the bug in English?

Comment 7

[Cody Roesch [:croesch]]

Here is the video link for the bug.
http://youtu.be/vSG6HdT0kug

Comment 8

[Jason Smith [:jsmith]]

This looks like a minor regression to me, but I want to double check if something like this would be a certification issue or not.

Beatriz - Would an issue like this be a certification blocker or not?

Comment 9

[Beatriz Rodríguez [:brg]]

(In reply to Jason Smith [:jsmith] from comment #8)
> This looks like a minor regression to me, but I want to double check if
> something like this would be a certification issue or not.
> 
> Beatriz - Would an issue like this be a certification blocker or not?
This is not a blocker for us. The blocker isssue would be if the USSD code is included in the call log but from the description of the bug it does not seem to be the issue.

Comment 10

[Anshul]

I am seeing the same issue on 2.0. Since this seems like a regression as per comment #5 I am requesting 2.0?

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Anshul from comment #10)
> I am seeing the same issue on 2.0. Since this seems like a regression as per
> comment #5 I am requesting 2.0?

Since this is broken on 1.4 we I pushing the nomination to that release.

Comment 12

[Anshul]

(In reply to bhavana bajaj [:bajaj] [NOT reading Bugmail, needInfo please] from comment #11)
> (In reply to Anshul from comment #10)
> > I am seeing the same issue on 2.0. Since this seems like a regression as per
> > comment #5 I am requesting 2.0?
> 
> Since this is broken on 1.4 we I pushing the nomination to that release.
Bhavana, the bug is not that critical to warrant a fix in 1.4 in my opinion; trying to minimize the changes to 1.4.

Comment 13

[Preeti Raghunath(:Preeti)]

moving back to 2.0?

Comment 14

[Preeti Raghunath(:Preeti)]

Moving this to backlog for 1.4

Comment 15

[Gabriele Svelto [:gsvelto]]

I just verified that this happens on 2.0 and master too. This was tested on a Hamachi device.

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

The issue here is caused by both the MultiSimActionButton and KeypadManager registering event handlers on the same button (keypad-callbar-call-action) see:

https://github.com/mozilla-b2g/gaia/blob/2c2ced34ebafb23d7f0d3bec1a304911ac6fad40/shared/js/dialer/keypad.js#L192
https://github.com/mozilla-b2g/gaia/blob/2c2ced34ebafb23d7f0d3bec1a304911ac6fad40/shared/js/dialer/keypad.js#L205

When the user taps the button both event handlers are triggered. In the common call case due to a timing issue the value of |this._phoneNumber| is not cleared before we hit the second handler and thus it's skipped:

https://github.com/mozilla-b2g/gaia/blob/2c2ced34ebafb23d7f0d3bec1a304911ac6fad40/shared/js/dialer/keypad.js#L303

When using the MMI code path on the other hand the |this._phoneNumber| is cleared before the second handler gets invoked and thus it goes ahead and fetches/displays the last called number.

This patch stops all propagation for the event delivered to the MultiSimActionButton if we're starting any dial action. This prevents the second event handler from triggering in a more robust and non-racy way.

A couple of notes regarding tests: this currently breaks the shared tests for the MultiSimActionButton but that's because those tests are registering multiple event handlers on the same button (grrr....) so I'll need to rewrite them to instance only one button and to register only one event handler per test. The second issue is that this won't be racy anymore but will depend on the order in which event handlers are registered so I'll add a test that verifies the event handler order of registration to prevent regressions.

Comment 17

[Anthony Ricaud (:rik)]

Attachment: Alternative approach

What about just registering the fetchLastCalled before the MultiSimAction? That makes testing easier I believe.

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Anthony Ricaud (:rik) from comment #17)
> What about just registering the fetchLastCalled before the MultiSimAction?
> That makes testing easier I believe.

Yes this would make the testing a bit easier but I'm not very fond of the approach because it still relies on a specific order of events for everything to work correctly. In the scenario where there's no phone number typed in the click will trigger the two event handlers with the fetchLastCalled one coming first. This one will launch the DB query which will later return the last called number; then the MultiSimAction event handler will be executed and finally we'll get the result back from the DB and populate the screen.

The issue here is that while it's unlikely to happen there's no guarantee that the MultiSimAction event handler will be executed before the DB callback returns. In fact if for some reason we'll make CallLogDBManager.getGroupAtPosition() become synchronous at some stage it would cause breakage as the MultiSimAction button would find itself a populated number instead of no number and then start a call right away.

So all in all I'm more keen of using stopImmediatePropagation() as that guarantees that the second event-handler won't be called no matter what the order of events is (except for the event callers which are executed in order of registration but that's standard behavior so it's always guaranteed).

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

Here's the complete patch with fixes for the unit-tests and additional coverage. I had to modify the initSubject() function in the MultiSimActionButton tests to re-create all the components whenever it was called. Previously this would re-use the same button and register multiple event listeners to it causing some... interesting behavior once I started invoking stopImmediatePropagation().

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

Comment 21

[Anthony Ricaud (:rik)]

Comment on attachment 8460284 [details] [diff] [review]
[PATCH] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

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

Redirecting the review to Doug.

::: shared/js/dialer/keypad.js
@@ -300,5 @@
>    },
>  
>    fetchLastCalled: function hk_fetchLastCalled() {
> -    if (this._phoneNumber !== '') {
> -      return;

I still think we should keep this. That would make it not dependant of the handlers order.

Comment 22

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8460284 [details] [diff] [review]
[PATCH] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

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

Thanks for the cleanups you did here. I'm actually going to ask for a few more small ones since this isn't a serious regression or a blocker. I do prefer this approach over Anthony's just because it's less fragile.

Please move multi_sim_action_button_test.js to apps/sharedtest/test/unit/ while you're here.

::: apps/communications/dialer/test/unit/keypad_test.js
@@ +611,5 @@
>      });
>    });
>  
>    suite('Initializing MultiSimActionButton', function() {
> +    var callBarCallAction;

I'd prefer calling this callBarCallActionElt or something like that since it's a DOM element.

@@ +632,5 @@
>        assert.equal(subject._phoneNumber,
>          MockMultiSimActionButtonSingleton._phoneNumberGetter());
>      });
> +
> +    test('Should add the first click event handler to the button', function() {

Thanks for adding this!

@@ +635,5 @@
> +
> +    test('Should add the first click event handler to the button', function() {
> +      assert.equal(addEventListenerSpy.firstCall.args[0], 'click');
> +      assert.equal(addEventListenerSpy.firstCall.args[1],
> +        MockMultiSimActionButtonSingleton._click);

Please line up the parameters here.

::: apps/communications/dialer/test/unit/multi_sim_action_button_test.js
@@ +36,5 @@
>  
>    mocksHelperForMultiSimActionButton.attachTestHelpers();
>  
> +  var initSubject = function(options) {
> +    options = options ? options : {};

Instead of:
foo = options.bar ? options.bar : default;

Use:
foo = options.bar || default;

For this and each of the ones below.

@@ +39,5 @@
> +  var initSubject = function(options) {
> +    options = options ? options : {};
> +    var callback = options.callback ? options.callback : function() {};
> +    var noSettingsTrigger =
> +      options.noSettingsTrigger ? options.noSettingsTrigger : false;

This name confused me until I read where it's used. Maybe "noTriggerSettingsCallback" instead would be better.

@@ +83,5 @@
>    };
>  
>    var shouldUsePrimarySimCard = function() {
>      var callStub = this.sinon.stub();
> +    initSubject({ callback: callStub });

This is bad naming, please fix it while you're here. s/callStub/callbackStub/

@@ +169,5 @@
>        navigator.mozIccManager.addIcc(1, {});
>      });
>  
>      suite('settings loading race conditions', function() {
> +      var callStub;

MSAB isn't used only for calls, so I actually prefer the old name in this case.

@@ +260,5 @@
>          sinon.assert.notCalled(showSpy);
>        });
>  
>        test('should return current serviceId of call', function() {
>          var callStub = this.sinon.stub();

Please fix this as well while you're here.

@@ -396,5 @@
>               function() {
>            MockTelephonyHelper.mInUseSim = 1;
>            MockNavigatorMozTelephony.mTriggerEvent({type: 'callschanged'});
> -
> -          var setAttributesSpy = this.sinon.spy(MockMozL10n, 'setAttributes');

Removing this makes it not actually test the same thing. You'll have to reset the called count within sinon here.

::: shared/js/dialer/keypad.js
@@ -300,5 @@
>    },
>  
>    fetchLastCalled: function hk_fetchLastCalled() {
> -    if (this._phoneNumber !== '') {
> -      return;

I agree.

::: shared/js/multi_sim_action_button.js
@@ +67,5 @@
>      return;
>    }
>  
> +  if (event) {
> +    // Prevent the call action button handler from triggering.

s/call action button handler/KeypadManager handler/

I don't think there's anything else that registers this event on this button, so we'd might as well be specific.

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

New patch view review comments addressed, the PR was also updated.

Comment 24

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8460912 [details] [diff] [review]
[PATCH v2] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

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

Just a couple of nits then this is good. Thanks!

::: apps/sharedtest/test/unit/multi_sim_action_button_test.js
@@ +42,5 @@
> +    var noSettingsTriggerCallback = options.noSettingsTriggerCallback || false;
> +
> +    MockTelephonyHelper.mInUseSim = setCardIndex;
> +
> +    button = options.button ? options.button : document.createElement('button');

Switch to || here.

@@ +168,5 @@
> +      navigator.mozIccManager.addIcc(1, {});
> +    });
> +
> +    suite('settings loading race conditions', function() {
> +      var callStub;

s/callStub/callbackStub/

Comment 25

[Gabriele Svelto [:gsvelto]]

Thanks for the review, I've updated my PR with your last nits addressed. I'll wait for Try to turn green before pushing to master.

Comment 26

[Gabriele Svelto [:gsvelto]]

Try was green except for an unrelated intermittent:

https://tbpl.mozilla.org/?rev=14f5810c2142d3b98c2c45c3279b1c8e1ed53fdf&tree=Gaia-Try

Pushed to gaia/master a142e47584d2a38f37cb5a02e937d006677e1c3e

Comment 27

[Juanjo Iglesias]

Testing today with Flame master (Gecko 692e97f Gaia f8755a2) is working fine.
Whenever I type a wrong USSD string, the dialer is empty and no other string re-appear in the dialer.

Comment 28

[Gabriele Svelto [:gsvelto]]

(In reply to Juanjo Iglesias from comment #27)
> Testing today with Flame master (Gecko 692e97f Gaia f8755a2) is working fine.
> Whenever I type a wrong USSD string, the dialer is empty and no other string
> re-appear in the dialer.

Excellent, thanks for checking.

Comment 29

[Beatriz Rodríguez [:brg]]

Is there any chance to include this patch in 2.0 branch?

Comment 30

[Gabriele Svelto [:gsvelto]]

(In reply to Beatriz Rodríguez [:brg] from comment #29)
> Is there any chance to include this patch in 2.0 branch?

This would need approval, we can try since it affects the user experience.

Comment 31

[Beatriz Rodríguez [:brg]]

This issue was a regression of dialer with a bad impact of performance for the user.
Gabriele, as you know the patch, could you please ask for approval?

Comment 32

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8460912 [details] [diff] [review]
[PATCH v2] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

Asking for approval since this change is very small but fixes a regression, also see comment 31.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 947140
[User impact] if declined: When calling a USSD code the keypad screen will display the last called number instead of being blank at the end of the interaction 
[Testing completed]: Tested on both single-SIM and multi-SIM devices and covered with unit-tests
[Risk to taking this patch] (and alternatives if risky): Practically none, the patch is 5 lines worth of changes that prevent the event from being sent to the wrong element once it's been processed plus 800+ lines of adjustments to make the unit-tests more robust and capable of catching this issue.
[String changes made]: None

Comment 33

[bhavana bajaj [:bajaj]]

:brg, is this a cert blocker on your side ? At this time we do not want to take anything other than blocker issues to 2.0 given the remaining time to release 2.0 and keeping in mind we do not want to take any risk by landing non-blocker patches. If you think this could be a cert blocker, please nominate it with 2.0? with the reason and we can help make a blocking decision.

Comment 34

[Beatriz Rodríguez [:brg]]

[Blocking Requested - why for this release]: This issue will be easy to repro during certification.

In comment 9 I said it wont be a blocker but after further testing of MMI codes and USSD, we had faced that this is very easy to reproduce and very weird user impact who would not understand why dialer is showing a number that he does not type.

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8460912 [details] [diff] [review]
[PATCH v2] Prevent the click handler that fetches the last called number from being executed if we've already started a dial action

Clearing the approval request in favour of working with the blocking nom.

Comment 36

[Lawrence Mandel [:lmandel] (use needinfo)]

2.0+ for this very small but user visible change.

Comment 37

[Gabriele Svelto [:gsvelto]]

Cherry-picked and pushed to gaia/v2.0 0be2332b5591ec0d40390705520c3c1cc30051ef

There was only a small change required in the unit-tests and the rest applied cleanly.

Comment 38

[Coler]

This issue has been verified successfully on Flame2.0&2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_USSDcode.mp4

Flame2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141201000201
Version         32.0

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0

Comment 39

[Coler]

Attachment: Verify_Flame_USSDcode.MP4