Stk Timer Management Proactive Command error conditions not supported

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Spencer, Assigned: frsela)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [caf priority: p2][CR 725350])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Timer Management Proactive Command error conditions are not caught in two cases:
1) A Deactivate command is sent and the timer has not been started
2) A Get Current Time command is sent and the timer has not been started

Both cases should return 0x24: 'action in contradiction with the current timer state'

This is specified in 3GPP TS 11.14 section 6.4.21 (TIMER MANAGEMENT):

"when the SIM requests the ME to deactivate the timer, then: if the timer is already deactivated, the ME shall inform the SIM using TERMINAL RESPONSE ('action in contradiction with the current timer state')"

and 

"when the SIM requests the ME to get the current value of the timer, then: if the timer is deactivated, the ME shall inform the SIM using TERMINAL RESPONSE ('action in contradiction with the current timer state')"

https://github.com/mozilla-b2g/gaia/blob/9141d16e1881eda5192b7b1dc6a7174d2b0a8613/apps/system/js/icc_worker.js#L437
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#872
(Reporter)

Updated

4 years ago
Flags: needinfo?(frsela)
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 2.1?
(Reporter)

Comment 2

4 years ago
The current response code at line 463 is fine. There needs to be a new seperate terminal response with result code 0x24 in both the STK_TIMER_DEACTIVATE (line 461) and STK_TIMER_GET_CURRENT_VALUE (line 473) cases that is sent instead of the STK_RESULT_OK terminal response in the error cases.

advanced_timer returns 0 when the timer had not been started yet and stop() is called (https://github.com/mozilla-b2g/gaia/blob/2a64e655196e86b5c7f12a521507c328d59b23cd/shared/js/advanced_timer.js#L37). Similarly advanced_timer returns 0 when the timer had not been started yet and queryPendingTime() is called (https://github.com/mozilla-b2g/gaia/blob/2a64e655196e86b5c7f12a521507c328d59b23cd/shared/js/advanced_timer.js#L51). When either of these cases happen the new seperate terminal response with result code 0x24 is sent. This new terminal response does not need to include timerId or timerValue (3GPP TS 11.14 section 6.4.21)
Flags: needinfo?(sbarton)

Updated

4 years ago
Blocks: 1025317

Comment 3

4 years ago
Hi Spencer, could you provide what's the user impact here? Thanks.
Flags: needinfo?(sbarton)
(Reporter)

Comment 4

4 years ago
Howie: This bug fix is necessary to meet specifications (3GPP TS 11.14). The SIM Timer Expiration Envelope Command has been implemented but remains buggy. - Spencer
Flags: needinfo?(sbarton)

Comment 5

4 years ago
Hi Fernando, could you help with this? thank you
Status: UNCONFIRMED → NEW
blocking-b2g: 2.1? → 2.1+
Ever confirmed: true
Flags: needinfo?(frsela)
(In reply to howie [:howie] from comment #5)
> Hi Fernando, could you help with this? thank you

Sure, I'll take it :)
Assignee: nobody → frsela
Flags: needinfo?(frsela)
Created attachment 8472900 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl
Attachment #8472900 - Flags: review?(htsai)
Created attachment 8472905 [details] [review]
Gaia work to answer with the correct error code

It is needed a patched gecko (patch #8472900) to use this gaia fix !
Attachment #8472905 - Flags: review?(ehung)
Attachment #8472905 - Flags: feedback?(sbarton)
(In reply to [PTO from 18 to 21 Aug] Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #7)
> Created attachment 8472900 [details] [diff] [review]
> Adding new STK_RESULT code in the MozIccManager.webidl

Hey Fernando,

I assume there is a patche for ril_worker implementation coming, right?

Updated

4 years ago
Flags: needinfo?(frsela)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> (In reply to [PTO from 18 to 21 Aug] Fernando R. Sela (no CC, needinfo
> please) [:frsela] from comment #7)
> > Created attachment 8472900 [details] [diff] [review]
> > Adding new STK_RESULT code in the MozIccManager.webidl
> 
> Hey Fernando,
> 
> I assume there is a patche for ril_worker implementation coming, right?

For the Gaia part the constant value is only needed.
For the Gecko part I assumed the result code received will be passed to the SIM card so no more changes are needed. Correct me if I'm wrong.
Flags: needinfo?(frsela)
Comment on attachment 8472900 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl

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

Comment 10 is right. This looks good to me, thanks.
Currently we also need DOM peer's review on .webidl change, otherwise the patch can't be landed. Please invite them as well. :)
Attachment #8472900 - Flags: review?(htsai) → review+
Attachment #8472900 - Flags: superreview?(jonas)

Comment 12

4 years ago
Comment on attachment 8472905 [details] [review]
Gaia work to answer with the correct error code

Hi Fernando,

In addition to error STK_RESULT_ACTION_CONTRADICTION_TIMER_STATE, the STK response also needs to contain the timerId, i.e.:

if (a_timer.queryPendingTime(options.timerId) === 0) {
  icc.responseSTKCommand(message, {
    timer: {
      'timerId': options.timerId,
    },
    resultCode: icc._iccManager.STK_RESULT_ACTION_CONTRADICTION_TIMER_STATE
  });
else {
...
}

This needs to be done for both DEACTIVATE and GET_CURRENT_VALUE cases.
Attachment #8472905 - Flags: feedback?(sbarton) → feedback-
Comment on attachment 8472905 [details] [review]
Gaia work to answer with the correct error code

Thank you Carol for your feedback ;)
Fixed !
Attachment #8472905 - Flags: feedback- → feedback?(cyang)

Comment 14

4 years ago
Comment on attachment 8472905 [details] [review]
Gaia work to answer with the correct error code

Looks good. Thanks Fernando!
Attachment #8472905 - Flags: feedback?(cyang) → feedback+

Comment 15

4 years ago
Comment on attachment 8472905 [details] [review]
Gaia work to answer with the correct error code

I actually don't have right to review System, redirect to Tim.
@Tim, Tzu-lin knows STK, feel free to set f? to him if you need his feedback.
Attachment #8472905 - Flags: review?(ehung) → review?(timdream)
Attachment #8472905 - Flags: review?(timdream) → review+
Comment on attachment 8472900 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl

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

Hsin-Yi's review on STK and Telephony interfaces is enough for me. No need for sr unless you really want to. And generally on STK Hsin-Yi's input is going to be much more useful than mine :)
Attachment #8472900 - Flags: superreview?(jonas) → superreview+
Hrm.. I guess there's a checkin hook that requires that you guys get a DOM peer to review. Let's look at how we can fix that outside of this bug.
Hi Tim,

Since this Gaia patch depends in a little gecko change [1], should we add a check in Gaia to support this patch with old and new gecko versions?

Something like:

  if (!icc._iccManager.STK_TIMER_DEACTIVATE) {
    icc._iccManager.STK_TIMER_DEACTIVATE = 0x24;
  }

Or since this is a not very common used option, we only add a recommendation to inform about a required Gecko version to use this. WDYT?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900
Flags: needinfo?(timdream)
Comment on attachment 8472900 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl

Approval Request Comment
[Feature/regressing bug #]: 1026556
[User impact if declined]: This will be needed by FirefoxOS v2.1
[Describe test coverage new/current, TBPL]: -
[Risks and why]: none, is only a new constant value
[String/UUID change made/needed]: none
Attachment #8472900 - Flags: approval-mozilla-b2g32?
Attachment #8472900 - Flags: approval-mozilla-aurora?
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #18)
> Hi Tim,
> 
> Since this Gaia patch depends in a little gecko change [1], should we add a
> check in Gaia to support this patch with old and new gecko versions?
> 
> Something like:
> 
>   if (!icc._iccManager.STK_TIMER_DEACTIVATE) {
>     icc._iccManager.STK_TIMER_DEACTIVATE = 0x24;
>   }
> 
> Or since this is a not very common used option, we only add a recommendation
> to inform about a required Gecko version to use this. WDYT?
> 
> [1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900

That's fine. Although the syntax should be

if (!('STK_TIMER_DEACTIVATE' in icc._iccManager))

and we need a comment above the in the code.
Flags: needinfo?(timdream)
Thank you.
Code updated. You can check it in the GitHub.

diff --git a/apps/system/js/icc.js b/apps/system/js/icc.js
index 25e1dab..5272503 100755
--- a/apps/system/js/icc.js
+++ b/apps/system/js/icc.js
@@ -9,8 +9,18 @@ var icc = {
   _inputTimeout: 40000,
   _toneDefaultTimeout: 5000,
 
+  checkPlatformCompatibility: function icc_checkPlatformCompat() {
+    // The STK_RESULT_ACTION_CONTRADICTION_TIMER_STATE constant will be added
+    // in the next versions of the platform. This code avoid errors if running
+    // in old versions. See bug #1026556
+    if (!('STK_RESULT_ACTION_CONTRADICTION_TIMER_STATE' in this._iccManager)) {
+      this._iccManager.STK_RESULT_ACTION_CONTRADICTION_TIMER_STATE = 0x24;
+    }
+  },
+
   init: function icc_init() {
     this._iccManager = window.navigator.mozIccManager;
+    checkPlatformCompatibility();
     var self = this;
     this.clearMenuCache(function() {
       window.navigator.mozSetMessageHandler('icc-stkcommand',
Flags: needinfo?(timdream)
Looks good, maybe file a bug to get it removed in the future?
Flags: needinfo?(timdream)
Attachment #8472900 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8472900 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl

Setting back to "?".  Needs to land in central before.
Attachment #8472900 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Blocks: 1059166
See Also: → bug 1059166
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #24)
> Gaia code landed:
> https://github.com/mozilla-b2g/gaia/commit/
> 66c5ef590b6fd75865fd215dc632c561afdeffc9
> Followup bug to remove workaround commented in #21 - Bug 1059166

Reverted for Gaia unit test failures which your Gaia Try run conveniently also had. Please look at your results before merging next time.
https://github.com/mozilla-b2g/gaia/commit/d21c76cb3cd31dfaf8745034441d6fe83dca01b9
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #24)
> > Gaia code landed:
> > https://github.com/mozilla-b2g/gaia/commit/
> > 66c5ef590b6fd75865fd215dc632c561afdeffc9
> > Followup bug to remove workaround commented in #21 - Bug 1059166
> 
> Reverted for Gaia unit test failures which your Gaia Try run conveniently
> also had. Please look at your results before merging next time.
> https://github.com/mozilla-b2g/gaia/commit/
> d21c76cb3cd31dfaf8745034441d6fe83dca01b9

Oups! Sorry ... OMG! the last minute change I forgot double check :'(
Fixed here: https://github.com/mozilla-b2g/gaia/pull/23398
waiting tests ...
Created attachment 8480352 [details] [review]
Gaia work to answer with the correct error code - Fixed

Same patch as [1] but fixed.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8472905&action=edit
Attachment #8472905 - Attachment is obsolete: true
Attachment #8480352 - Flags: review+
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #19)
> Comment on attachment 8472900 [details] [diff] [review]
> Adding new STK_RESULT code in the MozIccManager.webidl
> 
> Approval Request Comment
> [Feature/regressing bug #]: 1026556
> [User impact if declined]: This will be needed by FirefoxOS v2.1
> [Describe test coverage new/current, TBPL]: -
> [Risks and why]: none, is only a new constant value
> [String/UUID change made/needed]: none

Given the unser impact I am confused why this would be needed on 2.0 (which is on b2g32) ? We are only consider release blocking issue on 2.0 at this point so unless this is a show stopper for release lets get this fixed in 2.1 (to be based of aurora soon)
Fernando, I got this response from the repo when pushing the patch as you requested me.

remote: ************************** ERROR ****************************
remote: Pushing to an APPROVAL REQUIRED tree requires your top changeset comment to include: a=... (or, more accurately, a\S*=...)
remote: *************************************************************

Please, address this and ping me when you are done. I'll be happy to push it again.
Flags: needinfo?(frsela)
Created attachment 8482619 [details] [diff] [review]
Bug1026556.patch

This only adds the reviewers to the commit summary. same patch as [1]

r+ - htsai
sr+ - jonas

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900
Attachment #8472900 - Attachment is obsolete: true
Attachment #8472900 - Flags: approval-mozilla-b2g32?
Attachment #8472900 - Flags: approval-mozilla-aurora?
Attachment #8482619 - Flags: superreview+
Attachment #8482619 - Flags: review+
Flags: needinfo?(frsela)
Beatriz, is this a certification issue? It's not a regression, so not sure we should block the release on it. Thanks!
Flags: needinfo?(beatriz.rodriguezgomez)
(In reply to Dietrich Ayala (:dietrich) from comment #31)
> Beatriz, is this a certification issue? It's not a regression, so not sure
> we should block the release on it. Thanks!

Unless you remove the whole piece you need it to pass certification. According to comment 4, it seems a new feature that it is not working fine yet. It will block GCF certification and operator cert as well.

>The SIM Timer Expiration Envelope Command has been implemented but remains buggy.
Flags: needinfo?(beatriz.rodriguezgomez)
(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #30)
> Created attachment 8482619 [details] [diff] [review]
> Bug1026556.patch
> 
> This only adds the reviewers to the commit summary. same patch as [1]
> 
> r+ - htsai
> sr+ - jonas
> 
> [1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900

IS this ready to be checked-in given the r+ ?
Comment on attachment 8482619 [details] [diff] [review]
Bug1026556.patch

Mercurial version (if prefered for landing) of the Same patch as [1]

r+ - htsai
sr+ - jonas

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900
Created attachment 8485619 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl (Mercurial version)

Mercurial version (if prefered for landing) of the Same patch as [1]

r+ - htsai
sr+ - jonas

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8472900
Attachment #8485619 - Flags: superreview+
Attachment #8485619 - Flags: review+
Attachment #8482619 - Attachment is obsolete: true

Updated

4 years ago
Whiteboard: [CR 725350]

Updated

4 years ago
Whiteboard: [CR 725350] → [caf priority: p2][CR 725350]
https://hg.mozilla.org/mozilla-central/rev/1736899d7171
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Comment on attachment 8485619 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl (Mercurial version)

Approval Request Comment
[Feature/regressing bug #]: 1026556
[User impact if declined]: This will be needed by FirefoxOS v2.1
[Describe test coverage new/current, TBPL]: -
[Risks and why]: none, is only a new constant value
[String/UUID change made/needed]: none
Attachment #8485619 - Flags: approval-mozilla-b2g32?
Attachment #8485619 - Flags: approval-mozilla-aurora?
Comment on attachment 8485619 [details] [diff] [review]
Adding new STK_RESULT code in the MozIccManager.webidl (Mercurial version)

This is for FFOS 2.1 so uplift to aurora is enough.
Attachment #8485619 - Flags: approval-mozilla-b2g32?

Updated

4 years ago
Attachment #8485619 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9343a6f56e82

v2.1: https://github.com/mozilla-b2g/gaia/commit/78db9166f544fa120fcd1ea3b0a047998e9d3274
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
You need to log in before you can comment on or make changes to this bug.