Bug 172738 – Implementors of memoryCost() need to be thread-safe

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

JSC calls visitChildren(JSCell* cell, SlotVisitor& visitor) on its GC threads, yet I don't expect that any implementers of visitChildren() or memoryCost() are thread-safe. For example, AudioBuffer::memoryCost() loops over channels with no lock, HTMLCollection::memoryCost() does a bunch of math on map sizes, HTMLCanvasElement::memoryCost() consults the image buffer, which races with image buffer assignment etc.

[Radar WebKit Bug Importer]

<rdar://problem/32474881>

[Mark Lam]

1. It is safe for the GC to scan any JS object pointers in these DOM objects because the GC controls the life-cycle of JS objects referenced by those pointers.

2. It is safe for the GC to scan wrapped objects because:
   a. all JS DOM object wrappers extend JSDOMWrapper, and
   b. JSDOMWrapper has a Ref to the wrapped object.
   This means that the wrapped object is guaranteed to outlive the JS wrapper.

   Also, there's a 1 to 1 correspondence between the wrapper and the wrapped object.
   We never change the wrapped object after creation of the wrapper.

Based on the above, the work in visitChildren should be safe because the "visiting" is should only be done on JS objects pointers.  Calling memoryCost() on wrapped objects is safe because the wrapped object is guaranteed to be alive still.

The remaining question is whether the various implementations of memoryCost() does anything with sub-objects that are not tied to the liveness of JS objects.

We should still do a survey of the visitChildren implementations to see if they call any other C++ member functions that may also do thread unsafe work.

[Mark Lam]

Created attachment 315267 [details]
proposed patch.

[Build Bot]

Attachment 315267 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/LiveNodeList.h:94:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/html/CachedHTMLCollection.h:45:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 315267 [details]
proposed patch.

r=me.

[Mark Lam]

Thanks for the review.  Landed in r219460: <http://trac.webkit.org/r219460>.