Bug 128764

Summary: The JSContainerConvertor and ObjcContainerConvertor need to protect JSValueRefs
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, mmirman, msaboff, oliver
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=156966
Attachments:
Description Flags
the patch.
ggaren: review-
simpler patch.
mhahnenberg: review+
Part 2 of fix. mhahnenberg: review+

Description Mark Lam 2014-02-13 14:03:33 PST 
The JSContainerConvertor and ObjcContainerConvertor were holding on to JSValueRefs in a HashMap and Vector, and these are not scanned by the GC. Hence, objects were getting collected when we're not expecting it.

The fix is to introduce a ProtectedRef class that takes care of managing the stored JSValueRefs, and calling JSValueProtect() and JSValueUnprotect() on them as needed.

ref: <rdar://problem/16029133>.
Comment 1 Mark Lam 2014-02-13 14:45:16 PST 
Created attachment 224114 [details]
the patch.
Comment 2 Mark Lam 2014-02-13 14:59:34 PST 
Mark H suggested using Strong<Unknown> instead of my ProtectedRef.  Will give that a try.
Comment 3 Geoffrey Garen 2014-02-13 14:59:42 PST 
Comment on attachment 224114 [details]
the patch.

Let's use Strong or ProtectedPtr instead of introducing a new type here.
Comment 4 Mark Lam 2014-02-13 16:56:18 PST 
Created attachment 224131 [details]
simpler patch.
Comment 5 Mark Hahnenberg 2014-02-13 16:57:18 PST 
Comment on attachment 224131 [details]
simpler patch.

Much improved! r=me
Comment 6 Mark Lam 2014-02-13 17:02:42 PST 
Thanks.  Landed in r164077: <http://trac.webkit.org/r164077>.
Comment 7 Mark Lam 2014-02-13 18:23:55 PST 
Found some bugs.  Fix coming.
Comment 8 Mark Lam 2014-02-13 18:28:41 PST 
Created attachment 224143 [details]
Part 2 of fix.
Comment 9 Mark Hahnenberg 2014-02-13 18:31:30 PST 
Comment on attachment 224143 [details]
Part 2 of fix.

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        toJS() is the wrong cast function to used. We need to use toJSForGC() instead.

to use.

> Source/JavaScriptCore/ChangeLog:9
> +        Also we need to acquire the JSLock because to protect accessed to the Strong

...JSLock to prevent concurrent accesses to the Strong handle list.
Comment 10 Mark Lam 2014-02-13 18:37:57 PST 
Thanks.  Part 2 landed in r164089: <http://trac.webkit.org/r164089>.
Comment 11 Mark Lam 2014-02-19 15:19:24 PST 
The regression test for this fix is at <https://webkit.org/b/129067>.