[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

RESOLVED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jinghua.xing, Assigned: chens)

Tracking

({verifyme})

unspecified
2.2 S5 (6feb)
x86_64
Linux
verifyme

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.0 ?, b2g-v2.0M ?, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Based on v2.1
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Updated

4 years ago
Summary: 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 → [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
(Reporter)

Comment 3

4 years ago
vance:

Can you help us for this issue?
Thank you.
Flags: needinfo?(vchen)
Shawn -

Any suggestions on this one?

Thanks

Vance
Flags: needinfo?(vchen) → needinfo?(sku)

Comment 5

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

Thanks!!
Shawn
Flags: needinfo?(sku) → needinfo?(shchen)
(Assignee)

Comment 6

4 years ago
Created attachment 8542773 [details] [review]
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!
Assignee: nobody → shchen
Flags: needinfo?(shchen) → needinfo?(Jinghua.Xing)
Attachment #8542773 - Flags: feedback?(ejchen)
Attachment #8542773 - Flags: feedback?(arthur.chen)
(Reporter)

Comment 7

4 years ago
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?
Flags: needinfo?(Jinghua.Xing)
(Assignee)

Comment 8

4 years ago
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!
Attachment #8542773 - Flags: feedback?(arthur.chen) → review?(arthur.chen)
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.
Attachment #8542773 - Flags: review?(arthur.chen)
Attachment #8542773 - Flags: feedback?(ejchen)
(Assignee)

Comment 10

4 years ago
Created attachment 8552332 [details] [review]
Pull request to master

Hi Arthur,

Update pull request per offline discussion, would you review this patch and give some feedback? thanks!
Attachment #8542773 - Attachment is obsolete: true
Attachment #8552332 - Flags: review?(arthur.chen)
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.
Attachment #8552332 - Flags: review?(arthur.chen)
(Assignee)

Comment 12

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

Hi Arthur, 
I've revised the patch, would you review this patch again? thanks!
Attachment #8552332 - Flags: review?(arthur.chen)
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.
Attachment #8552332 - Flags: review?(arthur.chen)
(Assignee)

Comment 14

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

Hi Arthur, would you review this patch again? thanks!
Attachment #8552332 - Flags: review?(arthur.chen)
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.
Attachment #8552332 - Flags: review?(arthur.chen)
(Assignee)

Comment 16

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

Add assertion for 'ril.cf.enabled', would you mind to review it again? thanks!
Attachment #8552332 - Flags: review?(arthur.chen)
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.
Attachment #8552332 - Flags: review?(arthur.chen)
Attachment #8552332 - Flags: review?(alive)
Attachment #8552332 - Flags: review+
(Assignee)

Comment 18

4 years ago
Unit tests updated and comment addressed, thanks Arthur :)
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.
Attachment #8552332 - Flags: review?(alive) → review+
(Assignee)

Comment 20

4 years ago
master: https://github.com/mozilla-b2g/gaia/commit/ba613ae583a706131c45e885f65d428d4a541a81
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
status-b2g-v2.0: --- → ?
status-b2g-v2.0M: --- → ?
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
(Assignee)

Comment 21

4 years ago
Jinghua, could you try this patch (attachment 8552332 [details] [review]) and verify the problem is fixed?
Flags: needinfo?(Jinghua.Xing)
(Assignee)

Comment 22

4 years ago
We should also verify this on master before requesting uplift.
Keywords: verifyme

Comment 23

4 years ago
(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.
Flags: needinfo?(Jinghua.Xing)

Comment 24

4 years ago
(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

4 years ago
Created attachment 8555821 [details] [diff] [review]
7715_v2.1.patch
Attachment #8555821 - Flags: review?(shchen)

Comment 26

4 years ago
chen, please help to review, thanks
Flags: needinfo?(chens)
(Assignee)

Comment 27

4 years ago
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?
Flags: needinfo?(chens)
Attachment #8555821 - Flags: review?(chens) → feedback-

Comment 28

4 years ago
(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.
(Assignee)

Comment 29

4 years ago
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.
Attachment #8552332 - Flags: approval-gaia-v2.2?
Attachment #8552332 - Flags: approval-gaia-v2.1?

Updated

4 years ago
blocking-b2g: --- → 2.1+

Updated

4 years ago
Attachment #8552332 - Flags: approval-gaia-v2.2?
Attachment #8552332 - Flags: approval-gaia-v2.2+
Attachment #8552332 - Flags: approval-gaia-v2.1?
Attachment #8552332 - Flags: approval-gaia-v2.1+
v2.2: https://github.com/mozilla-b2g/gaia/commit/5b52511b028506d5fbe83651cc9dfe7460f0fbe8
v2.1: https://github.com/mozilla-b2g/gaia/commit/5f6703f35ec8ed4ee5c377eaf03839c4ea0c77fd
status-b2g-v2.1: affected → fixed
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S5 (6feb)

Updated

4 years ago
status-b2g-v2.1S: affected → fixed
You need to log in before you can comment on or make changes to this bug.