Bug 102555 – Store MutationObserver callback in a hidden property for V8

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Store MutationObserver callback in a hidden property for V8

[Elliott Sprehn]

Created attachment 174752 [details]
Patch

[Elliott Sprehn]

This is the smallest possible change, and it allows us to keep all the existing code related to callback handling.

[Adam Barth]

Comment on attachment 174752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174752&action=review

This looks great.  My only question is whether V8WeakCallback is really a property of the MutationCallback or whether it is functionality that all callbacks should have that V8MutationObserver activates in V8MutationObserver::constructorCallback.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3325
> +        ${className}* wrapper = static_cast<${className}*>(parameter);

I'm not sure wrapper is the right term here.  Perhaps |object| ?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3386
> +        m_callback.Dispose();

Should m_callback be a ScopedPersistent?  You seem to be re-implementing the ScopedPersistent pattern.  We can fix that in a followup patch.

[Adam Barth]

*** Bug 102553 has been marked as a duplicate of this bug. ***

[Elliott Sprehn]

So this fixes the leak with cycles and the callback, but interestingly it seems v8 is smart enough to not keep objects that are not reachable alive so:

var observer = new WebKitMutationObserver(function() {});

doesn't actually leak without this patch because v8 knows the callback can't reach observer.

but

var observer = new WebKitMutationObserver(function() { observer });

does leak.

I think there's something even more fundamentally leaky in MutationObservers, because even without leaks once you observe() a node you leak:

<script>
function noCycles(fn) {
  var observer = new WebKitMutationObserver(fn);
  observer.observe(document.body, {attributes:true}) // LEAK!!!
}

for (i=0; i < 1000; i++) noCycles(function(){});

gc();

window.location.href = 'about:blank';
</script>

In this test observers and nodes never get collected.

Try loading the file in DRT several times: DumpRenderTree /test.html /test.html /test.html

and you get:

Content-Type: text/plain
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x8
  RenderBlock {HTML} at (0,0) size 800x8
    RenderBody {BODY} at (8,8) size 784x0
#EOF
#EOF
#EOF
LEAK: 32 WebCoreNode

and if you instrument the constructor and destructor for MutationObserver you have 3000 live MutationObservers when DRT closes.

[Elliott Sprehn]

(In reply to comment #5)
> ...
> I think there's something even more fundamentally leaky in MutationObservers, because even without leaks once you observe() a node you leak:
> 

That should say "even without *cycles*"

[Adam Klein]

I can reproduce Elliott's other leak, but am at something of a loss to explain it. Somehow it appears the Document is being kept alive, and that in turn is keeping all nodes in its tree, as well as any registered observers, alive. So my question is, what's keeping the Document alive?

[Adam Klein]

(In reply to comment #7)
> I can reproduce Elliott's other leak, but am at something of a loss to explain it. Somehow it appears the Document is being kept alive, and that in turn is keeping all nodes in its tree, as well as any registered observers, alive. So my question is, what's keeping the Document alive?

For the record, this appears to be due to http://trac.webkit.org/changeset/134830, which made MutationObserver an ActiveDOMObject.

[Adam Klein]

(In reply to comment #8)
> (In reply to comment #7)
> > I can reproduce Elliott's other leak, but am at something of a loss to explain it. Somehow it appears the Document is being kept alive, and that in turn is keeping all nodes in its tree, as well as any registered observers, alive. So my question is, what's keeping the Document alive?
> 
> For the record, this appears to be due to http://trac.webkit.org/changeset/134830, which made MutationObserver an ActiveDOMObject.

And, for the record, something like that ActiveDOMObject patch is required for the patch on this bug to be correct, since we now rely on the wrapper to keep things alive appropriately.

[Adam Barth]

Yes.  We just need to make sure not to claim we have pending activity after stop() is called on the mutation observer.

[Elliott Sprehn]

(In reply to comment #10)
> Yes.  We just need to make sure not to claim we have pending activity after stop() is called on the mutation observer.

Yeah, the complication is that they want observation of Nodes from different documents to work:

var OtherFramesObserverClass = iframe.contentWIndow.MutationObserver;
var observer = new OtherFramesObserverClass(function() {});
iframe.parentNode.removeChild(iframe);
observer.observe(node, { ... });

so stopping all pending activity when the original window's document goes away is not right.

[Elliott Sprehn]

Created attachment 174795 [details]
Patch

[Elliott Sprehn]

Created attachment 174799 [details]
Patch

[Elliott Sprehn]

(In reply to comment #3)
> (From update of attachment 174752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174752&action=review
> 
> This looks great.  My only question is whether V8WeakCallback is really a property of the MutationCallback or whether it is functionality that all callbacks should have that V8MutationObserver activates in V8MutationObserver::constructorCallback.

I added a new argument to ::create() that lets you pass an owner and put it into a dependent retained like mode.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3325
> > +        ${className}* wrapper = static_cast<${className}*>(parameter);
> 
> I'm not sure wrapper is the right term here.  Perhaps |object| ?

Done.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3386
> > +        m_callback.Dispose();
> 
> Should m_callback be a ScopedPersistent?  You seem to be re-implementing the ScopedPersistent pattern.  We can fix that in a followup patch.

Yeah. I added a patch that used ScopedPersistent but I'm nervous about trying to change both things at the same time so I uploaded another without it.

[Build Bot]

Comment on attachment 174799 [details]
Patch

Attachment 174799 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14859738

New failing tests:
fast/mutation/leak-cycle-observer-wrapper.html

[Adam Barth]

Comment on attachment 174799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174799&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3287
> -    static PassRefPtr<${className}> create(v8::Local<v8::Value> value, ScriptExecutionContext* context)
> +    static PassRefPtr<${className}> create(v8::Local<v8::Value> value, ScriptExecutionContext* context, v8::Local<v8::Object> owner = (v8::Object*)0)

Is there some reason to require a Local handle here?  Also, the way to make a default parameter is as follows:

v8::Handle<v8::Object> owner = v8::Handle<v8::Object>()

In principle, we don't know that 0 maps to empty (and we don't use C-style casts in WebKit).

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:52
> +    V(callback)

Please put this in alphabetical order.

[Elliott Sprehn]

Comment on attachment 174799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174799&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3287
>> +    static PassRefPtr<${className}> create(v8::Local<v8::Value> value, ScriptExecutionContext* context, v8::Local<v8::Object> owner = (v8::Object*)0)
> 
> Is there some reason to require a Local handle here?  Also, the way to make a default parameter is as follows:
> 
> v8::Handle<v8::Object> owner = v8::Handle<v8::Object>()
> 
> In principle, we don't know that 0 maps to empty (and we don't use C-style casts in WebKit).

I used a Local because that's what value is as well. I can change to just v8::Handle.

[Elliott Sprehn]

(In reply to comment #15)
> (From update of attachment 174799 [details])
> Attachment 174799 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14859738
> 
> New failing tests:
> fast/mutation/leak-cycle-observer-wrapper.html

Hmm, this is really weird that the test has different output on mac but passes on cr-linux...

[Elliott Sprehn]

adamk rolled out my ActiveDOMObject patch so we can't land this now. I'm blocked until we resolve the wrapper drop off.

[Adam Klein]

The patch on bug 102328 is in the cq and fixes the wrapper dropoff bug. Once it lands, this patch should also be safe to land.

[Adam Klein]

Created attachment 175254 [details]
Patch for landing (on behalf of esprehn)

[WebKit Review Bot]

Comment on attachment 175254 [details]
Patch for landing (on behalf of esprehn)

Clearing flags on attachment: 175254

Committed r135305: <http://trac.webkit.org/changeset/135305>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.