Bug 45426 – [Chromium] fast/dom/dataset-gc.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dirk Pranke]

This test was introduced in r66954 (see bug 45358). If I understand the conversation on #webkit correctly, the wrapper object is being recreated incorrectly instead of persisting and being tied to the lifetime of the element.

JamesR suggests that we need to add a call to V8DomWrapper::setHIddenReference() at around line 860 of CodeGeneratorV8.pm ... patch to follow shortly.

[Erik Arvidsson]

Created attachment 67498 [details]
Work in progress

[Erik Arvidsson]

The test still fails after my changes but I don't understand why. This seems to follow the same pattern as NamedNodeMap.

Nate, do you have any ideas?

[Nate Chapin]

(In reply to comment #2)
> The test still fails after my changes but I don't understand why. This seems to follow the same pattern as NamedNodeMap.
> 
> Nate, do you have any ideas?

Are we sure NamedNodeMap is right?  If I'm reading this correctly, your code is tying the Element wrapper's lifetime to that of the DOMStringMap, when we want the opposite.

:(

[Erik Arvidsson]

Created attachment 67631 [details]
Patch

[Nate Chapin]

Comment on attachment 67631 [details]
Patch


> @@ -44,6 +44,7 @@
>  #include "V8DedicatedWorkerContext.h"
>  #include "V8DOMApplicationCache.h"
>  #include "V8DOMMap.h"
> +#include "V8DOMStringMap.h"
>  #include "V8DOMWindow.h"
>  #include "V8EventListenerList.h"
>  #include "V8EventSource.h"

Remove include

> +v8::Handle<v8::Value> toV8(DOMStringMap* impl)
> +{
> +    if (!impl)
> +        return v8::Null();
> +    v8::Handle<v8::Object> wrapper = V8DOMStringMap::wrap(impl);
> +    // Add a hidden reference from the element to the DOMStringMap.
> +    Element* element = impl->element();
> +    if (!wrapper.IsEmpty() && element)
> +        V8DOMWrapper::setHiddenReference(V8Element::wrap(element), wrapper);

I think this needs to be toV8(element) rather than V8Element::wrap(element). wrap() may not handle subclasses of Element correctly.

[Erik Arvidsson]

(In reply to comment #5)
> (From update of attachment 67631 [details])
> 
> > @@ -44,6 +44,7 @@
> >  #include "V8DedicatedWorkerContext.h"
> >  #include "V8DOMApplicationCache.h"
> >  #include "V8DOMMap.h"
> > +#include "V8DOMStringMap.h"
> >  #include "V8DOMWindow.h"
> >  #include "V8EventListenerList.h"
> >  #include "V8EventSource.h"
> 
> Remove include

Done.

> > +v8::Handle<v8::Value> toV8(DOMStringMap* impl)
> > +{
> > +    if (!impl)
> > +        return v8::Null();
> > +    v8::Handle<v8::Object> wrapper = V8DOMStringMap::wrap(impl);
> > +    // Add a hidden reference from the element to the DOMStringMap.
> > +    Element* element = impl->element();
> > +    if (!wrapper.IsEmpty() && element)
> > +        V8DOMWrapper::setHiddenReference(V8Element::wrap(element), wrapper);
> 
> I think this needs to be toV8(element) rather than V8Element::wrap(element). wrap() may not handle subclasses of Element correctly.

toV8 returns a V8::Value* but we need a V8::Object*

[Nate Chapin]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 67631 [details] [details])
> > 
> > > @@ -44,6 +44,7 @@
> > >  #include "V8DedicatedWorkerContext.h"
> > >  #include "V8DOMApplicationCache.h"
> > >  #include "V8DOMMap.h"
> > > +#include "V8DOMStringMap.h"
> > >  #include "V8DOMWindow.h"
> > >  #include "V8EventListenerList.h"
> > >  #include "V8EventSource.h"
> > 
> > Remove include
> 
> Done.
> 
> > > +v8::Handle<v8::Value> toV8(DOMStringMap* impl)
> > > +{
> > > +    if (!impl)
> > > +        return v8::Null();
> > > +    v8::Handle<v8::Object> wrapper = V8DOMStringMap::wrap(impl);
> > > +    // Add a hidden reference from the element to the DOMStringMap.
> > > +    Element* element = impl->element();
> > > +    if (!wrapper.IsEmpty() && element)
> > > +        V8DOMWrapper::setHiddenReference(V8Element::wrap(element), wrapper);
> > 
> > I think this needs to be toV8(element) rather than V8Element::wrap(element). wrap() may not handle subclasses of Element correctly.
> 
> toV8 returns a V8::Value* but we need a V8::Object*

Nevermind.  The code generator changed and I missed it.  I was think of what is now wrapSlow()

[Erik Arvidsson]

Is this good to go?

I'm a bit concerned about using V8Element::wrap since toV8(element) checks isHTMLElement and isSVGElement and calls toV8 on those. I guess I could do the same thing but I'm not sure it is necessary?

[Nate Chapin]

(In reply to comment #8)
> Is this good to go?
> 
> I'm a bit concerned about using V8Element::wrap since toV8(element) checks isHTMLElement and isSVGElement and calls toV8 on those. I guess I could do the same thing but I'm not sure it is necessary?

So my knowledge of these functions (and which one is which) is apparently out of date, but yes, I'm pretty sure you need to be doing the isHTMLElement and isSVGElement calls.  We generally need to wrap an object as its real type to avoid problems when unwrapping them.

[Erik Arvidsson]

Created attachment 67948 [details]
Now uses V8DOMWrapper::getWrapper which handles the different kind of elements we might have

[Erik Arvidsson]

Created attachment 67958 [details]
Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do

[Nate Chapin]

Comment on attachment 67958 [details]
Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do


> +v8::Handle<v8::Value> toV8(DOMStringMap* impl)
> +{
> +    if (!impl)
> +        return v8::Null();
> +    v8::Handle<v8::Object> wrapper = V8DOMStringMap::wrap(impl);
> +    // Add a hidden reference from the element to the DOMStringMap.
> +    Element* element = impl->element();
> +    if (!wrapper.IsEmpty() && element)
> +        V8DOMWrapper::setHiddenWindowReference(element->document()->frame(), wrapper);
> +    return wrapper;

If that's what JSC does, I guess it's ok to do the same. I don't know anything about DOMStringMap, so I don't have a good idea whether it make sense to keep it around for the life of the global object.  It seems like the default answer should be to tie it to its owner object, though.

Where do the JSC bindings do this?  I didn't immediately see it.

[Erik Arvidsson]

(In reply to comment #12)
> Where do the JSC bindings do this?  I didn't immediately see it.

It is done in WebCore/bindings/js/JSElementCustom.cpp

 63     markDOMObjectWrapper(markStack, globalData, element->optionalDataset());

This was done in Bug 45358

[Adam Barth]

Comment on attachment 67958 [details]
Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do

Clearing flags on attachment: 67958

Committed r67805: <http://trac.webkit.org/changeset/67805>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

(In reply to comment #11)
> Created an attachment (id=67958) [details]
> Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do

Erik, apparently your patch doesn't do what JSC does: JSC makes DOMStringMap to live as long as elements is live, and you make it live as long as global object is alive, so it is apparently a leak.

There is another approach you may use which would be more inline with the way object lives are treated in v8 bindings: you could make a group out of element's and DOMStringMap's---in this case they would survive or get GCed together which is apparently what you're looking for.

[Erik Arvidsson]

(In reply to comment #16)
> (In reply to comment #11)
> > Created an attachment (id=67958) [details] [details]
> > Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do
> 
> Erik, apparently your patch doesn't do what JSC does: JSC makes DOMStringMap to live as long as elements is live, and you make it live as long as global object is alive, so it is apparently a leak.

That is how I understood the JSC code. I should have known better though.

> There is another approach you may use which would be more inline with the way object lives are treated in v8 bindings: you could make a group out of element's and DOMStringMap's---in this case they would survive or get GCed together which is apparently what you're looking for.

Yes, do you have any pointers?

[anton muhin]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #11)
> > > Created an attachment (id=67958) [details] [details] [details]
> > > Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do
> > 
> > Erik, apparently your patch doesn't do what JSC does: JSC makes DOMStringMap to live as long as elements is live, and you make it live as long as global object is alive, so it is apparently a leak.
> 
> That is how I understood the JSC code. I should have known better though.
> 
> > There is another approach you may use which would be more inline with the way object lives are treated in v8 bindings: you could make a group out of element's and DOMStringMap's---in this case they would survive or get GCed together which is apparently what you're looking for.
> 
> Yes, do you have any pointers?

Sure, take a look at http://www.google.com/codesearch/p?hl=en#N6Qhr5kJSgQ/WebCore/bindings/v8/V8GCController.cpp&q=file:v8gccontroller.cpp%20ObjectGrouperVisitor&sa=N&cd=1&ct=rc

You could either add wrapper for DOMStringMap when visiting Element (JSC requires the way to access the map for the element, so code should be already there) or when visiting the DOMStringMap itself if you have pointer back to the owning element.

Feel free to ping me if you have any questions.

[Erik Arvidsson]

I've been here before but I think that 

        V8DOMWrapper::setHiddenReference(V8DOMWrapper::getWrapper(element), wrapper);

should work. Nate said that getWrapper might return null but since the classList can only be retrieved through the element the wrapper should always be available.

I did look at V8GCController and I could not figure out what to do with that :'(

[Erik Arvidsson]

(In reply to comment #19)
s/classList/dataset/

[anton muhin]

(In reply to comment #19)
> I've been here before but I think that 
> 
>         V8DOMWrapper::setHiddenReference(V8DOMWrapper::getWrapper(element), wrapper);
> 
> should work. Nate said that getWrapper might return null but since the classList can only be retrieved through the element the wrapper should always be available.
> 
> I did look at V8GCController and I could not figure out what to do with that :'(

At least in https://bugs.webkit.org/attachment.cgi?id=67958&action=prettypatch you use V8DOMWrapper::setHiddenWindowReference(element->document()->frame(), wrapper) which makes a reference from global object to the wrapper (but I saw your https://bugs.webkit.org/attachment.cgi?id=67948&action=prettypatch which is better imho)

I think setHiddenReference thing should work right.  And there is no need to do it with object groping if you don't want to :)

Regarding getWrapper.  I don't know for sure, but it looks extremely likely that one can only create a wrapper for dataset when there is a live wrapper for element, so plain getWrapper should work.  To be completely safe you might want to prefer to use toV8 which will create a wrapper if one is missing.

[Erik Arvidsson]

(In reply to comment #21)
> Regarding getWrapper.  I don't know for sure, but it looks extremely likely that one can only create a wrapper for dataset when there is a live wrapper for element, so plain getWrapper should work.  To be completely safe you might want to prefer to use toV8 which will create a wrapper if one is missing.

toV8 creates a V8::Value. This needs a V8::Object.

[anton muhin]

(In reply to comment #22)
> (In reply to comment #21)
> > Regarding getWrapper.  I don't know for sure, but it looks extremely likely that one can only create a wrapper for dataset when there is a live wrapper for element, so plain getWrapper should work.  To be completely safe you might want to prefer to use toV8 which will create a wrapper if one is missing.
> 
> toV8 creates a V8::Value. This needs a V8::Object.

Yes.  That's why we have Cast or As methods :)  But you need to check before it's not empty.

v8::Value corresponds to any JS object (string, number, real object, etc) and v8::Object stands for real objects.  So if you manage to get anything from toV8, it must be object (unless handle is empty).  That probably should have been put into signature.