935802 - [fugu]no stk entry in the settings app

Status: RESOLVED DUPLICATE of bug 906164

(Firefox OS Graveyard :: Vendcom, defect)

Description

[Ming]

We ofen found that the stk menu was not setup.
After tracking some logs, i found there is a bug can cause this issue.

Briefly, system/js/icc.js registerd to receive the  broadcast msg of “icc-stkcommand" after the system is ready.
Actually, after these events: "webapps-registry-ready" and "system-message-listener-ready".

See into icc.js ,it's obviously that the problem is we registe the handler of msg of "icc-stkcommand" within a asynchronous callback in "clearcache".

Comment 1

[Ming]

Refered to hamachi ,modem side will report the UNSOLICITED_STK_PROACTIVE_COMMAND for many times util there is a response.
But for fugu , it seems the modem is always waiting for the response of the first command of UNSOLICITED_STK_PROACTIVE_COMMAND .


I sugguest that the system/icc.js need to be changed to make sure all the  icc-stkcommand be received.

Comment 2

[Ming]

So for fugu , if the sim is locked by pin , after reboot, enter the pin , all things are right ,because the stkcommand is delayed.

Comment 3

[Ming]

can i change the code in icc.init as below?
    this.clearMenuCache(function() {
	console.log("clearMenuCache done");
    });
    window.navigator.mozSetMessageHandler('icc-stkcommand',
        function callHandleSTKCommand(message) {
        self.handleSTKCommand(message);
    });


Will it cause any problem?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

ni? for fesela as he's the owner in STK app.

Comment 5

[Ming]

(In reply to Ming from comment #3)
Maybe we can delay the action of clearMenuCache 

like this:
	window.navigator.mozSetMessageHandler('icc-stkcommand',
	function callHandleSTKCommand(message) {
	  self.needClearCache = false;
	  self.handleSTKCommand(message);
	});
	setTimeout(function(){
		if (self.needClearCache) {
			this.clearMenuCache(function(){});
		}
	},1000);

Comment 6

[Ming]

Attachment: 0001-bug-234341-no-stk-entry-in-the-settings.patch

Comment 7

[Ming]

Comment on attachment 828477 [details] [diff] [review]
0001-bug-234341-no-stk-entry-in-the-settings.patch

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

obsolete

Comment 8

[Ming]

Attachment: 0001-bug-234341-no-stk-entry-in-the-settings.patch

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 828484 [details] [diff] [review]
0001-bug-234341-no-stk-entry-in-the-settings.patch

For Gaia you need to go through Pull Request from github.

Also now Bugzilla has intergrated with github.

see http://globau.wordpress.com/2013/10/21/github-pull-requests-and-bugzilla/

Comment 10

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> ni? for fesela as he's the owner in STK app.

Hi, this code is to remove STK menu entry before receiving any STK command.

A better solution is to check if the SIM was changed (based on ICC or similar) and avoid remove it if the SIM wasn't changed.

Currently we allways remove it and wait for the menu given by the STK app.

Comment 11

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

Comment on attachment 828477 [details] [diff] [review]
0001-bug-234341-no-stk-entry-in-the-settings.patch

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

As Yoshi told before, Gaia is reviewed as PR in github.

Anyway, some little comments...

::: apps/system/js/icc.js
@@ +19,5 @@
> +      // window.navigator.mozSetMessageHandler('icc-stkcommand',
> +        // function callHandleSTKCommand(message) {
> +          // self.handleSTKCommand(message);
> +        // });
> +    // });

Remove dead code

@@ +30,5 @@
>  
> +    setTimeout(function() {
> +        if (self.needClearCache) {
> +            this.clearMenuCache(function() {});
> +    },1000);

When needClearCache is enabled? Where is it defined?

Did you test race condition cases?

Also, if you receive the Apps menu and the the clearMenuChache is launched it'll remove the just received message... check this corner cases

Comment 12

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

A question, Is this bug the same as this other one?

https://bugzilla.mozilla.org/show_bug.cgi?id=907628

Comment 13

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

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #12)
> A question, Is this bug the same as this other one?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=907628

Also take a look at this other bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=898699

and a WIP patch proposal (not landed):

https://github.com/mozilla-b2g/gaia/pull/11711/files

Comment 14

[Ming]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #12)
> A question, Is this bug the same as this other one?
> https://bugzilla.mozilla.org/show_bug.cgi?id=907628

Yes, they seems to be same.  But at last ,it suggest turn to bug 93385.
I couldn't find a solution for the problem of system/icc.js.

The rootcause is asynchronous callback  in it，not in the settings app. 

If modem side isn't stronger like qcom ,who sends the RIL_UNSOL_STK_PROACTIVE_COMMAND for many times,  this always be a problem.

Thanks very much!

Comment 15

[Ming]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #11)
When needClearCache is enabled? Where is it defined?

Did you
> test race condition cases?

Also, if you receive the Apps menu and the the
> clearMenuChache is launched it'll remove the just received message... check
> this corner cases

Sorry .
1. I added the definiton later in the update patch. But i forget to obsolete the old one.

2. I didn't test the race condition case.

3. In icc.init ,if the stkcommand comes first , then the clearMenuChache will have no chance to run.

Comment 16

[Ming]

(In reply to Ming from comment #3)
> can i change the code in icc.init as below?
   
> this.clearMenuCache(function() {
	console.log("clearMenuCache done");
   
> });
    window.navigator.mozSetMessageHandler('icc-stkcommand',
       
> function callHandleSTKCommand(message) {
       
> self.handleSTKCommand(message);
    });


It seems this is enough. It seems that multi calls to mozSettings.createLock.set/get will be executed in the order they calls .

Is there someone can verify this?

Comment 17

[Jason Smith [:jsmith]]

mvines - comment 14 implies that this is a com ril bug. Can someone on your team look into this?

Removing koi? since this is a vendor bug.

Comment 18

[Michael Vines [:m1] [:evilmachines]]

Not much I can help with here unfortunately ;)

Comment 19

[Jason Smith [:jsmith]]

Fernando - So I'm confused what to do with this bug. Is this technically a Gaia bug on our side? Or is this a bug in the vendor code? And is this a regression or not?

Comment 20

[Ken Chang[:kenkai]]

Joe, I wonder if this is 1.3+.

Comment 21

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

(In reply to Jason Smith [:jsmith] from comment #19)
> Fernando - So I'm confused what to do with this bug. Is this technically a
> Gaia bug on our side? Or is this a bug in the vendor code? And is this a
> regression or not?

Hi Jason,

This is a race condition between the STK Cache management and the RIL sending the STK Main Menu command.

Currently we clean the cached STK menu each time we restart the device, after that we register for new STK Commands.

If the RIL sents the command before we register the callback, we lost it :(

Sometime ago we delayed the SIM initialization on gecko side to avoid these effects;

Now the suggested patch is to register the ICC callback as soon as possible, and start the cleaning process. If we receive the command before start cleaning, we avoid remove it.

Anyway, this patch can have another race conditions if the ICC message and clean timeout fires at the sametime. But since the SIM sents this message in a really early stage, we can "assure" that a second is enough to check cache cleaning ...

Comment 22

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

Comment on attachment 828484 [details] [diff] [review]
0001-bug-234341-no-stk-entry-in-the-settings.patch

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

::: apps/system/js/icc.js
@@ +20,5 @@
> +      // window.navigator.mozSetMessageHandler('icc-stkcommand',
> +        // function callHandleSTKCommand(message) {
> +          // self.handleSTKCommand(message);
> +        // });
> +    // });

Avoid dead code...

Comment 23

[Ming]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #21)
> we can "assure" that a second is enough to check cache
> cleaning ...

Can the cache clening be done ahead of the message of "webapps-registry-ready"?

if yes, it'll be ok.  As the delay the gecko side does.

Comment 24

[Steven Yang [:styang]]

James, do you want to submit another patch?

Comment 25

[James Zhang (Spreadtrum)]

(In reply to Steven Yang [:styang] from comment #24)
> James, do you want to submit another patch?

I suggest add sprd flag to support sprd modem behavior.
We need confirm the flag first, use 'moz.sprd.ril'?

Comment 26

[Ken Chang[:kenkai]]

(In reply to James Zhang from comment #25)
> (In reply to Steven Yang [:styang] from comment #24)
> > James, do you want to submit another patch?
> 
> I suggest add sprd flag to support sprd modem behavior.
> We need confirm the flag first, use 'moz.sprd.ril'?
James, do you want to use this in RIL_worker.js? Or other files?

Comment 27

[James Zhang (Spreadtrum)]

(In reply to Ken Chang from comment #26)
> (In reply to James Zhang from comment #25)
> > (In reply to Steven Yang [:styang] from comment #24)
> > > James, do you want to submit another patch?
> > 
> > I suggest add sprd flag to support sprd modem behavior.
> > We need confirm the flag first, use 'moz.sprd.ril'?
> James, do you want to use this in RIL_worker.js? Or other files?

It's better in RIL_worker.js. We can try to use a meaningful name just like 'ro.moz.mute.call.to_ril'.

Comment 28

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

(In reply to Ming from comment #23)
> (In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from
> comment #21)
> > we can "assure" that a second is enough to check cache
> > cleaning ...
> 
> Can the cache clening be done ahead of the message of
> "webapps-registry-ready"?
> 
> if yes, it'll be ok.  As the delay the gecko side does.

I think this is a reasonable solution ;)

Comment 29

[Ming]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #28)

> 
> I think this is a reasonable solution ;)

Will you take this bug?

Comment 30

[Enpei]

fugu master build has this bug as well.

Gaia      b411fd6c7d074ce4b4e8248e28981411a1420197
Gecko     c5fbc5c2efc86f33591a23b7e54c6ff4f1a472e0
BuildID   20131204140040
Version   28.0a1

Comment 31

[James Zhang (Spreadtrum)]

(In reply to Enpei from comment #30)
> fugu master build has this bug as well.
> 
> Gaia      b411fd6c7d074ce4b4e8248e28981411a1420197
> Gecko     c5fbc5c2efc86f33591a23b7e54c6ff4f1a472e0
> BuildID   20131204140040
> Version   28.0a1
This build with sprd patch?

Steven, can you help to merge ming's patch to 1.2f?

Comment 32

[Steven Yang [:styang]]

Please make sure there's no other way out in short-term. Ken, comments?

Comment 33

[Ken Chang[:kenkai]]

(In reply to Steven Yang [:styang] from comment #32)
> Please make sure there's no other way out in short-term. Ken, comments?
Another thought, I wonder if RILC could keep sending RIL_UNSOL_STK_PROACTIVE_COMMAND until RILC get a response from STK
app.

Comment 34

[Ming]

(In reply to Ken Chang[:ken] from comment #33)

> Another thought, I wonder if RILC could keep sending
> RIL_UNSOL_STK_PROACTIVE_COMMAND until RILC get a response from STK
> app.


pls refer to comment 23 & 28 

I think that's the best way to fix this bug.

Comment 35

[Ken Chang[:kenkai]]

(In reply to Ming from comment #34)
> pls refer to comment 23 & 28 
> 
> I think that's the best way to fix this bug.
I wonder if a delay is a good solution. Since we never know what is the best delay time.

Comment 36

[Edgar Chen [:edgar]]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #21)
> (In reply to Jason Smith [:jsmith] from comment #19)
> > Fernando - So I'm confused what to do with this bug. Is this technically a
> > Gaia bug on our side? Or is this a bug in the vendor code? And is this a
> > regression or not?
> 
> Hi Jason,
> 
> This is a race condition between the STK Cache management and the RIL
> sending the STK Main Menu command.
> 
> Currently we clean the cached STK menu each time we restart the device,
> after that we register for new STK Commands.
> 
> If the RIL sents the command before we register the callback, we lost it :(
> 
> Sometime ago we delayed the SIM initialization on gecko side to avoid these
> effects;
> 
> Now the suggested patch is to register the ICC callback as soon as possible,
> and start the cleaning process. If we receive the command before start
> cleaning, we avoid remove it.
> 
> Anyway, this patch can have another race conditions if the ICC message and
> clean timeout fires at the sametime. But since the SIM sents this message in
> a really early stage, we can "assure" that a second is enough to check cache
> cleaning ...

It is a flaw in system message, will fix it in bug 906164.