1061993 - Closing an invalid fd in WiFi

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[bhargavg1]

When using the patch from Bug 1057220 to catch all invalid fds being closed, we see a crash when operating the WiFi module

You can see the point of crash in the logs as, 

libc    : close() ret -1 errno -9 fd 213

In most of the cases when the crash happens see,

D DHCP    : failed to set prefixLength 0: Cannot assign requested address


this crash has been seen 10 times in automation

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We need a stack here.

Comment 2

[bhargavg1]

Attachment: 4f1ab763-c886-0107-766e939f-01103235.txt

parsed dump file

Comment 3

[bhargavg1]

Attachment: 4f1ab763-c886-0107-766e939f-01103235.extra

extra file at time of crash

Please look for "close()" string in the file for the exact point

Comment 4

[bhargavg1]

Attachment: logcat_radio_27-08-2014-20-12-57.log.txt

Finally the logcat

Comment 5

[bhargavg1]

Attachment: logcat_27-08-2014-20-12-57.log.txt.zip

Comment 6

[bhargavg1]

[Blocking Requested - why for this release]:

Comment 7

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

Suspect to be a race condition while calling ifc_* functions because they all share same variable |ifc_ctl_sock| while doing ioctl() instead of request socket on their own.

Comment 8

[Henry Chang [:hchang]]

There is a checkin-needed patch (see Bug 1038531) which may fix
the race condition by serializing all libnetutil command to one 
thread (NetworkWorker). We can rerun the automation test while
the patch lands.

Comment 9

[bhargavg1]

(In reply to Henry Chang [:henry] from comment #8)
> There is a checkin-needed patch (see Bug 1038531) which may fix
> the race condition by serializing all libnetutil command to one 
> thread (NetworkWorker). We can rerun the automation test while
> the patch lands.

Do you have a patch for v2.0? we can verify the same if available

Comment 10

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

There are two threads running ifc_* functions at same time after wifi disabled:
1. Network Worker thread [1]: ifc_act_on_ipv4_route()
2. Wifi Control thread [2]: ifc_configure() -> ifc_reset_connection()

The log shows crash happen in thread(30) running |close()| in |ifc_act_on_ipv4_route()|, while another thread(15) running |ioctl()| in |ifc_reset_connection()|.

Because ifc_* shares ifc_ctl_sock[3] between functions, the crash might be caused by ifc_act_on_ipv4_route() and ifc_configure() calls close() almost at same time, leaves no time for the set and check of ifc_ctl_sock in ifc_close()[4] to run.

Since ifc_* functions are not thread-safe, executing all ifc_* functions in single thread, as the patch mentioned in comment 8, might resolve this issue.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkWorker.cpp#226
[2] http://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiProxyService.cpp#269
[3] https://android.googlesource.com/platform/system/core/+/master/libnetutils/ifc_utils.c#58
[4] https://android.googlesource.com/platform/system/core/+/master/libnetutils/ifc_utils.c#148

Comment 11

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.076
Moz BuildID: 20140829000203
B2G Version: 2.0
Gecko Version: 32.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=5bf3b8cdea15e62ce7bf77a15085a18e24e33c44
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=f2ab70e9ab18439dae25e6eb21f186708b2888de

Comment 12

[Henry Chang [:hchang]]

(In reply to bhargavg1 from comment #9)
> (In reply to Henry Chang [:henry] from comment #8)
> > There is a checkin-needed patch (see Bug 1038531) which may fix
> > the race condition by serializing all libnetutil command to one 
> > thread (NetworkWorker). We can rerun the automation test while
> > the patch lands.
> 
> Do you have a patch for v2.0? we can verify the same if available

I believe the latest patch in Bug 1038531 cannot apply to 2.0. 
Maybe one of the obsoleted patch could (such as attachment 8462323 [details] [diff] [review])

Comment 13

[Henry Chang [:hchang]]

Attachment: Patch_for_V2_0.patch

Comment 14

[Inder]

(In reply to Henry Chang [:henry] from comment #13)
> Created attachment 8483383 [details] [diff] [review]
> Patch_for_V2_0.patch

This is a large change and i think too risky for 2.0 at this point. Can we have an alternate solution for 2.0 which is less risky?

Comment 15

[Henry Chang [:hchang]]

(In reply to Inder from comment #14)
> (In reply to Henry Chang [:henry] from comment #13)
> > Created attachment 8483383 [details] [diff] [review]
> > Patch_for_V2_0.patch
> 
> This is a large change and i think too risky for 2.0 at this point. Can we
> have an alternate solution for 2.0 which is less risky?

Probably moving only the function which causes the race condition
is enough. Will provide a lighter patch as soon as possible.

Comment 16

[Henry Chang [:hchang]]

A more lightweight solution: adding exclusive lock to ifc_* functions [1]

[1] http://hg.mozilla.org/mozilla-central/file/776fa9cf70cd/dom/network/NetUtils.cpp#l88

Comment 17

[Henry Chang [:hchang]]

Attachment: Less risky patch

Chuck,

Could you please take a look at this patch? I add a lock to ifc functions
to avoid race condition and it has lower rick to the release branch. Thansk!

Comment 18

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

Comment on attachment 8483928 [details] [diff] [review]
Less risky patch

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

Looks good, thanks!

::: dom/network/src/NetUtils.cpp
@@ +60,5 @@
>  DEFINE_DLFUNC(ifc_remove_host_routes, int32_t, const char*)
>  DEFINE_DLFUNC(ifc_remove_default_route, int32_t, const char*)
>  DEFINE_DLFUNC(dhcp_stop, int32_t, const char*)
>  
> +NetUtils::NetUtils() 

Nit: Extra space

Comment 19

[bhavana bajaj [:bajaj]]

CAF is picking this up today to test and ensure the issue is fixed. NI inder so it does not fall off the plate.

Comment 20

[Inder]

It was built into our AU: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.081. We will update with our findings shortly.

Comment 21

[Greg Grisco]

We want to see  if crash reproduces with "Less risky patch" and bionic close() debug patch

Comment 22

[Henry Chang [:hchang]]

Attachment: No-risk patch (r+'d) (for 2.0 only)

Removed the trailing white space and renamed the patch since it's not risky at all :p

Comment 23

[Henry Chang [:hchang]]

No news is good news!

Comment 24

[Inder]

Let's wait for final confirmation from our side. Adding ni on Greg to confim once we get results on this.

Comment 25

[howie [:howie]]

Hi Greg and Inder, any update on this one? Thank you.

Comment 26

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Howie,

We are not seeing this issue anymore in our stability test. Please go ahead and land this patch. We will create a new bug if it comes again :) .

Comment 27

[howie [:howie]]

Hi Henry, please help to land this on master first. After it's fixed, request approval flag: approval‑mozilla‑b2g32 (v2.0). If the patch also apply to v2.1, please request the following approval too: approval‑mozilla‑aurora.

Comment 28

[Henry Chang [:hchang]]

(In reply to howie [:howie] from comment #27)
> Hi Henry, please help to land this on master first. After it's fixed,
> request approval flag: approval‑mozilla‑b2g32 (v2.0). If the patch also
> apply to v2.1, please request the following approval too:
> approval‑mozilla‑aurora.

This patch will only be applied to branched releases since this issue
can be fixed whenever Bug 1038531 lands.

Comment 29

[Henry Chang [:hchang]]

Comment on attachment 8487039 [details] [diff] [review]
No-risk patch (r+'d) (for 2.0 only)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Wifi
User impact if declined: Switching wifi off may cause invalid fd to be closed.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): No 
String or UUID changes made by this patch: No

Comment 30

[Henry Chang [:hchang]]

Attachment: Bug1061993 (applicable for 2.1 and m-c)

Comment 31

[Henry Chang [:hchang]]

By the way, since the patch is not harmful to master, I also attached patch for m-c. 
Thanks.

Comment 32

[Henry Chang [:hchang]]

Comment on attachment 8487039 [details] [diff] [review]
No-risk patch (r+'d) (for 2.0 only)

Withdraw mozilla-aurora approval request since this patch needs to rebase to apply to mozilla-aurora

Comment 33

[Vincent Chang[:vchang][changyihsin]]

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ab68be61e8c

Comment 35

[howie [:howie]]

Comment on attachment 8487039 [details] [diff] [review]
No-risk patch (r+'d) (for 2.0 only)

Correct the nom in Comment 29 to approval‑mozilla‑b2g32 (v2.0)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

There needs to be an Aurora approval request for the v2.1 uplift as well.

Comment 37

[Inder]

Bhavana -- We need this fixed for 2.1 as well.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5cf783171d5c

(In reply to Inder from comment #37)
> Bhavana -- We need this fixed for 2.1 as well.

Needs an approval request first. See comment 35.

Comment 39

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/5cf783171d5c

Comment 40

[bhavana bajaj [:bajaj]]

Henry, can you please help with the needed aurora patch here?

Comment 41

[Ryan VanderMeulen [:RyanVM]]

(In reply to bhavana bajaj [:bajaj] from comment #40)
> Henry, can you please help with the needed aurora patch here?

The patch applies cleanly to Aurora. Just needs an approval request...

Comment 42

[Henry Chang [:hchang]]

Comment on attachment 8489894 [details] [diff] [review]
Bug1061993 (applicable for 2.1 and m-c)

Approval Request Comment
[Feature/regressing bug #]: Wifi
[User impact if declined]: Turning off wifi may cause to close invalid fd.
[Describe test coverage new/current, TBPL]: Bug 1057220 and wifi test cases
[Risks and why]: No
[String/UUID change made/needed]: No

Comment 43

[Henry Chang [:hchang]]

Sorry for late response. Already requested approval.
Thanks!

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ec3db1220e0

Comment 45

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.