Bug 102555

Created attachment 174752 [details] Patch

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

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.

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

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.

(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*"

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?

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

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

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

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

Created attachment 174795 [details] Patch

Created attachment 174799 [details] Patch

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

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

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.

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.

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

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

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.

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

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>

All reviewed patches have been landed. Closing bug.