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+

Mark Lam
Reported 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>.
Attachments
the patch. (19.90 KB, patch)
2014-02-13 14:45 PST, Mark Lam 		ggaren: review-
DetailsFormatted DiffDiff
simpler patch. (2.48 KB, patch)
2014-02-13 16:56 PST, Mark Lam 		mhahnenberg: review+
DetailsFormatted DiffDiff
Part 2 of fix. (2.66 KB, patch)
2014-02-13 18:28 PST, Mark Lam 		mhahnenberg: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2014-02-13 14:45:16 PST
Created attachment 224114 [details] the patch.
Mark Lam
Comment 2 2014-02-13 14:59:34 PST
Mark H suggested using Strong<Unknown> instead of my ProtectedRef. Will give that a try.
Geoffrey Garen
Comment 3 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.
Mark Lam
Comment 4 2014-02-13 16:56:18 PST
Created attachment 224131 [details] simpler patch.
Mark Hahnenberg
Comment 5 2014-02-13 16:57:18 PST
Comment on attachment 224131 [details] simpler patch. Much improved! r=me
Mark Lam
Comment 6 2014-02-13 17:02:42 PST
Thanks. Landed in r164077: <http://trac.webkit.org/r164077>.
Mark Lam
Comment 7 2014-02-13 18:23:55 PST
Found some bugs. Fix coming.
Mark Lam
Comment 8 2014-02-13 18:28:41 PST
Created attachment 224143 [details] Part 2 of fix.
Mark Hahnenberg
Comment 9 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.
Mark Lam
Comment 10 2014-02-13 18:37:57 PST
Thanks. Part 2 landed in r164089: <http://trac.webkit.org/r164089>.
Mark Lam
Comment 11 2014-02-19 15:19:24 PST
The regression test for this fix is at <https://webkit.org/b/129067>.
Note You need to log in before you can comment on or make changes to this bug.