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.
This is also being tracked in <rdar://problem/17979784> as a follow up fix.
Created attachment 246061 [details] the patch.
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.
Thanks for the review. I've simplified the comments and fixed the typo. Landed in r179648: <http://trac.webkit.org/r179648>.
(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