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

See Also:

Attachments
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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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

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