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.

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

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

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

Can we check to see if this happens on 1.3?

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

Can we get a video of the bug in English?

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

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?

(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.

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

(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.

(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.

moving back to 2.0?

Moving this to backlog for 1.4

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

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.

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

(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).

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 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 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.

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

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/

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.

Try was green except for an unrelated intermittent: https://tbpl.mozilla.org/?rev=14f5810c2142d3b98c2c45c3279b1c8e1ed53fdf&tree=Gaia-Try Pushed to gaia/master a142e47584d2a38f37cb5a02e937d006677e1c3e

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.

(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.

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

(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.

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 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

: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.

[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 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.

2.0+ for this very small but user visible change.

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.

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