522332 - nsProfileLock::FatalSignalHandler is not async-signal-safe

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Karl Tomlinson (:karlt)]

nsProfileLock::FatalSignalHandler()
 -> RemovePidLockFiles()
   -> lock->Unlock()
     -> free()

Perhaps either nsProfileLock could use a free-list instead of free, or perhaps special Unlock()ing is needed for signal handling.

(There also seems to be an assumption here that the handler will only be called on one thread.)

Comment 1

[Justin Lebar (not reading bugmail)]

Since it's a fatal signal, do we need to bother calling free()?

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Jesse, could you see if this patch fixes the hang you reported in bug 566208?

Comment 3

[Jesse Ruderman]

Comment on attachment 457722 [details] [diff] [review]
Patch v1

I can't reproduce my bug at will.  I tried various combinations of long and short timeouts, builds with and without breakpad.  Sorry I can't usefully test this patch.

Comment 4

[Jesse Ruderman]

Is it only in the killed-by-abort case that we skip the "free"?  Or does this introduce a leak into normal shutdown that I'll need to tell Valgrind to ignore?

Comment 5

[Jesse Ruderman]

Rather than "See bug 522332", I think the comment should explain exactly why it's not safe to call free() in this case, and what the implications of not calling free() are.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

This one should only leak on a fatal signal.  I dunno if that's worth informing Valgrind of.

Comment 7

[Jesse Ruderman]

Thanks.

We're going to "leak" almost everything when we get a fatal signal, so "leaking" one additional thing is fine.

Comment 8

[timeless]

+            if (!aFatalSignal)
+              free(mPidLockFileName);

free is under-indented

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to comment #8)
> free is under-indented

Fixed locally.  Thanks.

Comment 10

[Justin Lebar (not reading bugmail)]

Nominating as blocking 2.0 since this harms user experience when FF crashes -- it's much better for it to cleanly die than to hang.

Comment 11

[Dave Townsend [:mossop]]

On the fence really but I guess being able to kill Firefox cleanly is important.

Comment 12

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/52172131101f