1026556 - Stk Timer Management Proactive Command error conditions not supported

Status: RESOLVED FIXED

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

Description

[Spencer]

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

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

You mean change this response code?
https://github.com/mozilla-b2g/gaia/blob/9141d16e1881eda5192b7b1dc6a7174d2b0a8613/apps/system/js/icc_worker.js#L463

Comment 2

[Spencer]

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)

Comment 3

[howie [:howie]]

Hi Spencer, could you provide what's the user impact here? Thanks.

Comment 4

[Spencer]

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

Comment 5

[howie [:howie]]

Hi Fernando, could you help with this? thank you

Comment 6

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to howie [:howie] from comment #5)
> Hi Fernando, could you help with this? thank you

Sure, I'll take it :)

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Adding new STK_RESULT code in the MozIccManager.webidl

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Gaia work to answer with the correct error code

It is needed a patched gecko (patch #8472900) to use this gaia fix !

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 10

[Fernando R. Sela (no CC, needinfo please) [: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.

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 12

[Carol Yang [:cyang]]

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.

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Thank you Carol for your feedback ;)
Fixed !

Comment 14

[Carol Yang [:cyang]]

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

Looks good. Thanks Fernando!

Comment 15

[Evelyn Hung]

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.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 18

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 19

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 21

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Looks good, maybe file a bug to get it removed in the future?

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 24

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Gaia code landed: https://github.com/mozilla-b2g/gaia/commit/66c5ef590b6fd75865fd215dc632c561afdeffc9
Followup bug to remove workaround commented in #21 - Bug 1059166

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 26

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 27

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 28

[bhavana bajaj [:bajaj]]

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

Comment 29

[José Antonio Olivera Ortega [:jaoo]]

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.

Comment 30

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 31

[Dietrich Ayala (:dietrich)]

Beatriz, is this a certification issue? It's not a regression, so not sure we should block the release on it. Thanks!

Comment 32

[Beatriz Rodríguez [:brg]]

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

Comment 33

[bhavana bajaj [:bajaj]]

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

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 35

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 36

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1736899d7171

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1736899d7171

Comment 38

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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

Comment 39

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

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.

Comment 40

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Landed gaia patch too: https://github.com/mozilla-b2g/gaia/commit/e5fbb645ef78b42535319a647f61372c9124c00b

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9343a6f56e82

v2.1: https://github.com/mozilla-b2g/gaia/commit/78db9166f544fa120fcd1ea3b0a047998e9d3274