Bug 172738 - Implementors of memoryCost() need to be thread-safe
Summary: Implementors of memoryCost() need to be thread-safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-30 16:01 PDT by Simon Fraser (smfr)
Modified: 2019-08-07 13:30 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (18.39 KB, patch)
2017-07-12 12:03 PDT, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-05-30 16:01:13 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-05-30 16:02:16 PDT
<rdar://problem/32474881>
Comment 2 Mark Lam 2017-05-30 16:49:36 PDT
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.
Comment 3 Mark Lam 2017-07-12 12:03:49 PDT
Created attachment 315267 [details]
proposed patch.
Comment 4 Build Bot 2017-07-12 12:04:52 PDT
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.
Comment 5 Keith Miller 2017-07-13 11:16:49 PDT
Comment on attachment 315267 [details]
proposed patch.

r=me.
Comment 6 Mark Lam 2017-07-13 12:17:46 PDT
Thanks for the review.  Landed in r219460: <http://trac.webkit.org/r219460>.