Bug 93661 – [JSC] MutationObservers should not create circular, leaky references

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

In an email, abarth wrote:

--------
In reading the MutationObservers code recently, I had the following
questions.  Although these questions are phrased in terms of V8, I
have the same questions w.r.t. the JavaScriptCore implementation.

1) Do MutationObservers leak memory?  Specification, consider the
following JavaScript:

function foo() {
  function bar() {
    alert(observer.qux);
  }
  var observer = new MutationObserver(bar);
}

How does the memory we allocate for WebCore::MutationObserver ever get
freed?  It appears that

A) MutationObserver has a RefPtr<MutationCallback> m_callback, which
(concretely) is of type V8MutationCallback
B) V8MutationCallback has a v8::Persistent<v8::Object> m_callback,
which is a handle to the JavaScript function bar().
C) bar() is a closure that contains observer, which (concretely) is of
type V8MutationObserver, which holds a reference to MutationObserver
via the wrapper map.

That appears to be a reference cycle that loops through the V8 and C++
heaps, which means we'll leak.  Is there some mechanism that saves us
from this leak?

2) What keeps the MutationObserver JavaScript wrapper alive?
Specification, consider the following JavaScript:

function bar(mutations, observer) {
    alert(observer.qux);
}
function foo() {
  var observer = new MutationObserver(bar);
  observer.qux = "PASS";
  observer.observe(document.body, ...);
}

Once foo() completes, there are no longer any references to observer
in the V8 heap, so the V8 garbage collector will collect the
V8MutationObserver object (and we'll remove it from the wrapper map).
Presumably Node::registerMutationObserver will cause the DOM to retain
the underlying WebCore::MutationObserver object.  At some point in the
future, the mutation observer will trigger and call bar.  The second
argument to bar is the MutationObserver object, which will get a fresh
JavaScript wrapper because the previous JavaScript wrapper is no
longer present in the wrapper map.  The new wrapper will not have
property qux because that property was stored on the old wraper, which
was discarded.
---------

The answers to Adam's questions here is that both (1) and (2) are problems. I suspect that solving (1) (by pushing the circular references into the JS heap) will also solve (2) in the process.

[Elliott Sprehn]

Do you know where to start on this? What code should I be looking at that does this already?

[Adam Klein]

bindings/v8/*EventListener* is probably a good start, at least to get familiar with the issues involved (weak handles, hidden properties).

[Adam Barth]

> Do you know where to start on this? What code should I be looking at that does this already?

There's a drawing on a whiteboard in the SF office about one approach.  We can talk it through on Monday if you're available.

[Adam Barth]

The key idea is to store callback as a (hidden) property on the MutationObserver wrapper object rather than via a persistent handle in C++ land.  That should keep any cycles in the V8 heap so that the V8 garbage collector can deal with them.

One approach to solving problem (2), is to make MutationObserver an ActiveDOMObject so that it can implement hasPendingActivity and can be notified when its document is stopped.

[Elliott Sprehn]

Created attachment 161050 [details]
Patch

Fix cicular references and dropped wrappers. This patch is rather big since it introduces new primitives for doing this abstractly which should also benefit UndoManager and future APIs. Im not sure how to write tests for this, does someone have an example of tests for leaks?

[Elliott Sprehn]

Comment on attachment 161050 [details]
Patch

Sorry, my JSC code and compile was busted. Trying to fix that now.

[Elliott Sprehn]

Created attachment 161068 [details]
Patch

Fix XCode build files.

[Elliott Sprehn]

Okay this should probably be split into 3 patches. One for JSC, one for V8, and finally a patch here that uses them to fix MutationObservers.

[Elliott Sprehn]

This should probably be split into 3 patches. One for JSC, one for V8, and finally a patch here that uses them to fix MutationObservers.

[Adam Klein]

Comment on attachment 161050 [details]
Patch

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

Comments on the generic and V8 portions; JSC is not my strong suit.

> Source/WebCore/bindings/v8/V8MutationCallback.cpp:58
> +    if (!mutations)

This ugly little block will go away if you make the first arg a const ref.

> Source/WebCore/bindings/v8/V8MutationCallback.h:47
> +class V8MutationCallback : public MutationCallback, public ActiveDOMCallback, public V8WrapperRetainedValue<MutationObserver> {

Design question: how'd you end up with inheritance for V8WrapperRetainedValue instead of composition? It's a little confusing to me since the V8MutationCallback _itself_ isn't "wrapper-retained", only the underlying v8::Object.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:50
> +        ASSERT(!wrapper.IsEmpty());

Any reason not to ASSERT(!value.IsEmpty()) too?

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:53
> +        wrapper->SetHiddenValue(m_propertyName, value);

Seems like this should probably be before the makeWeak() calls, since SetHiddenValue() can trigger GC (though I guess |wrapper| and |value| will likely be alive until this constructor finishes.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:67
> +    void releaseScriptValue()

Why is this protected instead of private?

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:69
> +        v8::HandleScope handleScope;

Don't think you need this HandleScope.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:78
> +    static v8::Persistent<v8::String> createPropertyName()

Why do we need a new propertyName each time? I guess this is for generality? Seems like in this case, there's no way for a MutationObserver to have more than one MutationCallback associated with it. Not sure it's worth the complexity for now (for example, I don't think this code is thread-safe).

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:86
> +    OwnHandle<v8::Object> m_wrapper;

OwnHandle is sadly dead as of a week ago (replaced by SharedPersistent), but that doesn't support weak handles. abarth, is the correct thing here to roll-your-own weak handles using v8 API?

> Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp:68
> +    RefPtr<MutationCallback> callback = V8MutationCallback::create(wrapper, arg->ToObject(), context);

Since you've already type-checked arg above, this can just be arg.As<v8::Object>() (just a type-cast, no need to run code).

> Source/WebCore/dom/MutationCallback.h:51
> +    virtual bool call(MutationRecordArray*, MutationObserver*) = 0;

Now that we're not subject to the whims of the IDL compiler, this ought to be a "const MutationRecordArray&" (and MutationRecordVector's probably a better name, if you feel like changing that too; you could even drop the typedef altogether, which again was only there to satisfy WebKit IDL).

> Source/WebCore/dom/MutationObserver.h:95
> +    explicit MutationObserver(PassRefPtr<MutationCallback>, ScriptExecutionContext*);

Nit: can drop the "explicit" here given two args.

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 161050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161050&action=review
>
> ...
> 
> Design question: how'd you end up with inheritance for V8WrapperRetainedValue instead of composition? It's a little confusing to me since the V8MutationCallback _itself_ isn't "wrapper-retained", only the underlying v8::Object.

No reason,  but it does require more manual code in the JSC side without it since you'd have to expose the retainedBy method yourself. I can change to composition if you'd prefer?

> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:50
> > +        ASSERT(!wrapper.IsEmpty());
> 
> Any reason not to ASSERT(!value.IsEmpty()) too?

It didn't seem like a restriction that was necessary on this generic construct. For instance you may allow new AwesomeNewAPI() {} and just create a noop ApiCallback. Since that's a future thing I'll add an ASSERT for now.


> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:67
> > +    void releaseScriptValue()
> 
> Why is this protected instead of private?

I had done it so someone could release it early, but thinking about it now we should probably make it private and let you manage the lifecycle indirectly.

> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:78
> > +    static v8::Persistent<v8::String> createPropertyName()
> 
> Why do we need a new propertyName each time? I guess this is for generality? Seems like in this case, there's no way for a MutationObserver to have more than one MutationCallback associated with it. Not sure it's worth the complexity for now (for example, I don't think this code is thread-safe).

UndoManager has an API like:

undoManager.transact({undo: function() {} });
undoManager.transact({undo: function() {} });
undoManager.transact({undo: function() {} });

And this new generic API allows you do:

undoManager->transact(DOMTransaction::create(undoManagerWrapper, arg[0]))

and not worry about maintaining an array, or doing the inversion they have right now where DOMTransaction actually gets it's own wrapper, and then sets a hidden property cycle on it, and then uses a custom GC marking function (which is rather confusing to understand).

http://code.google.com/p/chromium/source/search?q=V8DomTransaction&origq=V8DomTransaction&btnG=Search+Trunk

> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:86
> > +    OwnHandle<v8::Object> m_wrapper;
> 
> OwnHandle is sadly dead as of a week ago (replaced by SharedPersistent), but that doesn't support weak handles. abarth, is the correct thing here to roll-your-own weak handles using v8 API?

Doh! That sucks, since this worked perfectly :) It'd be unfortunate to have to roll my own weak handle. Can we just add that feature back into the SharedPersistent?

[Adam Barth]

> > > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:86
> > > +    OwnHandle<v8::Object> m_wrapper;
> > 
> > OwnHandle is sadly dead as of a week ago (replaced by SharedPersistent), but that doesn't support weak handles. abarth, is the correct thing here to roll-your-own weak handles using v8 API?
> 
> Doh! That sucks, since this worked perfectly :) It'd be unfortunate to have to roll my own weak handle. Can we just add that feature back into the SharedPersistent?

OwnHandle got renamed to ScopedPersistent.  You can still use the weak handle functionality directly from V8.

(I'm going to delete SharedPersistent in favor of updating the one client to use ScopedPersistent as well.)

[Adam Klein]

(In reply to comment #11)
> (In reply to comment #10)
> > Design question: how'd you end up with inheritance for V8WrapperRetainedValue instead of composition? It's a little confusing to me since the V8MutationCallback _itself_ isn't "wrapper-retained", only the underlying v8::Object.
> 
> No reason,  but it does require more manual code in the JSC side without it since you'd have to expose the retainedBy method yourself. I can change to composition if you'd prefer?

If it makes it easier for you to have the V8 and JSC versions match up, that's fine with me, I don't think this will be used often enough for it to be a big deal either way.

> > > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:78
> > > +    static v8::Persistent<v8::String> createPropertyName()
> > 
> > Why do we need a new propertyName each time? I guess this is for generality? Seems like in this case, there's no way for a MutationObserver to have more than one MutationCallback associated with it. Not sure it's worth the complexity for now (for example, I don't think this code is thread-safe).
> 
> UndoManager has an API like:
> 
> undoManager.transact({undo: function() {} });
> undoManager.transact({undo: function() {} });
> undoManager.transact({undo: function() {} });
> 
> And this new generic API allows you do:
> 
> undoManager->transact(DOMTransaction::create(undoManagerWrapper, arg[0]))
> 
> and not worry about maintaining an array, or doing the inversion they have right now where DOMTransaction actually gets it's own wrapper, and then sets a hidden property cycle on it, and then uses a custom GC marking function (which is rather confusing to understand).
> 
> http://code.google.com/p/chromium/source/search?q=V8DomTransaction&origq=V8DomTransaction&btnG=Search+Trunk

Sounds reasonable. If we do go with this new-name-each-time method, then you'll want to make this thread-safe, probably by moving the counter to V8PerIsolateData. I also suspect you'll want some other prefix on there besides the number, otherwise it'd be pretty easy for someone to do the same trick elsewhere and use 'WebCore::HiddenProperty::0' for something else.

[Adam Barth]

Comment on attachment 161068 [details]
Patch

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

This looks like a very plausible approach.  I agree with inferno that we should try to do this with composition.

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:63
>      Vector<char, 64> prefixedName;
>      prefixedName.append(V8_HIDDEN_PROPERTY_PREFIX, sizeof(V8_HIDDEN_PROPERTY_PREFIX) - 1);
>      ASSERT(name && strlen(name));
>      prefixedName.append(name, strlen(name));

We should add a call to prefixedName.reserveCapacity(...)

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:60
> -        static v8::Handle<v8::String> hiddenReferenceName(const char* name);
> +        static v8::Handle<v8::String> hiddenReferenceName(const char* name, bool symbol = true);

Rather than using a raw boolean parameter, can we use an enum?

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:51
> +        m_value.makeWeak();

I don't think you need to keep a handle to m_value.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:52
> +        m_wrapper.makeWeak();

It makes sense to keep a reference to m_wrapper so that you can remove the property if C++ tells you that you don't need it anymore.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:65
> +    v8::Handle<v8::Object> scriptValue() const
> +    {
> +        return m_value.get();
> +    }

This doesn't appear to be needed.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:80
> +        static double nextId = 0;

Please add an ASSERT(mainThread()) because this code isn't thread-safe.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:83
> +        return v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName(numberToString(nextId++, buffer), false));

This isn't sufficient to guarantee the uniqueness of the name.  We need to add the string "V8WrapperRetainedValue" in there somewhere.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:88
> +    v8::Persistent<v8::String> m_propertyName;

This should be a ScopedPersistent.  Currently you're leaking the v8::String.

[Elliott Sprehn]

(In reply to comment #14)
> (From update of attachment 161068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161068&action=review
> 
> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:51
> > +        m_value.makeWeak();
> 
> I don't think you need to keep a handle to m_value.

You do, so you can access the value later for using it as a callback, or as the Object with "undo" etc. later. If we don't keep a handle then we need to go through GetHiddenValue in scriptValue() which requires a hash table lookup.

I chose this approach since you only pay the hash table penalty once with the intention of seeing if we can migrate some of the other custom GC things to this approach as well. Indeed if we switch to using SetHiddenField here we could probably clean up V8EventListener without a perf hit too.

> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:52
> > +        m_wrapper.makeWeak();
> 
> It makes sense to keep a reference to m_wrapper so that you can remove the property if C++ tells you that you don't need it anymore.

We do keep a reference to it, but indirectly through the ActiveDOMObject wrapper. This must be weak by design, otherwise we create a cycle because MutationObserver has a reference to it's own wrapper through this object, which has a reference to the callback, which has a reference to the wrapper, which has a reference to the MutationObserver and we're back where we started. :)

Both are weak intentionally, it's subtle.

> 
> > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:65
> > +    v8::Handle<v8::Object> scriptValue() const
> > +    {
> > +        return m_value.get();
> > +    }
> 
> This doesn't appear to be needed.

I don't understand. You need a getter so V8MutationCallback can get the value back later to use it as a callback function.

[Adam Barth]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 161068 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161068&action=review
> > 
> > 
> > > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:51
> > > +        m_value.makeWeak();
> > 
> > I don't think you need to keep a handle to m_value.
> 
> You do, so you can access the value later for using it as a callback, or as the Object with "undo" etc. later. If we don't keep a handle then we need to go through GetHiddenValue in scriptValue() which requires a hash table lookup.

I feel like we should use GetHiddenValue until we have a benchmark that tells us that it's too slow.

> I chose this approach since you only pay the hash table penalty once with the intention of seeing if we can migrate some of the other custom GC things to this approach as well. Indeed if we switch to using SetHiddenField here we could probably clean up V8EventListener without a perf hit too.

That seems fine, but we should add that complexity when it's needed and we can show the perf win.

> > > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:52
> > > +        m_wrapper.makeWeak();
> > 
> > It makes sense to keep a reference to m_wrapper so that you can remove the property if C++ tells you that you don't need it anymore.
> 
> We do keep a reference to it, but indirectly through the ActiveDOMObject wrapper. This must be weak by design, otherwise we create a cycle because MutationObserver has a reference to it's own wrapper through this object, which has a reference to the callback, which has a reference to the wrapper, which has a reference to the MutationObserver and we're back where we started. :)
> 
> Both are weak intentionally, it's subtle.

Yeah, I understand.  :)

> > > Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:65
> > > +    v8::Handle<v8::Object> scriptValue() const
> > > +    {
> > > +        return m_value.get();
> > > +    }
> > 
> > This doesn't appear to be needed.
> 
> I don't understand. You need a getter so V8MutationCallback can get the value back later to use it as a callback function.

IMHO, we should get it via GetHiddenValue.

The alternative is to add an implicit reference during GC and avoid the storage overhead in the wrapper.  As-is, we're paying the memory overhead twice.

[Elliott Sprehn]

After much research I realize that my approach here for JSC leaks. Instead it should just use PrivateName and model the code in V8DependentRetained (see 95519).

[Geoffrey Garen]

Please add a regression test. You should be able to test for a memory leak using the internals object, looping the leaky code, and measuring memory usage.

[Elliott Sprehn]

Created attachment 164969 [details]
Patch

This also fixes the broken V8DependentRetained and JSDepenendentRetained which never actually worked because they were submitted in patches that don't use them so they never had to compile...

[Adam Barth]

Comment on attachment 164969 [details]
Patch

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

> Source/WebCore/bindings/js/JSDependentRetained.h:85
> +    JSC::Strong<JSDOMGlobalObject> m_globalObject;

It's not a problem to strongly retain the global object?

> Source/WebCore/dom/MutationObserver.h:95
> -    explicit MutationObserver(PassRefPtr<MutationCallback>);
> +    MutationObserver(PassRefPtr<MutationCallback>, ScriptExecutionContext*);

I think we typically pass the ScriptExecutionContext* as the first argument.

[Adam Barth]

The LGTM, with the one JSC question above.

[Adam Klein]

Comment on attachment 164969 [details]
Patch

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

This looks good, attached are a few cleanups you could do now or later.

> Source/WebCore/bindings/js/JSMutationCallback.cpp:69
> +    for (size_t i = 0; i < mutations.size(); ++i)

Looks like JSDOMBinding.h has a helper that'll let you get rid of this loop: jsArray()

> Source/WebCore/bindings/v8/V8MutationCallback.cpp:66
> +    for (size_t i = 0; i < mutations.size(); ++i)

Similar to jsArray(), there's a v8Array() that you can use here.

[Elliott Sprehn]

Comment on attachment 164969 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDependentRetained.h:85
>> +    JSC::Strong<JSDOMGlobalObject> m_globalObject;
> 
> It's not a problem to strongly retain the global object?

Good question. The only place anyone keeps this thing around is in JSCallbackData so I don't have an example. I think you're right that this should be Weak.

>> Source/WebCore/dom/MutationObserver.h:95
>> +    MutationObserver(PassRefPtr<MutationCallback>, ScriptExecutionContext*);
> 
> I think we typically pass the ScriptExecutionContext* as the first argument.

Ah, this is confusing, the code generator puts it last, but people put it first. :P See: generated V8FileCallback vs XMLHttpRequest.

[Adam Barth]

> Ah, this is confusing, the code generator puts it last, but people put it first. :P See: generated V8FileCallback vs XMLHttpRequest.

We should fix the code generator to be more like people.  ;)

[Build Bot]

Comment on attachment 164969 [details]
Patch

Attachment 164969 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13953261

[Elliott Sprehn]

(In reply to comment #25)
> (From update of attachment 164969 [details])
> Attachment 164969 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/13953261

I don't know how to fix this link error. I added JS_EXPORT_PRIVATE and that fixes the mac build. How do I export symbols for windows?

[Ryosuke Niwa]

Add the symbol to .def file.

[Elliott Sprehn]

Created attachment 164986 [details]
Patch

Attempt to fix the windows build.

[WebKit Review Bot]

Attachment 164986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Elliott Sprehn]

Created attachment 164987 [details]
Patch

Attempt to fix the windows build.

[Geoffrey Garen]

Comment on attachment 164987 [details]
Patch

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

> Source/WebCore/bindings/js/JSMutationCallback.h:55
> +    JSC::Strong<JSDOMGlobalObject> m_globalObject;

This is a reference cycle.

[Elliott Sprehn]

(In reply to comment #31)
> (From update of attachment 164987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164987&action=review
> 
> > Source/WebCore/bindings/js/JSMutationCallback.h:55
> > +    JSC::Strong<JSDOMGlobalObject> m_globalObject;
> 
> This is a reference cycle.

That's a JSC::Weak in the patch you r-'ed.

[Elliott Sprehn]

Comment on attachment 164987 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSMutationCallback.h:55
>>> +    JSC::Strong<JSDOMGlobalObject> m_globalObject;
>> 
>> This is a reference cycle.
> 
> That's a JSC::Weak in the patch you r-'ed.

Err, my bad, I was looking at the one in JSDependentRetained. What should I use here instead? Can I just use a Weak?

[Adam Klein]

What's the status of this patch?

[Elliott Sprehn]

(In reply to comment #34)
> What's the status of this patch?

I was going to attempt it again this week. I got side tracked killing the NodeRareData map in Bug 100057. :)

[Elliott Sprehn]

(In reply to comment #31)
> (From update of attachment 164987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164987&action=review
> 
> > Source/WebCore/bindings/js/JSMutationCallback.h:55
> > +    JSC::Strong<JSDOMGlobalObject> m_globalObject;
> 
> This is a reference cycle.

Geoffrey, How should I be solving this? You're saying this is a cycle because the wrapper for MutationObserver has a JSC::Strong for the window as well?

[Geoffrey Garen]

> Geoffrey, How should I be solving this? 

I guess that depends. What behavior do you want?

> You're saying this is a cycle because the wrapper for MutationObserver has a JSC::Strong for the window as well?

JSMutationCallback::m_globalObject is a leak because it's a strong reference, so if anything in the DOM references the callback, you have a retain cycle.

[Adam Barth]

> I guess that depends. What behavior do you want?

We have a whiteboard diagram somewhere that explains what we're trying to accomplish, but that's hard to communicate over the interwebs.

In Bug 102328, we're making MutationObserver into an ActiveDOMObject.  While the MutationObserver has pending activity or while the MutationObserver wrapper is still referenced from a live JavaScript object, we'd like to keep the MutationObserver wrapper and its callback alive.  Once that is no longer the case, we'd like the wrapper and the callback to be collected by the garbage collector.

[Adam Klein]

(In reply to comment #37)
> > Geoffrey, How should I be solving this? 
> 
> I guess that depends. What behavior do you want?
> 
> > You're saying this is a cycle because the wrapper for MutationObserver has a JSC::Strong for the window as well?
> 
> JSMutationCallback::m_globalObject is a leak because it's a strong reference, so if anything in the DOM references the callback, you have a retain cycle.

I'm curious why is this is not also a problem for the V8 side of the patch. V8MutationCallback::m_worldContext holds a v8::Persistent handle to a v8::Context.

[Adam Barth]

v8::Persistent is poorly named.  v8::Persistent just means "raw pointer to a V8 object".  It doesn't have any lifetime implications.

[Elliott Sprehn]

Someone needs to fix JSC for circular references and this is resolved.

[Adam Barth]

We probably need to do the same two things we did for V8, right?

1) Integrate MutationObserver with GC opaque roots so that its wrapper has the right lifetime.
2) Store the callback object on the MutationObserver wrapper using a PrivateName.

It looks like part (1) is done in bug 102744 and WebCore can use PrivateName as of bug 102546.  That means we just need to move the storage of the callback to the wrapper.

[Adam Barth]

I can take a look at this today.

[Adam Barth]

> I can take a look at this today.

I've decided not to work on this bug today.

[Geoffrey Garen]

<rdar://problem/12967528>

[Adam Klein]

Note that in http://trac.webkit.org/changeset/138841 Chromium stopped using the IDL file to generate V8MutationCallback, since its implementation had diverged almost completely from other callbacks. I suspect that a similar change on the JSC side would be the easiest way forward. The two bits of behavior are:

1. JSMutationCallback should only hold a weak handle to the callback function.
2. A PrivateName should be added on the JSMutationObserver wrapper to the callback function.

(1) is problematic while JSMutationCallback is generated from an IDL, since a new weak version of JSCallbackData would be required. I suspect (though I'm not positive) that a completely custom version won't need the same thread-handling code in JSCallbackData, since mutation observers are only used on the main thread.

[Adam Klein]

Geoffrey, is anyone more familiar with JSC than I or Elliott planning on fixing this? If not I'm likely to give it another shot.

[Adam Klein]

Created attachment 185055 [details]
WIP

[Adam Klein]

See WIP patch. The only things possibly wrong with it are

1) Lack of a test
2) Use of WeakHandleOwner to keep the callback alive. I believe this will result in it taking two GC cycles to free the callback. First cycle will be able to collect the MutationObserver wrapper, and the second will see the wrapper is gone and collect the callback. But perhaps this could be accomplished in one cycle using a PrivateName? I only hesitated on that since it's not commonly used in JSC bindings, but if it's a cleaner approach, adding it to JSMutationObserverCustom.cpp should be doable.

I'd like feedback especially on (2), but also on the rest of the new stuff in JSMutationCallback (I've reorganized how the callback works to store a DOMWrapperWorld instead of a JSDOMGlobalObject, for example).

[Adam Klein]

Created attachment 185063 [details]
WIP: PrivateName alternative

[Adam Klein]

The PrivateName implementation seems noticeably simpler, fwiw.

[Adam Barth]

Comment on attachment 185063 [details]
WIP: PrivateName alternative

The PrivateName approach looks fine.  One reason PrivateName might not be used much in the bindings is that it's a relatively new addition to JSC (or at least newly exposed to WebCore).

[Adam Klein]

Okay, so that leaves the test. ggaren referred to memory usage being exposed on internals, but I don't see that (nor do I see it listed here: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree). What am I missing?

[Build Bot]

Comment on attachment 185063 [details]
WIP: PrivateName alternative

Attachment 185063 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16152812

[Adam Barth]

I'm not sure how to write a test for this patch.

[Adam Klein]

Created attachment 185095 [details]
Patch

[Adam Barth]

Comment on attachment 185095 [details]
Patch

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

This looks good provided it's safe to ref the DOMWrapperWorld.

> Source/WebCore/bindings/js/JSMutationCallback.h:47
> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE { return ContextDestructionObserver::scriptExecutionContext(); }

I think you can do this with the "using" directive.

> Source/WebCore/bindings/js/JSMutationCallback.h:53
> +    RefPtr<DOMWrapperWorld> m_isolatedWorld;

Is it safe to ref the DOMWrapperWorld?  For example, does the DOMWrapperWorld keep the global object alive, which might keep the mutation observer instance alive, which keeps the JSMutationCallback alive?

[Adam Klein]

Comment on attachment 185095 [details]
Patch

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

>> Source/WebCore/bindings/js/JSMutationCallback.h:47
>> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE { return ContextDestructionObserver::scriptExecutionContext(); }
> 
> I think you can do this with the "using" directive.

I think we had this same discussion with V8MutationCallback :). The compiler won't let me get away with just "using"; it won't recognize the "using" as an implementation of MutationCallback::scriptExecutionContext and complains that JSMutationCallback can't be instantiated.

>> Source/WebCore/bindings/js/JSMutationCallback.h:53
>> +    RefPtr<DOMWrapperWorld> m_isolatedWorld;
> 
> Is it safe to ref the DOMWrapperWorld?  For example, does the DOMWrapperWorld keep the global object alive, which might keep the mutation observer instance alive, which keeps the JSMutationCallback alive?

I can tell you that JSEventListener holds a RefPtr<DOMWrapperWorld> in what seems to be a similar situation, but I don't know enough of the details to be sure this isn't still a leak. ggaren's input would be useful here.

[Adam Barth]

(In reply to comment #58)
> (From update of attachment 185095 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185095&action=review
> 
> >> Source/WebCore/bindings/js/JSMutationCallback.h:47
> >> +    virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE { return ContextDestructionObserver::scriptExecutionContext(); }
> > 
> > I think you can do this with the "using" directive.
> 
> I think we had this same discussion with V8MutationCallback :). The compiler won't let me get away with just "using"; it won't recognize the "using" as an implementation of MutationCallback::scriptExecutionContext and complains that JSMutationCallback can't be instantiated.

Probably.  :)

> >> Source/WebCore/bindings/js/JSMutationCallback.h:53
> >> +    RefPtr<DOMWrapperWorld> m_isolatedWorld;
> > 
> > Is it safe to ref the DOMWrapperWorld?  For example, does the DOMWrapperWorld keep the global object alive, which might keep the mutation observer instance alive, which keeps the JSMutationCallback alive?
> 
> I can tell you that JSEventListener holds a RefPtr<DOMWrapperWorld> in what seems to be a similar situation, but I don't know enough of the details to be sure this isn't still a leak. ggaren's input would be useful here.

Makes sense.

[Geoffrey Garen]

LGTM.

RefPtr<DOMWrapperWorld> is OK because all references from DOMWrapperWorld to JS are weak / garbage collected.

[WebKit Review Bot]

Comment on attachment 185095 [details]
Patch

Clearing flags on attachment: 185095

Committed r141296: <http://trac.webkit.org/changeset/141296>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.