Bug 27488 – SVG and XPath memory leaks in V8 bindings

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mads Ager]

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

[Mads Ager]

Created attachment 33155 [details]
Add 'create' methods to SVGPODTypeWrappers and XPathNSResolvers to avoid memory leaks.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Adam Barth]

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.

[Mads Ager]

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.

[Mads Ager]

Created attachment 33327 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.

[Adam Barth]

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?

[Eric Seidel (no email)]

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.

[Mads Ager]

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

[Mads Ager]

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

[Adam Barth]

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

[Mads Ager]

Created attachment 33430 [details]
Avoid memory leaks and cleanup use of get() and release() on RefPtrs.

[Adam Barth]

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.

[Adam Barth]

Will land.

[Adam Barth]

Darn.  The canary died before I could give this a try.

[Adam Barth]

The canary is alive again.

[Adam Barth]

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