Bug 128764 - The JSContainerConvertor and ObjcContainerConvertor need to protect JSValueRefs
Summary: The JSContainerConvertor and ObjcContainerConvertor need to protect JSValueRefs
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
Keywords: InRadar
Depends on:
Reported: 2014-02-13 14:03 PST by Mark Lam
Modified: 2016-04-25 04:33 PDT (History)
6 users (show)

See Also:

the patch. (19.90 KB, patch)
2014-02-13 14:45 PST, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
simpler patch. (2.48 KB, patch)
2014-02-13 16:56 PST, Mark Lam
mhahnenberg: review+
Details | Formatted Diff | Diff
Part 2 of fix. (2.66 KB, patch)
2014-02-13 18:28 PST, Mark Lam
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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


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