1115493 - [FOS7715 v2.1]Setup call-forwarding for both SIM cards in DSDS, then removal one sim card and reopen the device, the status bar still shows two icons of call forwarding

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[jinghua.xing]

This issue is because we receive the event 'simslot-iccinfochange' event before  set the 'ril.cf.enabled' to [false,false] by default. So we will get the data [true, true] by reading the _value in _callForwardingHelper.Then 'ril.cf.enabled' will be set to [true,true] instead of [false,true]. This will lead to two call-forwarding icons showed on the status bar.

Below is the logs of this issue:

12-23 21:08:33.200 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:49 in sh_getValue: arvin --- _getValue: SETTINGS_KEYril.cf.enabled
12-23 21:08:42.030 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:52 in sh_getValue/req.onsuccess: arvin --- _getValue: SETTINGS_KEYril.cf.enabled, value=true,true

12-23 21:08:42.030 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:64 in sh_setValue: arvin --- _setValue: SETTINGS_KEY=ril.cf.enabled, value=false,false
12-23 21:08:42.040 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:64 in sh_setValue: arvin --- _setValue: SETTINGS_KEY=ril.cf.enabled, value=true,true

12-23 21:08:45.960 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:67 in sh_setValue/req.onsuccess: arvin --- onsuccess: SETTINGS_KEY=ril.cf.enabled, value=false,false
12-23 21:08:46.010 E/GeckoConsole(  131): Content JS LOG at app://system.gaiamobile.org/shared/js/settings_helper.js:67 in sh_setValue/req.onsuccess: arvin --- onsuccess: SETTINGS_KEY=ril.cf.enabled, value=true,true

Comment 1

[jinghua.xing]

Based on v2.1

Comment 2

[jinghua.xing]

I have tried to find two ways for this issue.

The first one is to split 'ril.cf.enabled' setting to two settings, each corresponds to the call-forwarding state of one separate SIM card. In this way, when we set the call-forwarding state of one SIM card to settings, it won't affect another one.

The second way is we use 
>navigator.mozSettings.createLock().set({'ril.cf.enabled':this._defaultCallForwardingIconStates})
directly to set the 'ril.cf.enabled' to [false, false] instead of _callForwardingHelper.set() and then use
>navigator.mozSettings.createLock().get('ril.cf.enabled')
in function _initCallForwardingState() to get current state of call-forwarding directly in settings instead of read the _value in _callForwardingHelper. In this way, the get and set operation of 'ril.cf.enabled' setting will be handled one by one and ensure setting the value of 'ril.cf.enabled' is behind setting it to default.

Comment 3

[jinghua.xing]

vance：

Can you help us for this issue?
Thank you.

Comment 4

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Shawn -

Any suggestions on this one?

Thanks

Vance

Comment 5

[shawn ku [:sku]]

Hi Chens:
 Please help check this issue first per comment 0 and 2.

Thanks!!
Shawn

Comment 6

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to 2.1

Jinghua, could you try this patch first? I think it can fix this problem.

Arthur and EJ, this patch uses _callForwardingIconInitializedStates as default and overwrite it when we have new call forwarding state, can you take a look and give some feedback? thanks!

Comment 7

[jinghua.xing]

Hi Chens:

Sorry for my delay. I have tried the patch you provided, it can fix the problem. Thank you.

But I am not sure if we can use _callForwardingIconInitializedStates directly before setting the property 'ril.cf.enabled' successfully. What will happen if setting the property returns an error?

Comment 8

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8542773 [details] [review]
Pull request to 2.1

Hi Arthur, 

This patch tend to use _callForwardingIconInitializedStates as default value and fixes the problem we have. I've tried different combinations and it seems works fine. Could you review this patch and give some feedback? thanks!

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8542773 [details] [review]
Pull request to 2.1

Sorry for the late response.

This issue is basically a racing condition. In this case we should wait for both icons being set as hidden then we do the initialization. 

I think we can remove the setting of the default values but call to `_initCallForwardingState` directly. In `initCallForwardingState` we set the icon as hidden when the icc info is not available.

Comment 10

[Sherman Chen [:chens] (inactive)]

Attachment: Pull request to master

Hi Arthur,

Update pull request per offline discussion, would you review this patch and give some feedback? thanks!

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8552332 [details] [review]
Pull request to master

Thanks for the patch. Note that `callForwardingIconInitializedStates` is used to record whether an icon has been initialized, so we don't use it the cache the icon states. Icon states should be retrieved from `ril.cf.enabled`. 

In `_initCallForwardingState`, we should always set values to `ril.cf.enabled` based on the current availability of sim cards and the value stored in async storage.

Comment 12

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8552332 [details] [review]
Pull request to master

Hi Arthur, 
I've revised the patch, would you review this patch again? thanks!

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8552332 [details] [review]
Pull request to master

Note that for ALL cases that we are not able to get the value stored under the async storage, we hide the icon.

For example, it is possible that when the first call to `_initCallForwardingState` is made, the sim card is available but the card state is not ready or the icc id is unavailable.

Comment 14

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8552332 [details] [review]
Pull request to master

Hi Arthur, would you review this patch again? thanks!

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8552332 [details] [review]
Pull request to master

The patch is looking good! We should also add assertions ensuring `_initCallForwardingState` does set 'ril.cf.enabled' to the correct value when in those three cases that iccid is unavailable.

Comment 16

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8552332 [details] [review]
Pull request to master

Add assertion for 'ril.cf.enabled', would you mind to review it again? thanks!

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8552332 [details] [review]
Pull request to master

r=me with the last comments addressed. Thanks.

Request Alive's review ensuring it won't break anything.

Comment 18

[Sherman Chen [:chens] (inactive)]

Unit tests updated and comment addressed, thanks Arthur :)

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8552332 [details] [review]
Pull request to master

I think this will not affect bug 1098168, but I'll confirm later when landed.

Comment 20

[Sherman Chen [:chens] (inactive)]

master: https://github.com/mozilla-b2g/gaia/commit/ba613ae583a706131c45e885f65d428d4a541a81

Comment 21

[Sherman Chen [:chens] (inactive)]

Jinghua, could you try this patch (attachment 8552332 [details] [review]) and verify the problem is fixed?

Comment 22

[Sherman Chen [:chens] (inactive)]

We should also verify this on master before requesting uplift.

Comment 23

[Yang Yang]

(In reply to Sherman Chen [:chens] from comment #21)
> Jinghua, could you try this patch (attachment 8552332 [details] [review]) and verify
> the problem is fixed?

Hi, chen:

I have verify this patch on FOS7715 v2.1, it seems ok, the problem is fixed.

Comment 24

[Yang Yang]

(In reply to Sherman Chen [:chens] from comment #21)
> Jinghua, could you try this patch (attachment 8552332 [details] [review]) and verify
> the problem is fixed?

I think this problem is happende because of this statement： this._callForwardingHelper.set(this._defaultCallForwardingIconStates);

this._defaultCallForwardingIconStates is an array and there should be a object.

I also have my own patch for this bug, please review it, do you think it works fine?

Comment 25

[Yang Yang]

Attachment: 7715_v2.1.patch

Comment 26

[Yang Yang]

chen, please help to review, thanks

Comment 27

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8555821 [details] [diff] [review]
7715_v2.1.patch

> this._defaultCallForwardingIconStates is an array and there should be a object.

Why do you think it should be an object? 

> I also have my own patch for this bug, please review it, do you think it works 
> fine?

Have you ever tried that? I don't think this make any difference and won't work.

We have this patch (attachment 8552332 [details] [review]) land on master and will be uplifted to 2.1/2.1s. And since you have confirmed this patch works, why don't you wait for this being uplifted and fix this problem?

Comment 28

[Yang Yang]

(In reply to Sherman Chen [:chens] from comment #27)


> > this._defaultCallForwardingIconStates is an array and there should be a object.
> 
> Why do you think it should be an object? 

Sorry, i think i am wrong for that.
I use (states instanceof Array) and (states.constructor === Array) to judge whether states is an array, and both returns true, so states is an array.

I think your patch is ok, and wait for it be uplifted to 2.1/2.1s.

Comment 29

[Sherman Chen [:chens] (inactive)]

Comment on attachment 8552332 [details] [review]
Pull request to master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Pre-existed situation
[User impact] if declined: Call forwarding status for dual sim is not correct and will confuse user.
[Testing completed]: Manual test and unit test.
[Risk to taking this patch] (and alternatives if risky): Should be pretty low, verified by partner's QA, and this patch can fix race condition in some cases.
[String changes made]: None.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/5b52511b028506d5fbe83651cc9dfe7460f0fbe8
v2.1: https://github.com/mozilla-b2g/gaia/commit/5f6703f35ec8ed4ee5c377eaf03839c4ea0c77fd