1055380 - [Tarako]Sometimes UI doesn't get updated correctly after switching between two APs several times

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Zhenqing Liu]

STR:
  1. Connect to one AP.
  2. Connect to another AP in the list.
  3. Reconnect to the first AP and then the second one.
  4. Repeat Step 3 and sometimes UI will not show the right connection state.

Comment 1

[Zhenqing Liu]

When this bug appears, the wpa_supplicant has connected to AP but UI doesn't get updated correctly.

I have checked with our supplicant engineer. Root cause is gecko only use |AP_SCAN 1| to control supplicant. So supplicant will try to connect to other available APs according to its own policy.
And when someone taps the same AP which supplicant is trying to connect to, this bug will probably come out.

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Can't reproduce on flame 2.1, I think set AP_SCAN to 1 have nothing to do with such issue.
The cause described in comment 1 also doesn't match the STR, wpa_supplicant shouldn't trigger auto connection while it's already connecting to an AP.

Comment 3

[Zhenqing Liu]

According to our supplicant engineer, only setting AP_SCAN to 2 after connection can stop supplicant from trying to connect to another AP.

If we only set AP_SCAN to 1 and tap the same AP supplicant trying to connect to,  |CTRL-EVENT-STATE-CHANGE id=-1 state=0| related to the tapping will not be notified. So gecko state machine will not get updated and UI will not get updated accordingly.

I comment out these lines in WifiWorker.js

 if (manager.state === "COMPLETED" &&
        fields.state !== "DISCONNECTED" &&
        fields.state !== "INTERFACE_DISABLED" &&
        fields.state !== "INACTIVE" &&
        fields.state !== "SCANNING") {
      return false;
    }

and give the patch to our custom, the bug never appears.

Comment 4

[Zhenqing Liu]

Attachment: logs

Bug appeared at the last connection.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I don't think its related to value of AP_SCAN. It seems to be the reassociate mechanism is triggered, by ENABLE_NETWORK or RECONNECT or both, makes wpa_supplicant tend to connect to network with higher priority(Phone-WIFI is 8 while cisco2.4G is 10).
WifiWorker seems to prevent this behavior by giving highest priority to the network assigned to connect by API, but the log shows different.
I think the root cause is priority management, and will check how to fix it.

On the other hand, the reassociate behavior is not taken into consideration in current state machine.
We need to support such state transition, instead of comment out the state transition check.

Comment 6

[Zhenqing Liu]

I was just trying to make a workaround. The perfect solution still relies on you.

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type.

|network.priority| should be integer type but is read as string type in |getNetworkConfiguration()|, and leads to wrong highest priority value because the comparison in |_reloadConfiguredNetworks()|[1] will be compared based on string type instead of integer. 
Now we provide type information for each network config field, and cast them into correct type upon read. So we can get correct highest priority value and resolve value issue described in comment 5.

[1] http://hg.mozilla.org/mozilla-central/file/cd2acc7ab2f8/dom/wifi/WifiWorker.js#l2521

Comment 8

[Henry Chang [:hchang]]

(In reply to Chuck Lee [:chucklee] from comment #7)
> Created attachment 8477255 [details] [diff] [review]
> Cast network configs into correct data type.
> 
> |network.priority| should be integer type but is read as string type in
> |getNetworkConfiguration()|, and leads to wrong highest priority value
> because the comparison in |_reloadConfiguredNetworks()|[1] will be compared
> based on string type instead of integer. 
> Now we provide type information for each network config field, and cast them
> into correct type upon read. So we can get correct highest priority value
> and resolve value issue described in comment 5.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/cd2acc7ab2f8/dom/wifi/WifiWorker.
> js#l2521

I am positive to tagging type info to the parsing table.
Just want to know if it would be easier to parse to integer before
comparison? Not sure if there's anything I am not aware of.

If you choose to parse to integer before comparison, we'd better
of using |network.hasOwnProperty('priority')| rather than 
if (network.priority && ...) at [1] since the falsy |network.priority| 0
may still be meaningful.

Thanks!

[1] http://hg.mozilla.org/mozilla-central/file/cd2acc7ab2f8/dom/wifi/WifiWorker.js#l2521

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Henry Chang [:henry] from comment #8)
> (In reply to Chuck Lee [:chucklee] from comment #7)
> > Created attachment 8477255 [details] [diff] [review]
> > Cast network configs into correct data type.
> > 
> > |network.priority| should be integer type but is read as string type in
> > |getNetworkConfiguration()|, and leads to wrong highest priority value
> > because the comparison in |_reloadConfiguredNetworks()|[1] will be compared
> > based on string type instead of integer. 
> > Now we provide type information for each network config field, and cast them
> > into correct type upon read. So we can get correct highest priority value
> > and resolve value issue described in comment 5.
> > 
> > [1]
> > http://hg.mozilla.org/mozilla-central/file/cd2acc7ab2f8/dom/wifi/WifiWorker.
> > js#l2521
> 
> I am positive to tagging type info to the parsing table.
> Just want to know if it would be easier to parse to integer before
> comparison? Not sure if there's anything I am not aware of.

I think it's more programmer-friendly to provide data in expected type for later use, and prevent cause bug like this one if programmer forget to do the cast.
 
> If you choose to parse to integer before comparison, we'd better
> of using |network.hasOwnProperty('priority')| rather than 
> if (network.priority && ...) at [1] since the falsy |network.priority| 0
> may still be meaningful.
> 
> Thanks!

Thank you for pointing this out!

Comment 10

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type. V2

Address comment 8 and fix a priority re-prioritize bug found while testing.

Comment 11

[Henry Chang [:hchang]]

Comment on attachment 8478941 [details] [diff] [review]
Cast network configs into correct data type. V2

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

This looks good and have no other concerns. Thanks!

Comment 12

[Zhenqing Liu]

Hi Chuck, I have tried this patch on v1.3t and Wifi can't connect to AP. This line will be printed from |adb logcat|.

08-28 05:54:36.110 E/GeckoConsole(   86): [JavaScript Error: "configured is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/WifiWorker.js" line: 2789}]

Could you help give a patch on v1.3t?

Comment 13

[Zhenqing Liu]

Patching to v1.4 will have the same result as v1.3t .

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type. V3. r=hchang

Fix error reported in comment 12.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type. V4. r=hchang

Fix type of configuration "disabled"

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type (1.3t)

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type (1.4)

Comment 18

[Zhenqing Liu]

Hi Chuck,
Glad to see WIFI can connect to AP normally with the new patch. But there is still a problem.
Touch |search again| button after connecting to an AP and we will get the following error from |adb logcat|:

08-29 01:56:17.260 E/GeckoConsole(   86): [JavaScript Error: "priority is not defined" {file: "jar:file:///system/b2g/omni.ja!/components/WifiWorker.js" line: 2289}]

Related source line:   if (network.hasOwnProperty(priority) &&

Scan command will not be dispatched to supplicant. So UI keep saying"Seatching".
Same problem on v1.3t and v1.4.

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type. V5. r=hchang

Fix nit.

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type (1.3t)

Fix nit.

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Cast network configs into correct data type (1.4)

Fix nit.

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=1f6c764bba15

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c80f5c1fd92d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c80f5c1fd92d