Bug 141268

Summary: r179576 introduce a deadlock potential during GC thread suspension
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, gyuyoung.kim, mmirman, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch. msaboff: review+

Description Mark Lam 2015-02-04 15:24:35 PST
http://trac.webkit.org/r179576 introduced a potential for deadlocking.  In the GC thread suspension loop, we currently delete MachineThreads::Thread that we detect to be invalid.  This is unsafe because we may have already suspended some threads, and one of those suspended threads may still be holding the heap lock.  An attempt to do a deletion there may therefore result in a deadlock.

The fix is to put to the invalid threads in a separate toBeDeleted list, and delete them only after GC has resumed all threads.
Comment 1 Mark Lam 2015-02-04 15:26:08 PST
This is also being tracked in <rdar://problem/17979784> as a follow up fix.
Comment 2 Mark Lam 2015-02-04 15:35:48 PST
Created attachment 246061 [details]
the patch.
Comment 3 Michael Saboff 2015-02-04 16:14:32 PST
Comment on attachment 246061 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=246061&action=review

r=me

> Source/JavaScriptCore/ChangeLog:15
> +        The fix is to put to the invalid threads in a separate toBeDeleted list,

Wording.  Did you mean "The fix is to put the invalid threads ..."?

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:479
> +                // Put the invalid thread on the threadsToBeDeleted list.
> +                // We can't just delete it here because we have suspended other
> +                // threads, and they may still be holding the heap lock. An
> +                // attempt to delete the invalid thread here may therefore
> +                // result in a deadlock. Hence, we need to defer the deletion
> +                // till after we have resumed all threads.

Simplify the comment as we discussed in person.
Comment 4 Mark Lam 2015-02-04 16:34:01 PST
Thanks for the review.  I've simplified the comments and fixed the typo.

Landed in r179648: <http://trac.webkit.org/r179648>.
Comment 5 Gyuyoung Kim 2015-02-04 19:52:38 PST
(In reply to comment #4)
> Thanks for the review.  I've simplified the comments and fixed the typo.
> 
> Landed in r179648: <http://trac.webkit.org/r179648>.

I fix a build break on EFL port since r179648.
- http://trac.webkit.org/changeset/179666