Last Comment Bug 772643 - Chrome hang monitor can deadlock the browser
: Chrome hang monitor can deadlock the browser
Status: RESOLVED FIXED
[ironic]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-10 14:04 PDT by Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
Modified: 2012-07-15 18:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't allocate memory while a thread is suspended (1.10 KB, patch)
2012-07-10 15:03 PDT, Vladan Djeric (:vladan)
ehsan: review+
vladan.bugzilla: checkin+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-07-10 14:04:17 PDT
The chrome hang monitor suspends the hung thread, walks its stack, and then resumes the thread (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp#122).

This falls down if the suspended thread holds the allocator lock.  When the stack walker tries to malloc at http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp#114, we deadlock.  I hit this on my local machine today.
Comment 1 Vladan Djeric (:vladan) 2012-07-10 15:03:09 PDT
Created attachment 640811 [details] [diff] [review]
Don't allocate memory while a thread is suspended
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-10 15:09:35 PDT
Comment on attachment 640811 [details] [diff] [review]
Don't allocate memory while a thread is suspended

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

::: xpcom/threads/HangMonitor.cpp
@@ +123,5 @@
>    MOZ_ASSERT(winMainThreadHandle);
> +
> +  // The thread we're about to suspend might have the alloc lock
> +  // so allocate ahead of time
> +  callStack.SetCapacity(400);

Nit: Please make this a static const with a sensible name and put it perhaps after line 23.
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-10 20:30:58 PDT
https://hg.mozilla.org/mozilla-central/rev/45c7c796fd81

Note You need to log in before you can comment on or make changes to this bug.