r179576 introduce a deadlock potential during GC thread suspension

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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.

[Mark Lam]

This is also being tracked in <rdar://problem/17979784> as a follow up fix.

[Mark Lam]

Created attachment 246061 [details]
the patch.

[Michael Saboff]

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.

[Mark Lam]

Thanks for the review.  I've simplified the comments and fixed the typo.

Landed in r179648: <http://trac.webkit.org/r179648>.

[Gyuyoung Kim]

(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