Bug 128764 – The JSContainerConvertor and ObjcContainerConvertor need to protect JSValueRefs

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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>.

[Mark Lam]

Created attachment 224114 [details]
the patch.

[Mark Lam]

Mark H suggested using Strong<Unknown> instead of my ProtectedRef.  Will give that a try.

[Geoffrey Garen]

Comment on attachment 224114 [details]
the patch.

Let's use Strong or ProtectedPtr instead of introducing a new type here.

[Mark Lam]

Created attachment 224131 [details]
simpler patch.

[Mark Hahnenberg]

Comment on attachment 224131 [details]
simpler patch.

Much improved! r=me

[Mark Lam]

Thanks.  Landed in r164077: <http://trac.webkit.org/r164077>.

[Mark Lam]

Found some bugs.  Fix coming.

[Mark Lam]

Created attachment 224143 [details]
Part 2 of fix.

[Mark Hahnenberg]

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.

[Mark Lam]

Thanks.  Part 2 landed in r164089: <http://trac.webkit.org/r164089>.

[Mark Lam]

The regression test for this fix is at <https://webkit.org/b/129067>.