Bug 27488 - SVG and XPath memory leaks in V8 bindings
Summary: SVG and XPath memory leaks in V8 bindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-21 01:04 PDT by Mads Ager
Modified: 2009-07-24 16:46 PDT (History)
3 users (show)

See Also:


Attachments
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks. (19.24 KB, patch)
2009-07-21 01:11 PDT, Mads Ager
abarth: review-
Details | Formatted Diff | Diff
Avoid memory leaks and cleanup use of get() and release() on RefPtrs. (37.09 KB, patch)
2009-07-23 06:20 PDT, Mads Ager
abarth: review-
Details | Formatted Diff | Diff
Avoid memory leaks and cleanup use of get() and release() on RefPtrs. (37.28 KB, patch)
2009-07-24 05:34 PDT, Mads Ager
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mads Ager 2009-07-21 01:04:00 PDT
The V8 bindings for SVGPODTypeWrappers and XPathNSResolvers are not managing ref counts properly which leads to memory leaks.

These leaks are detected by valgrind on multiple layout tests.

For details see:

http://crbug.com/17244
http://crbug.com/17242
http://crbug.com/17237
http://crbug.com/9516
Comment 1 Mads Ager 2009-07-21 01:11:10 PDT
Created attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.
Comment 2 Eric Seidel (no email) 2009-07-22 23:05:28 PDT
Comment on attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.

In general it's not safe to use .get() on a RefPtr:

$result .= $indent . "RefPtr<V8SVGPODTypeWrapper<" . $nativeReturnType . "> > wrapper = ";
 1527         $result .= "V8SVGPODTypeWrapperCreatorForList<" . $nativeReturnType . ">::create($return, imp->associatedAttributeName());\n";
 1528         $return = "wrapper.get()";

I guess we can assume that whatever uses $return causes a ref to it?  Otherwise if you return the raw pointer, that pointer will have been deleted.
Comment 3 Eric Seidel (no email) 2009-07-22 23:07:08 PDT
Comment on attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.

I would encourage you to create a version of convertToV8Object which takes  PassRefPtr<T> instead of a raw pointer.  That makes the calling convention cleaner (that it takes ownership) and makes it easier to not have to do a .get().  you can do .release() instead and cause less ref-churn.

See the toJS PassRefPtr<T> version in JSDOMBinding.h if you're looking for an example.
Comment 4 Eric Seidel (no email) 2009-07-22 23:09:55 PDT
Comment on attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.

Also, I don't understand why V8 has its own copy of the PODTypeWrapper stuff.  The wrappers have nothing to do with JavaScriptCore (or certainly don't need to be tied to JSC).  They're the "impl" layer for types which SVG expects to be ref-counted but that we internally store as POD types.  It seems that JSC and v8 really really should share the PODTypeWrapper code. :(
Comment 5 Adam Barth 2009-07-22 23:17:31 PDT
Comment on attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.

R- based on Eric's comments.  I, for one, welcome our PassRefPtr overloads.
Comment 6 Mads Ager 2009-07-23 06:18:53 PDT
Thanks for the good suggestions Eric!

I have introduced the overloads for the convert*ToV8Object methods and cleaned up the use of get() and release() on RefPtrs.  What was in the previous patch should be safe, but the current version is nicer.

Regarding the V8 version of the PODTypeWrapper stuff, I don't know why it is different, but it is.  You are right that we should see if we can use the same POD type wrappers both for V8 and for JSC.  I think that should be a separate change though.
Comment 7 Mads Ager 2009-07-23 06:20:31 PDT
Created attachment 33327 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.
Comment 8 Adam Barth 2009-07-23 09:13:05 PDT
Comment on attachment 33327 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.

This is looking good.  A couple questions:

         if ($attrIsPodType) {
-            $resultObject = "wrapper";
+            $resultObject = "wrapper.get()";
+        }

You should just just WTF::getPtr unconditionally.  It should be free in the non-RefPtr case.

-    return V8DOMWrapper::convertNodeToV8Object(object.get());
+    return V8DOMWrapper::convertNodeToV8Object(object);
[...]
-    return V8DOMWrapper::convertNodeToV8Object(object.get());
+    return V8DOMWrapper::convertNodeToV8Object(object);

Why no release here?
Comment 9 Eric Seidel (no email) 2009-07-23 09:16:41 PDT
Comment on attachment 33327 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.

I totally agree.  The PODType unification stuff can be a separate change.  Had I been paying more attention long ago, I would not have let them diverge in the first place. ;)

Should this $wrapper use my?

                 $wrapper = "V8SVGStaticPODTypeWrapperWithPODTypeParent<$nativeType, $implClassName>::create($getterString, imp_wrapper)";
 589                 push(@implContentDecls, "    RefPtr<V8SVGStaticPODTypeWrapperWithPODTypeParent<$nativeType, $implClassName> > wrapper = $wrapper;\n");

You certainly don't want $wrapper used again outside that block, or you'd end up with two calls to create...

I would have probably used a different set of variables to make that whole block use less copy/paste.
Like adding a  my $wrapperType = "V8SVGStaticPODTypeWrapperWithPODTypeParent"
instead of pulling the "push" statement into every else clause.
it's OK as is, just probably could be less code written slightly differently.

I would have probably written all of the:
 623             $resultObject = $resultObject . ".get()";
 633         $result = $result . ".release()" if (IsRefPtrType($attrType));
 1556         $return = $return . ".release()" if (IsRefPtrType($returnType));

As:
$return .= ".release()"
Initially I thought you were assigning the refptrs to themselves in the c++ code. ;)


These can all be a template, like the JSC bindings:
  static v8::Handle<v8::Value> convertNodeToV8Object(PassRefPtr<Node> node)
 145         {
 146             return convertNodeToV8Object(node.get());
 147         }
 148 

see toJS PassRefPtr<T>:
http://trac.webkit.org/browser/trunk/WebCore/bindings/js/JSDOMBinding.h#L253

In this case there is no need for the intermediary RefPtr:
RefPtr<HTMLCollection> collection = htmlDocument->all();
 184     return V8DOMWrapper::convertToV8Object(V8ClassIndex::HTMLCOLLECTION, collection.release());
unless you're trying to shorten then line...

Same here:
  RefPtr<V8SVGStaticPODTypeWrapper<TransformationMatrix> > wrapper = V8SVGStaticPODTypeWrapper<TransformationMatrix>::create(result);
 62     return V8DOMWrapper::convertToV8Object(V8ClassIndex::SVGMATRIX, wrapper.release());

This change looks fine as is.  There are some nits above which you could fix.

Since you're not a committer, I'm going to r- this to ask for one more round. If you were, I could r+ this and you could fix any nits you deemed appropriate as you went to commit.

Thannks for this great cleanup!  Eventually I would like to re-write our autogen system to not be so horribly ugly (aka not perl)... but that's a ways off yet.
Comment 10 Mads Ager 2009-07-24 02:30:30 PDT
(In reply to comment #8)
> (From update of attachment 33327 [details])
> This is looking good.  A couple questions:
> 
>          if ($attrIsPodType) {
> -            $resultObject = "wrapper";
> +            $resultObject = "wrapper.get()";
> +        }
> 
> You should just just WTF::getPtr unconditionally.  It should be free in the
> non-RefPtr case.

Thanks.  I'm now including GetPtr.h and using it WTF::getPtr() unconditionally here.

> -    return V8DOMWrapper::convertNodeToV8Object(object.get());
> +    return V8DOMWrapper::convertNodeToV8Object(object);
> [...]
> -    return V8DOMWrapper::convertNodeToV8Object(object.get());
> +    return V8DOMWrapper::convertNodeToV8Object(object);
> 
> Why no release here?

Because object is a PassRefPtr in these two cases.
Comment 11 Mads Ager 2009-07-24 02:37:17 PDT
(In reply to comment #9)
> Should this $wrapper use my?

yes, done.

> I would have probably used a different set of variables to make that whole
> block use less copy/paste.
> Like adding a  my $wrapperType = "V8SVGStaticPODTypeWrapperWithPODTypeParent"
> instead of pulling the "push" statement into every else clause.
> it's OK as is, just probably could be less code written slightly differently.

I left this alone.  Trying it out I didn't manage to make it more readable than it is now.
 
> I would have probably written all of the:
>  623             $resultObject = $resultObject . ".get()";
>  633         $result = $result . ".release()" if (IsRefPtrType($attrType));
>  1556         $return = $return . ".release()" if (IsRefPtrType($returnType));
>
> As:
> $return .= ".release()"
> Initially I thought you were assigning the refptrs to themselves in the c++
> code. ;)

Thanks, done!
 
> These can all be a template, like the JSC bindings:
>   static v8::Handle<v8::Value> convertNodeToV8Object(PassRefPtr<Node> node)
>  145         {
>  146             return convertNodeToV8Object(node.get());
>  147         }
>  148 
> 
> see toJS PassRefPtr<T>:
> http://trac.webkit.org/browser/trunk/WebCore/bindings/js/JSDOMBinding.h#L253

I see what you mean.  That would be a larger cleanup because we use no overloading for this currently and I'm unsure if we could get bitten by changing it.  I'd like to leave that for a separate cleanup.
 
> In this case there is no need for the intermediary RefPtr:
> RefPtr<HTMLCollection> collection = htmlDocument->all();
>  184     return V8DOMWrapper::convertToV8Object(V8ClassIndex::HTMLCOLLECTION,
> collection.release());
> unless you're trying to shorten then line...

Thanks. done.

> Same here:
>   RefPtr<V8SVGStaticPODTypeWrapper<TransformationMatrix> > wrapper =
> V8SVGStaticPODTypeWrapper<TransformationMatrix>::create(result);
>  62     return V8DOMWrapper::convertToV8Object(V8ClassIndex::SVGMATRIX,
> wrapper.release());

Thanks, done.

I'm running layout tests now to make sure that there are no issues with my latest patch.  I'll update the patch later today.
Comment 12 Adam Barth 2009-07-24 02:49:03 PDT
> I see what you mean.  That would be a larger cleanup because we use no
> overloading for this currently and I'm unsure if we could get bitten by
> changing it.  I'd like to leave that for a separate cleanup.

In the intermediate term, we'd like to move to making these all overloaded so they can be as similar to toJS() as possible.
Comment 13 Mads Ager 2009-07-24 05:34:53 PDT
Created attachment 33430 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.
Comment 14 Adam Barth 2009-07-24 10:45:53 PDT
Comment on attachment 33430 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.

Yay!  Looks great.  Thanks for incorporating our comments.
Comment 15 Adam Barth 2009-07-24 12:36:19 PDT
Will land.
Comment 16 Adam Barth 2009-07-24 12:38:29 PDT
Darn.  The canary died before I could give this a try.
Comment 17 Adam Barth 2009-07-24 16:44:20 PDT
The canary is alive again.
Comment 18 Adam Barth 2009-07-24 16:46:33 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	WebCore/bindings/v8/V8DOMWrapper.h
	M	WebCore/bindings/v8/V8SVGPODTypeWrapper.h
	M	WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
	M	WebCore/bindings/v8/custom/V8ClientRectListCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp
	M	WebCore/bindings/v8/custom/V8CustomXPathNSResolver.h
	M	WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
	M	WebCore/bindings/v8/custom/V8DocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8ElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLFormElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLSelectElementCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
	M	WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeIteratorCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeListCustom.cpp
	M	WebCore/bindings/v8/custom/V8SVGMatrixCustom.cpp
	M	WebCore/bindings/v8/custom/V8TreeWalkerCustom.cpp
	M	WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
Committed r46383
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	WebCore/bindings/v8/V8DOMWrapper.h
	M	WebCore/bindings/v8/V8SVGPODTypeWrapper.h
	M	WebCore/bindings/v8/custom/V8HTMLFormElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomXPathNSResolver.cpp
	M	WebCore/bindings/v8/custom/V8DocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
	M	WebCore/bindings/v8/custom/V8ClientRectListCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeIteratorCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLDocumentCustom.cpp
	M	WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
	M	WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomXPathNSResolver.h
	M	WebCore/bindings/v8/custom/V8SVGMatrixCustom.cpp
	M	WebCore/bindings/v8/custom/V8ElementCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLSelectElementCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
	M	WebCore/bindings/v8/custom/V8NodeListCustom.cpp
	M	WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
	M	WebCore/bindings/v8/custom/V8TreeWalkerCustom.cpp
r46383 = e5e82f7bb3bca051b65dc01162d59d266c214e7d (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46383