984196 - glxtest should use _exit instead of exit

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(This bug might not have been observable for a bit while bug 97906 existed, so might not have shown up in testing of bug 981126.)

When I start up a debug build on Linux (in a VNC connection, possibly this is specific to running over VNC, but I'm not sure), the browser hangs.

The main Firefox process is doing this:

#0  0x00007f769504188d in __libc_waitpid (pid=<optimized out>, 
    stat_loc=<optimized out>, options=<optimized out>)
    at ../sysdeps/unix/sysv/linux/waitpid.c:41
#1  0x00007f768fee2fdf in mozilla::widget::GfxInfo::GetData (this=0x1ec2f20)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/widget/xpwidgets/GfxInfoX11.cpp:87
#2  0x00007f768fede759 in nsBaseWidget::ComputeShouldAccelerate (this=0x1be8330, 
    aDefault=false)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/widget/xpwidgets/nsBaseWidget.cpp:884
#3  0x00007f768fede23c in nsBaseWidget::GetLayerManager (this=<optimized out>, 
    aShadowManager=0x0, aBackendHint=<optimized out>, 
    aPersistence=<optimized out>, aAllowRetaining=0x7fff9b9dff0f)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/widget/xpwidgets/nsBaseWidget.cpp:1025
...

And the glxtest process is doing this:

#0  0x00007f769432a08d in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x00007f7694329f2c in __sleep (seconds=0)
    at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007f7690b2e004 in ah_crap_handler (signum=6)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsSigHandlers.cpp:90
#3  0x00007f7690b2f060 in exit (status=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsSigHandlers.cpp:110
#4  0x00007f7690b2f0d5 in fire_glxtest_process ()
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/glxtest.cpp:270
#5  0x00007f7690b262f9 in XREMain::XRE_mainInit (this=0x7fff9b9e0890, aExitFlag=
    0x7fff9b9e085f)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:2859
#6  0x00007f7690b2a80b in XREMain::XRE_main (this=0x7fff9b9e0890, argc=4, 
    argv=<optimized out>, aAppData=0x7fff9b9e0a40)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:4059
#7  0x00007f7690b2ab04 in XRE_main (argc=4, argv=0x7fff9b9e1da8, aAppData=
    0x7fff9b9e0a40, aFlags=<optimized out>)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:4291
#8  0x00000000004035bb in do_main (argc=4, argv=0x7fff9b9e1da8, xreDirectory=
    0x773010)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:282
#9  0x0000000000403a7c in main (argc=4, argv=0x7fff9b9e1da8)
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:643


I think this should be using _exit() rather than exit() like the other uses in glxtest, so that this doesn't happen.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #0)
> (This bug might not have been observable for a bit while bug 97906 existed,

... er, while bug 979069 existed

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Again use _exit() rather than exit() in glxtest to avoid hang in debug builds

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8391989 [details] [diff] [review]
Again use _exit() rather than exit() in glxtest to avoid hang in debug builds

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

*reads up on the difference between _exit vs. exit*

Ok cool. Thanks for the patch!

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, this was actually the result of a local patch I had to trap exit with our crash hooks; I'll merge this into that patch (which I'd forgotten about, but should probably land at some point).

Comment 5

[Benoit Girard (:BenWa)]

I couldn't find a credible source but common sources like stackoverflow and other Q&A forums seem to indicate that calling exit in the child can lead to temp files being deleted, buffers being flushed twice amongst other things. That we should use _exit.

Are you sure we don't still want this even if it didn't cause your problem? Viktor can do the follow up if we agree that we still want this.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure; I ended up merging it into the patch at https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/5f375e3858e7/trap-exit which I had locally to debug an unexpected exit() call at one point, but never landed.

Comment 7

[Benoit Girard (:BenWa)]

I think this might still be needed. Viktor can you do the follow up.

Comment 8

[Viktor Stanchev [:vikstrous]]

I guess this is the relevant stack overflow question: http://stackoverflow.com/questions/2329640/how-to-exit-a-child-process-exit-vs-exit

It seems to me that we still want to use _exit() because the exit is in a fork. The communication it does over a pipe with the parent process is done without buffers, so it should be safe to use _exit for all of the exit calls in the child. I can make a patch for this. I would push this to try first just in case though... Which tests make sense to run on try to make sure this didn't break anything?

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Viktor Stanchev [:vikstrous] from comment #8)
> it should be safe to use _exit for all of the exit calls
> in the child.

just the glxtest exit(s) is fine. Unless we can find other problematic areas.

> I can make a patch for this. I would push this to try first
> just in case though... Which tests make sense to run on try to make sure
> this didn't break anything?

I don't know. Maybe all the linux desktop tests.

Comment 10

[Viktor Stanchev [:vikstrous]]

Attachment: glx2

https://tbpl.mozilla.org/?tree=Try&rev=7da9cb0eb3d9

Comment 11

[Viktor Stanchev [:vikstrous]]

Oops didn't select debug builds too: https://tbpl.mozilla.org/?tree=Try&rev=7c2c0ebcb88c

Comment 12

[Benoit Girard (:BenWa)]

This is on my outbound queue. Waiting for inbound to open.

Comment 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba0551dd35d

Comment 14

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

I had to back this out in http://hg.mozilla.org/integration/mozilla-inbound/rev/4d5074dfad5d for breaking Valgrind builds:

https://tbpl.mozilla.org/php/getParsedLog.php?id=36628765&tree=Mozilla-Inbound

Comment 15

[Viktor Stanchev [:vikstrous]]

Attachment: glx22

I had to somehow prevent name mangling to be able to match the function name in the valgrind error suppression config, so I used extern "C".

https://tbpl.mozilla.org/?tree=Try&rev=63bc11258143

Comment 16

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbe736c0e58

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7dbe736c0e58