1113416 - Hang monitor + on-demand decompression can cause deadlocks

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

I believe this is the real cause behind bug 1059797.

Basically when a thread hangs, the hang monitor triggers a sighandler and reads the thread's profiler pseudostack, which may contain string constants in static storage.

However, if the page containing the constant hasn't been decompressed yet, on-demand decompression will kick in to load the string constant.

If the original hang signal happened during a memory allocation, on-demand decompression will try to allocate memory again, and cause a deadlock when trying to take a lock that's already taken.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1113416 - Don't read stack labels inside hang monitor sighandler;

When we're inside the hang monitor's signal handler, we must not read any string labels. Doing so may result in on-demand decompression kicking in on Android, which may result in a deadlock.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8544767 [details] [diff] [review]
Bug 1113416 - Don't read stack labels inside hang monitor sighandler;

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

This is some crazy stuff.

Comment 3

[Mike Hommey [:glandium]]

This makes so much more sense :)

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8544767 [details] [diff] [review]
Bug 1113416 - Don't read stack labels inside hang monitor sighandler;

That said, this should be reviewed by a peer of the xpcom module.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8544767 [details] [diff] [review]
Bug 1113416 - Don't read stack labels inside hang monitor sighandler;

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

I don't know what the stacks in BHM usually look like, so I'm curious about your thoughts on the comment below.

::: xpcom/threads/BackgroundHangMonitor.cpp
@@ +362,5 @@
>  
> +  // Remove unwanted "js::RunScript" frame from the stack
> +  for (const char** f = &mHangStack.back(); f >= mHangStack.begin(); f--) {
> +    if (!mHangStack.IsInBuffer(*f) && !strcmp(*f, "js::RunScript")) {
> +      mHangStack.erase(f);

My only concern is that we now have this O(N^2)-ish algorithm here.  Maybe it doesn't matter that much, as we *probably* won't have many RunScript entries and those that we do are likely to be close to the end, so the copying is minimal?

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> Comment on attachment 8544767 [details] [diff] [review]
> Bug 1113416 - Don't read stack labels inside hang monitor sighandler;
> 
> Review of attachment 8544767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know what the stacks in BHM usually look like, so I'm curious about
> your thoughts on the comment below.
> 
> ::: xpcom/threads/BackgroundHangMonitor.cpp
> @@ +362,5 @@
> >  
> > +  // Remove unwanted "js::RunScript" frame from the stack
> > +  for (const char** f = &mHangStack.back(); f >= mHangStack.begin(); f--) {
> > +    if (!mHangStack.IsInBuffer(*f) && !strcmp(*f, "js::RunScript")) {
> > +      mHangStack.erase(f);
> 
> My only concern is that we now have this O(N^2)-ish algorithm here.  Maybe
> it doesn't matter that much, as we *probably* won't have many RunScript
> entries and those that we do are likely to be close to the end, so the
> copying is minimal?

Right, stacks are usually less than 10 entries and RunScript only appears for JS entries near the end of the stack. And this block only runs infrequently because it only runs when there is a hang happening on a thread.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbf0b1ec001

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cbf0b1ec001