Closed Bug 984196 Opened 6 years ago Closed 6 years ago

glxtest should use _exit instead of exit

Categories

(Core :: Graphics, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dbaron, Assigned: vikstrous)

References

Details

Attachments

(1 file, 2 obsolete files)

(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.
(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
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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!
Attachment #8391989 - Flags: review?(bgirard) → review+
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).
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
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.
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
I think this might still be needed. Viktor can you do the follow up.
Status: RESOLVED → REOPENED
Flags: needinfo?(me)
Resolution: INVALID → ---
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?
Flags: needinfo?(bgirard)
Assignee: dbaron → vstanchev
(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.
Flags: needinfo?(bgirard)
Attached patch glx2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7da9cb0eb3d9
Attachment #8391989 - Attachment is obsolete: true
Attachment #8393099 - Flags: review?(bgirard)
Oops didn't select debug builds too: https://tbpl.mozilla.org/?tree=Try&rev=7c2c0ebcb88c
Attachment #8393099 - Flags: review?(bgirard) → review+
Flags: needinfo?(bgirard)
Summary: Linux debug builds hang on startup because glxtest triggers ah_crap_handler and hangs for 300 seconds → glxtest should use _exit instead of exit
This is on my outbound queue. Waiting for inbound to open.
Keywords: checkin-needed
Attached patch glx22Splinter Review
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
Attachment #8393099 - Attachment is obsolete: true
Attachment #8397220 - Flags: review?(bgirard)
Attachment #8397220 - Flags: review?(bgirard) → review+
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/7dbe736c0e58
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Flags: needinfo?(me)
You need to log in before you can comment on or make changes to this bug.