993821 - [Wifi] [Follow up of Bug 986365] Event thread is blocked in WaitForEvent caused wifi stay in searching state

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Comment 1

[Vincent Chang[:vchang][changyihsin]]

This is found when I doing the gaia ui test. 
STR, 
1. turn on wifi and connect to an AP. 
2. toggle settings to disable wifi. 
3. toggle settings to enabled wifi. 
repeat step 2 and 3 for about 400 times.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

The root cause here is
1. When disable wifi, the last event we should receive from wpa_supplicant is 
CTRL-EVENT-TERMINATING. Somehow wpa_supplicant doesn't send it. 
2. It causes event thread waiting in poll function in wifi.c. 
http://androidxref.com/4.0.4/xref/hardware/libhardware_legacy/wifi/wifi.c#662
3. Next time you turn on wifi, the connectToSupplicant command creates new monitor and control sockets and use them to communicate with wpa_supplicant. However, event thread still block on old socket.

Comment 3

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

Comment on attachment 8403743 [details] [diff] [review]
Patch v1.0

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

Please refine or remove the debug code, others looks good, thank you!

::: dom/wifi/WifiProxyService.cpp
@@ +16,5 @@
>  using namespace mozilla;
>  using namespace mozilla::dom;
>  
> +#define xxx(args...)  __android_log_print(ANDROID_LOG_INFO, "@@@", args)
> +

nit: "xxx" and "@@@" might not be a proper debug function name/debug message prefix.
If we need to keep these message for debug purpose, we should rename them, add a control flag, and disable them by default.

@@ +202,5 @@
>    mEventThreadList.SetLength(aNumOfInterfaces);
>    for (uint32_t i = 0; i < aNumOfInterfaces; i++) {
>      mEventThreadList[i].mInterface = aInterfaces[i];
>      rv = NS_NewThread(getter_AddRefs(mEventThreadList[i].mThread));
> +    NS_SetThreadName(mEventThreadList[i].mThread, "WifiE");

We usually don't set thread name, and it's seems thread name is not required to resolve the bug.
If we like to keep these names, maybe WifiEventThread/WifiCtrlThread is more friendly, since the max length is 15 chars.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1 r=chulee

Address the review comments.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1 for 1.3 branch

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Nominate for 1.3 ? I found this when doing the test after apply the patch in Bug 986365 which is a 1.3 blocker. We should apply this to prevent breaking wifi function.

Comment 7

[Vincent Chang[:vchang][changyihsin]]

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

Comment 8

[Tony Chung [:tchung]]

upgrading this to 1.4+.   1.3 branch is not getting any new automation at this point.   please land this on 1.4 branch, thanks!

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9aa8010a173a

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Vincent Chang[:vchang] from comment #6)
> Nominate for 1.3 ? I found this when doing the test after apply the patch in
> Bug 986365 which is a 1.3 blocker. We should apply this to prevent breaking
> wifi function.

(In reply to Tony Chung [:tchung] from comment #8)
> upgrading this to 1.4+.   1.3 branch is not getting any new automation at
> this point.   please land this on 1.4 branch, thanks!

Making this a 1.3+  based on comment #6. IN short we have to uplift https://bugzilla.mozilla.org/show_bug.cgi?id=986365 and by doing so we may break wifi unless we uplift this.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request b2g28 approval for v1.3 uplift on this ASAP.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8404517 [details] [diff] [review]
Patch v1.1 for 1.3 branch

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is found after applied patch in Bug 986365.
User impact if declined: Toggle wifi settings on/off for about 4 hours causes event thread blocked, we need to reboot the device to make wifi work again. 
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): No
String or IDL/UUID changes made by this patch: No

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/fd2d08c0d889