1154411 - Stability test uncovered attempted to close invalid file descriptor

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Stability, defect)

Description

[Greg Grisco]

CAF debug code checks for attempt to close invalid fd and is aborting when detecting this.  This instance happened during stability test.

cafbot will upload the logs.

Comment 1

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 2

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 3

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129
Moz BuildID: 20150409002503
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=ea735c21bfb0d78333213ff0376fce1eac89ead6
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=ac12d858a58574b2db482b5eed61750a3e210040
Patches: bug 1150923, bug 1133147, bug 1150271, bug 1148641, bug 1150924

Comment 4

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129

Comment 5

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129

Comment 6

[bhavana bajaj [:bajaj]]

sotaro/:fabrice, You guys helped in the past on identifying where the invalid fd close is happening, hence NI'ing you for help. Apparently, CAF has seen abort this ~14 times in the last run.

Comment 7

[Sotaro Ikeda [:sotaro]]

Okey, tomorrow, I am going to check if the crash has an useful information.

Comment 8

[Keven Kuo [:kkuo]]

Hi! Rex,

Need your help to take a look first. Thanks

--
Keven

Comment 9

[Rex Hung[:rhung]]

According to the minidump result, I though that both this bug and bug 1154416 have the same issue:
"Try to close a BAD file descriptor".

Why and where this BAD file descriptor comes from still needs to be investigated, but originally closing the BAD file descriptor should not crash the process, but just returns an error code for the reason.

However, both of call stack after calling the system call, close(), are really weird to me and need someone to provide the answer if possible:
1. Where is libnoselect.so from?
I locally synced source tree based on "caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129.xml", but cannot find this shared library. Is there any patch file not available for us?

2. Why does libnoselect.so catch the system call, close(), and trigger an abortion further for this case? Originally, libc.so, not libnoselect.so, should be used after calling the system call wraper function, __wrap_close(). So, is there any extra patch files here?

3. Based on the following call stack, it seems that the different version of libnoselect.so are used because different functions are called to trigger an abortion. Can anyone tell me the purpose of this shared library(libnoselect.so)?
Bug#1154411
 0  plugin-container!mozalloc_abort [mozalloc_abort.cpp : 37 + 0x4]
                    ^^^^^^^^^^^^^^^
 1  libnoselect.so!close [noselect.c : 89 + 0x3]
 2  libmozglue.so!__wrap_close [Nuwa.cpp : 1360 + 0x3]
 3  libnss3.so!pt_Close [ptio.c : 1252 + 0x5]
Bug#1154416
 0  libc.so!tgkill + 0xc
 1  libc.so!pthread_kill [pthread_kill.cpp : 49 + 0xb]
 2  libc.so!raise [raise.cpp : 32 + 0x9]
 3  libc.so!__libc_android_abort [abort.cpp : 47 + 0x5]
 4  libc.so!abort + 0x6
            ^^^^^
 5  libnoselect.so!close [noselect.c : 89 + 0x3]
 6  libmozglue.so!__wrap_close [Nuwa.cpp : 1360 + 0x3]
 7  libnss3.so!pt_Close [ptio.c : 1252 + 0x5]

Comment 10

[Rex Hung[:rhung]]

In short, libnoselect.so is the root cause which results in process crash, but we have no idea for it so far. Need someone help to clarify the purpose of libnoselect.so first.

Comment 11

[Rex Hung[:rhung]]

Hi, Greg,

Could you check comment 9 and give us some feedback if possible? Thank you.

Comment 12

[Greg Grisco]

(In reply to Rex Hung[:rhung] from comment #9)
> According to the minidump result, I though that both this bug and bug
> 1154416 have the same issue:
> "Try to close a BAD file descriptor".
> 
> Why and where this BAD file descriptor comes from still needs to be
> investigated, but originally closing the BAD file descriptor should not
> crash the process, but just returns an error code for the reason.
>
> However, both of call stack after calling the system call, close(), are
> really weird to me and need someone to provide the answer if possible:
> 1. Where is libnoselect.so from?
> I locally synced source tree based on
> "caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.129.xml", but cannot find this
> shared library. Is there any patch file not available for us?

In internal builds, we are overriding the close() function and check for invalid file descriptors and abort() when this situation is detected.  

> 2. Why does libnoselect.so catch the system call, close(), and trigger an
> abortion further for this case? Originally, libc.so, not libnoselect.so,
> should be used after calling the system call wraper function,
> __wrap_close(). So, is there any extra patch files here?
> 

We do this in hopes that this will catch problems that would otherwise be much more difficult to debug.  The change is an internal one, but basically we call the system close() and if the return value is < 0 and fd is not -1 and errno is not ENODEV then we abort.

> 3. Based on the following call stack, it seems that the different version of
> libnoselect.so are used because different functions are called to trigger an
> abortion. Can anyone tell me the purpose of this shared
> library(libnoselect.so)?
> Bug#1154411
>  0  plugin-container!mozalloc_abort [mozalloc_abort.cpp : 37 + 0x4]
>                     ^^^^^^^^^^^^^^^
>  1  libnoselect.so!close [noselect.c : 89 + 0x3]
>  2  libmozglue.so!__wrap_close [Nuwa.cpp : 1360 + 0x3]
>  3  libnss3.so!pt_Close [ptio.c : 1252 + 0x5]
> Bug#1154416
>  0  libc.so!tgkill + 0xc
>  1  libc.so!pthread_kill [pthread_kill.cpp : 49 + 0xb]
>  2  libc.so!raise [raise.cpp : 32 + 0x9]
>  3  libc.so!__libc_android_abort [abort.cpp : 47 + 0x5]
>  4  libc.so!abort + 0x6
>             ^^^^^
>  5  libnoselect.so!close [noselect.c : 89 + 0x3]
>  6  libmozglue.so!__wrap_close [Nuwa.cpp : 1360 + 0x3]
>  7  libnss3.so!pt_Close [ptio.c : 1252 + 0x5]

Comment 13

[Greg Grisco]

BTW, we recently turned this check back on (after discovering it had been turned off for some time).  So this particular crash might not be catching a new regression.

Comment 14

[[:fabrice] Fabrice Desré]

Greg, any chance you would share your noselect library? I'd like moz to run our tests with it (or something similar). thanks!

Comment 15

[Greg Grisco]

Sorry, Fabrice, but this library can't be shared.  However, I've pretty much described the close() handling in comment 12 if you want to try to implement something similar.

Comment 16

[Sotaro Ikeda [:sotaro]]

I confirmed it is a problem of ArrayBufferBuilder::mapToFileInPackage(). one fd is closed twice by the following. Then it seems regression of Bug 945152.

>  mozilla::AutoFDClose pr_fd;
>  mozilla::ScopedClose fd;

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#4082

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: log patch - Add some logs around fd close and ArrayBufferBuilder::mapToFileInPackage()

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: gaia patch - Do not compress files

Comment 19

[Sotaro Ikeda [:sotaro]]

By the following STR, I reproduced the problem.

[1] Created ROM by applying attachment 8593608 [details] [diff] [review] and flash the ROM.
      It prevent file compression. mapToFileInPackage() is used only when zipItem is not compressed.
      https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#4079
[2]  Enable Chinese keyboard by the following.
     Setting -> Keyboards -> 拼音
[3] Start Browser and touch "search or enter address".
     Keyborad should pop up.
[4] Change to Chinese keyboard and type something.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Do not close same fd twice

Comment 21

[Sotaro Ikeda [:sotaro]]

Greg, can you check if attachment 8593613 [details] [diff] [review] fix the problem?

Comment 22

[Greg Grisco]

Ok, I'm bringing this in now, thanks.  We'll probably won't see results until Monday.

Comment 23

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 24

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132
Moz BuildID: 20150413002502
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=cec00d643f517ffd96cde559cd3bbd43ab85816c
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=a47c1908cca4b834ae92e9a0507f09da46993f8b
Patches: bug 1150923, bug 1154439, bug 1133147, bug 1142806, bug 1150924

Comment 25

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132

Comment 26

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132

Comment 27

[Sotaro Ikeda [:sotaro]]

(In reply to cafbot (PoC: ggrisco) from comment #26)
> Created attachment 8593792 [details]
> decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132

It seems not applying attachment 8593613 [details] [diff] [review].

Comment 28

[Bobby Chien]

ni Greg per comment 27.

Comment 29

[Greg Grisco]

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> (In reply to cafbot (PoC: ggrisco) from comment #26)
> > Created attachment 8593792 [details]
> > decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.132
> 
> It seems not applying attachment 8593613 [details] [diff] [review].

We have to wait for AU 134.  I thought it would be picked up for testing on Sunday, but there was a slight delay.  I'll update you by tomorrow hopefully.

Comment 31

[Greg Grisco]

Hi Sotaro,

We haven't seen this crash in a full day of testing on AU134, so I think we definitely want this fix, thanks.

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Greg Grisco from comment #31)
> Hi Sotaro,
> 
> We haven't seen this crash in a full day of testing on AU134, so I think we
> definitely want this fix, thanks.

Good! I am going to proceed to review.

Comment 33

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c893d853b23

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8593613 [details] [diff] [review]
patch - Do not close same fd twice

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

::: dom/base/nsXMLHttpRequest.cpp
@@ +4124,1 @@
>      mMapPtr = JS_CreateMappedArrayBufferContents(fd, offset, size);

This looks fine, but can you just do:

  mozilla::AutoFDClose pr_fd;
  rv = aJarFile->OpenNSPRFileDesc(PR_RDONLY, 0, &pr_fd.rwget());
  // ...
  mMapPtr =
    JS_CreateMappedArrayBufferContents(PR_FileDesc2NativeHandle(pr_fd),
                                       offset,
                                       size);

Not having another fd on the stack seems simpler to reason about.

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And let's please backport this as far as we can.

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #34)
> Comment on attachment 8593613 [details] [diff] [review]
> patch - Do not close same fd twice
> 
> Review of attachment 8593613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsXMLHttpRequest.cpp
> @@ +4124,1 @@
> >      mMapPtr = JS_CreateMappedArrayBufferContents(fd, offset, size);
> 
> This looks fine, but can you just do:
> 
>   mozilla::AutoFDClose pr_fd;
>   rv = aJarFile->OpenNSPRFileDesc(PR_RDONLY, 0, &pr_fd.rwget());
>   // ...
>   mMapPtr =
>     JS_CreateMappedArrayBufferContents(PR_FileDesc2NativeHandle(pr_fd),
>                                        offset,
>                                        size);
> 
> Not having another fd on the stack seems simpler to reason about.

Yeah, it is simpler. I'll update the patch.

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Do not close same fd twice

Apply the comment. Carry "r=bent".

Comment 38

[Sotaro Ikeda [:sotaro]]

swu, do you know if ArrayBufferBuilder::mapToFileInPackage() is used on desktop Firefox?

Comment 39

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc42f95d0480

Comment 40

[Shian-Yow Wu [:swu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> swu, do you know if ArrayBufferBuilder::mapToFileInPackage() is used on
> desktop Firefox?

No, it's only enabled on B2G.
https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#1094

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to Shian-Yow Wu [:swu] from comment #40)
> 
> No, it's only enabled on B2G.
> https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#1094

Thanks!

Comment 42

[Sotaro Ikeda [:sotaro]]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8596721 [details] [diff] [review]
patch - Do not close same fd twice

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 945152
User impact if declined: System could cause a crash with unexpected reasons.
Testing completed: locally tested and codeaurora did tests.
Risk to taking this patch (and alternatives if risky): low risk.
String or UUID changes made by this patch: none.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ed10c7a4ddd8