Closed Bug 1007948 Opened 6 years ago Closed 6 years ago

[SIM PIN] Error message disappears when receiving iccinfochange

Categories

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

x86
Linux
defect
Not set

Tracking

(blocking-b2g:2.1+, b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: pgravel, Assigned: apastor)

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(3 files, 1 obsolete file)

Attached image simpin.png
Precondition : PIN1 Enable.
1. Boot the device.
2. Enter wrong PIN code when the PIN UI displayed.
3. Check the information message about entering the wrong PIN code.
4. If a IccInfoChanged message occurs, the information message immediately disappear.

See attached screenshot for event flow.

Reproducibility: low (timing based)

If a IccInfoChanged message happens after entering an invalid PIN, the error message simply disappears.

Can be made to occur 100% of the time if the code is modified to always send an IccInfoChanged Message right after a CardLockResult message, which causes the error message to never be visible to the user.
Requesting a 1.4 blocking flag as this is reported by the partner for 1.4.
blocking-b2g: --- → 1.4?
Anshul,

Please help with understanding if this is a regression from 1.3?

Is the DUT KK based?
Flags: needinfo?(anshulj)
Preeti, I am not sure if this is a regression from 1.3 (Phil can try to reproduce this on 1.3 and confirm tomorrow). In any case this clearly is an incorrect behavior and not really a new feature and is an issue raised by the partner.
Flags: needinfo?(anshulj)
This is not a regression, same issue is reproducible in 1.3.
Whiteboard: [systemsfe]
mvines - If this is already present on 1.3, why is this specifically needed on 1.4? What's the partner need?
Flags: needinfo?(mvines)
Might need to discuss this off bug, please sync with Anshul on the details
Flags: needinfo?(mvines)
(In reply to Jason Smith [:jsmith] from comment #5)
> mvines - If this is already present on 1.3, why is this specifically needed
> on 1.4? What's the partner need?
Jason, this issue is raised by our partner on 1.4 and therefore we marked it 1.4 blocking even though this bug existed since 1.3. This issue happens only under certain circumstances so I am fine moving it to the next release as long as the partners are okay; I am going to check with them and get back to you.
Peter,

Please review if this is a blocker.
Flags: needinfo?(pdolanjski)
(In reply to Preeti Raghunath(:Preeti) from comment #8)
> Peter,
> 
> Please review if this is a blocker.

FWIW - from a triage perspective, we didn't think this was a blocker, as the impact was minor & likelihood of reproduction was very unlikely.
blocking-b2g: 1.4? → ---
Flags: needinfo?(pdolanjski)
You can reproduce it easily doing:
1. enter a wrong pin code and get the error message
2. lock the phone
3. unlock the phone
4. see that the error message has disappeared.

That's because SimPinDialog.handleCardState() is called after 'will-unlock' events, and it resets the visibility of error message. We should instead hide error message only when the SIM card is unlocked successfully.

Let me work on a fix.
Assignee: nobody → gmarty
Might be a QCRIL-only triggered issue, as documented on bug 998918.
Duplicate of this bug: 998918
Attached file Github PR (obsolete) —
Attachment #8422419 - Flags: review?(timdream)
Tim, can you review this PR? Looks very trivial but there might be edge cases I'm not aware of.
Thanks!
Comment on attachment 8422419 [details] [review]
Github PR

I don't understand this well enough to tell if there isn't any side effect, if we remove this code. I think :jaoo might have.
Attachment #8422419 - Flags: review?(timdream) → review?(josea.olivera)
As I already mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=998918#c18 this issue happens only with QCRIL. QCRIL sends "iccinfochanged" message after a failed PIN unlock attempt, which I feel it should not.
(In reply to Sharaf from comment #16)
> As I already mentioned in
> https://bugzilla.mozilla.org/show_bug.cgi?id=998918#c18 this issue happens
> only with QCRIL. QCRIL sends "iccinfochanged" message after a failed PIN
> unlock attempt, which I feel it should not.
Just to be clear, this issue was discovered with QC RIL but the root cause of the issue is in Gaia the code that is common to both the RILs.
Ok, confused because this is marked as duplicate of Bug 998918, which is a QC RIL specific issue.

Comment #10 seems more common case and 100% reproducible.

Requesting for 1.4?
blocking-b2g: --- → 1.4?
Backlog for taking in2.0
blocking-b2g: 1.4? → backlog
Hey Jose, can you have a look at that PR when you have a minute?
It's just a one-liner, but don't know the app well enough to tell if it can have side effect. Thanks.
Flags: needinfo?(josea.olivera)
Comment on attachment 8422419 [details] [review]
Github PR

After having a look at this issue I've figured out that the dialog is shown when
an 'iccinfochange' event is fired because of the listener added in bug
965765. So I guess the issue happens on v1.3 as well.

I don't see any negative effect or risk taking this patch. The error message
won't be hidden so it would be visible when the dialog is shown (which happens
because the ICC card is still PIN bloked).

I'm giving you feedback+ to this patch as I'm not a peer of the system app. It
would be great to have a double check form Alive and the decision on taking this
patch/or propose some changes (if any).

pgravel, did you give a try taking the patch? Thanks.
Attachment #8422419 - Flags: review?(josea.olivera)
Attachment #8422419 - Flags: review?(alive)
Attachment #8422419 - Flags: feedback+
Flags: needinfo?(josea.olivera)
Will we run into a case that the error is fixed but we still show the error message so it confuses the user?
I carefully checked the code for such case and AFAIK this should never happen. But once again I'm not familiar with this code.
Comment on attachment 8422419 [details] [review]
Github PR

Even this fixes the problem but the best solution to me is not showing the dialog again when it's already shown when you get the iccinfochange.

Could you make this change?
Attachment #8422419 - Flags: review?(alive)
Comment on attachment 8422419 [details] [review]
Github PR

I updated the PR to create a dedicated method to display an error message, `showErrorMsg()`, that's the only place where the error message is displayed.

The `clear()` method is in charge of resetting the UI, including hiding the error message.

Alive, what do you think?
Attachment #8422419 - Flags: feedback+ → review?(alive)
(In reply to gmarty from comment #25)
> Comment on attachment 8422419 [details] [review]
> Github PR
> 
> I updated the PR to create a dedicated method to display an error message,
> `showErrorMsg()`, that's the only place where the error message is displayed.
> 
> The `clear()` method is in charge of resetting the UI, including hiding the
> error message.
> 
> Alive, what do you think?

Could you tell me why comment 24 doesn't work for you?
Flags: needinfo?(gmarty)
Attached file Github PR
Sorry, I misunderstood you.

This new PR makes sure that SimPinDialog.show() is not called if already opened.
It also fixes a bug with the number of tries left is set but not displayed (div with id="triesLeft").
Attachment #8422419 - Attachment is obsolete: true
Attachment #8422419 - Flags: review?(alive)
Attachment #8441404 - Flags: review?(alive)
Flags: needinfo?(gmarty)
Comment on attachment 8441404 [details] [review]
Github PR

r+ with nit, thanks.
Attachment #8441404 - Flags: review?(alive) → review+
Target Milestone: --- → 2.0 S4 (20june)
Merged in:
https://github.com/mozilla-b2g/gaia/commit/8ccac0744848bd3ceaa6aa1c972ae0eb1e9ddc1e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
[Blocking Requested - why for this release]:

Do we want to block 2.1 for this, since it is technically a regression from 2.0 (ie. we didn't do the backout there, and will have to live with bug 1059837)?
blocking-b2g: backlog → 2.1?
Keywords: regression
Triage group marking blocking because regression.
blocking-b2g: 2.1? → 2.1+
Guillaume, can you help investigate other solutions to this problem that won't cause bug 1059837?
Flags: needinfo?(gmarty)
I spent most of the afternoon yesterday looking into it.
The issue here is that we don't maintain a state in simcard_dialog.js about which SIM card is currently being asked for unlocking. We should refactor the logic to add real multi SIM support. I tried several fixes but there is always side effects somehow.
I think somebody else familiar with the code should take a look.
Flags: needinfo?(gmarty)
(In reply to Guillaume Marty [:gmarty] from comment #34)
> I spent most of the afternoon yesterday looking into it.
> The issue here is that we don't maintain a state in simcard_dialog.js about
> which SIM card is currently being asked for unlocking. We should refactor
> the logic to add real multi SIM support. I tried several fixes but there is
> always side effects somehow.
> I think somebody else familiar with the code should take a look.

Sounds sane, what kind of side effect? Do you have WIP?
Spoke with Guillaume this morning, and he does not have a WIP patch that would be helpful. Let's take this off his plate.

Alberto, this is 2.1 blocking bug. Can you help out here? Sounds like this will be a non-trivial fix.
Assignee: gmarty → nobody
Flags: needinfo?(apastor)
Guillaume, can you write down what you tried so we don't have to make the same mistakes again?
Flags: needinfo?(gmarty)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
First of all I worked in the optic of patching the current code, trying to avoid making huge changes.
I tried to reapply the patch above with simcard_dialog.js using an array of booleans for SimPinDialog.visible (rather than a boolean) with the SIM card id as the index. But the currentSlotIndex param is not always passed to SimLock.showIfLocked(), so we can't know which SIM card is requesting the pin code (see SimLock.init() as an example).

Then I though I would use SimPinDialog._currentSlot, it turns out it is not reliable and contains only the value of the SIM card detected last. We should probably use references to SIMSlotManager instead.

We need somehow to make SimPinDialog an instantiable object and create one instance per SIM card to unlock. Then we can just use the internal state and they won't be any conflicts. That means also probably one set of DOM element per dialog.

Alberto, I'm available via IRC or Vidyo if you want to discuss the code and the approach.
Flags: needinfo?(gmarty)
Thanks gmarty, I'll reach you via IRC to discuss it. Thanks!
Assignee: nobody → apastor
Flags: needinfo?(apastor)
(In reply to Guillaume Marty [:gmarty] from comment #38)
> First of all I worked in the optic of patching the current code, trying to
> avoid making huge changes.
> I tried to reapply the patch above with simcard_dialog.js using an array of
> booleans for SimPinDialog.visible (rather than a boolean) with the SIM card
> id as the index. But the currentSlotIndex param is not always passed to
> SimLock.showIfLocked(), so we can't know which SIM card is requesting the
> pin code (see SimLock.init() as an example).
> 
> Then I though I would use SimPinDialog._currentSlot, it turns out it is not
> reliable and contains only the value of the SIM card detected last. We
> should probably use references to SIMSlotManager instead.
> 
> We need somehow to make SimPinDialog an instantiable object and create one
> instance per SIM card to unlock. Then we can just use the internal state and
> they won't be any conflicts. That means also probably one set of DOM element
> per dialog.
> 
> Alberto, I'm available via IRC or Vidyo if you want to discuss the code and
> the approach.

This is just what I want to improve :)
One SIMPINDialog should map to only one SimSlot instance.
And we will have a SIMPINDialogManager to decide the timing to show 1st/2nd dialog according to the number of SIMSlots.

I'd done some cleanup in another bug, FYI
https://github.com/mozilla-b2g/gaia/pull/23910/files
Despite I totally agree with gmarty about refactoring the SIM lock handling (with 2 instances instead of 1), given that is a 2.1+ blocker, I worked in a workaround that seems less risky to me. I would like to hear gmarty's feedback before adding tests and so. Thanks!
Attachment #8494619 - Flags: feedback?(gmarty)
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

As you say, it's less risky to take this approach to uplift in 2.1 and work on a rewrite later.
It looks good to me, except the #triesLeft element is never shown and stays hidden. This must be fixed in simcard_dialog.js I think (see my original patch for that).

Apart from that f+!
Attachment #8494619 - Flags: feedback?(gmarty) → feedback+
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

Added unit tests

@gmarty, not sure about the 'hidden' attributes you were pointing out. What's the use case in which those 2 lines are needed? When I follow the STR of the bug with this patch, I can see the error message correctly. Probably I'm missing something.

Thanks!
Attachment #8494619 - Flags: review?(gmarty)
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

This needs to be reviewed by a system owner/peer. Alive is probably the best choice here.
Attachment #8494619 - Flags: review?(gmarty) → review?(alive)
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

Works for me for a blocker, thank you!
Hope we have time to revisit the refactoring in 2.2
Attachment #8494619 - Flags: review?(alive) → review+
master: https://github.com/mozilla-b2g/gaia/commit/d690ce554f2d105ac71cc13969a68df6ef76d7a1
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1059837
[User impact] if declined: If the screen goes to sleep while entering the PIN number, the state of that screen is lost (tries left, error messages, PUK, etc)
[Testing completed]: Added quite a lot of unit tests
[Risk to taking this patch] (and alternatives if risky): This bug needs a whole refactor of the SIM pin dialog, for showing 1 instance per SIM slot. This patch is a low risk workaround.
[String changes made]: -
Attachment #8494619 - Flags: approval-gaia-v2.1?(fabrice)
Comment on attachment 8494619 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393

Thanks for figuring out a solution with acceptable risk!
Attachment #8494619 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
This issue is verified fixed on Flame 2.1 and 2.2.

Result: Following STRs in Comment 10, the error message stayed after locking/unlocking the device.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141031001201
Gaia: f89c7b12c36572262c9ea76058694a139b1a8634
Gecko: 50d48f8a04c7
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141031061804
Gaia: a07994714f0552f89801d6097982308d8b0a1ee1
Gecko: 6bd2071b373f
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(In reply to Alberto Pastor [:albertopq] from comment #46)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> d690ce554f2d105ac71cc13969a68df6ef76d7a1

Hi, Alberto, I don't understand the reason why you add these codes below:

// Always showing the first slot first.
+ if (!this._alreadyShown && index > 1) {
+ return false;
+ }

It seems like the (index > 1) case will never be true, as the index can only be 0 or 1

Would you like to let me know the reason for these codes, thank you very much for kindly reply.
Flags: needinfo?(apastor)
At some point, the indexes for the slots were from 1 to n (instead of 0 to n). It seems that is not in that way anymore, so you are right, that doesn't makes sense now. It should be > 0. Thanks!
Flags: needinfo?(apastor)
(In reply to Alberto Pastor [:albertopq] from comment #41)
> Created attachment 8494619 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24393
> 

Alberto & gmarty, i think the changes below can cause bug 1119148

+ if (SimPinDialog.visible ? !skipped : skipped) {
+ return false;
+ }

Do you have any idea to avoid bug 1119148 and same time solve this bug, thanks
Flags: needinfo?(gmarty)
Flags: needinfo?(apastor)
I'm leaving Alberto to reply as he's more familiar with this codebase than me.
Flags: needinfo?(gmarty)
As :alive is pointing out in commen #40, we need to refactor this code for using 1 instance of the dialog for each SIM card (which was not feasible for the 2.1 timeframe). I'll try to visit this during this week. Thanks!
Flags: needinfo?(apastor)
You need to log in before you can comment on or make changes to this bug.