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.
Created attachment 67498 [details] Work in progress
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?
(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. :(
Created attachment 67631 [details] Patch
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.
(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*
(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()
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?
(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.
Created attachment 67948 [details] Now uses V8DOMWrapper::getWrapper which handles the different kind of elements we might have
Created attachment 67958 [details] Changed to use setHiddenWindowReference which is more inline with what the JSC bindings do
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.
(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
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>
All reviewed patches have been landed. Closing bug.
(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.
(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?
(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.
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 :'(
(In reply to comment #19) s/classList/dataset/
(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.
(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.
(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.