Bug 93661

Summary: [JSC] MutationObservers should not create circular, leaky references
Product: WebKit Reporter: Adam Klein <adamk>
Component: DOMAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, enrica, esprehn, fpizlo, ggaren, gyuyoung.kim, haraken, japhet, ojan.autocc, ojan, rafaelw, rakuco, rniwa, sukolsak, tommyw, webkit.review.bot
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 95519, 96034, 97955, 97956, 102328, 102546, 102744    
Bug Blocks: 68729    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP
none
WIP: PrivateName alternative
none
Patch none

Description Adam Klein 2012-08-09 14:58:26 PDT
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.
Comment 1 Elliott Sprehn 2012-08-17 12:59:19 PDT
Do you know where to start on this? What code should I be looking at that does this already?
Comment 2 Adam Klein 2012-08-17 16:27:24 PDT
bindings/v8/*EventListener* is probably a good start, at least to get familiar with the issues involved (weak handles, hidden properties).
Comment 3 Adam Barth 2012-08-19 13:38:57 PDT
> 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.
Comment 4 Adam Barth 2012-08-19 13:43:08 PDT
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.
Comment 5 Elliott Sprehn 2012-08-28 13:55:35 PDT
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?
Comment 6 Elliott Sprehn 2012-08-28 14:09:17 PDT
Comment on attachment 161050 [details]
Patch

Sorry, my JSC code and compile was busted. Trying to fix that now.
Comment 7 Elliott Sprehn 2012-08-28 15:02:57 PDT
Created attachment 161068 [details]
Patch

Fix XCode build files.
Comment 8 Elliott Sprehn 2012-08-28 15:07:42 PDT
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.
Comment 9 Elliott Sprehn 2012-08-28 15:16:12 PDT
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.
Comment 10 Adam Klein 2012-08-28 15:34:30 PDT
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.
Comment 11 Elliott Sprehn 2012-08-28 15:57:10 PDT
(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?
Comment 12 Adam Barth 2012-08-28 16:02:59 PDT
> > > 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.)
Comment 13 Adam Klein 2012-08-28 16:07:57 PDT
(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.
Comment 14 Adam Barth 2012-08-28 16:18:16 PDT
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.
Comment 15 Elliott Sprehn 2012-08-28 18:29:44 PDT
(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.
Comment 16 Adam Barth 2012-08-28 18:43:08 PDT
(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.
Comment 17 Elliott Sprehn 2012-09-05 17:52:43 PDT
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).
Comment 18 Geoffrey Garen 2012-09-06 16:23:57 PDT
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.
Comment 19 Elliott Sprehn 2012-09-20 13:15:27 PDT
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...
Comment 20 Adam Barth 2012-09-20 13:22:53 PDT
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.
Comment 21 Adam Barth 2012-09-20 13:23:22 PDT
The LGTM, with the one JSC question above.
Comment 22 Adam Klein 2012-09-20 13:47:38 PDT
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.
Comment 23 Elliott Sprehn 2012-09-20 13:51:17 PDT
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.
Comment 24 Adam Barth 2012-09-20 13:54:29 PDT
> 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.  ;)
Comment 25 Build Bot 2012-09-20 14:08:16 PDT
Comment on attachment 164969 [details]
Patch

Attachment 164969 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13953261
Comment 26 Elliott Sprehn 2012-09-20 14:12:39 PDT
(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?
Comment 27 Ryosuke Niwa 2012-09-20 14:19:42 PDT
Add the symbol to .def file.
Comment 28 Elliott Sprehn 2012-09-20 14:31:03 PDT
Created attachment 164986 [details]
Patch

Attempt to fix the windows build.
Comment 29 WebKit Review Bot 2012-09-20 14:34:44 PDT
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.
Comment 30 Elliott Sprehn 2012-09-20 14:35:52 PDT
Created attachment 164987 [details]
Patch

Attempt to fix the windows build.
Comment 31 Geoffrey Garen 2012-09-20 21:08:47 PDT
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.
Comment 32 Elliott Sprehn 2012-09-28 16:40:15 PDT
(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.
Comment 33 Elliott Sprehn 2012-09-28 17:24:49 PDT
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?
Comment 34 Adam Klein 2012-11-08 02:24:55 PST
What's the status of this patch?
Comment 35 Elliott Sprehn 2012-11-08 02:31:15 PST
(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. :)
Comment 36 Elliott Sprehn 2012-11-14 20:31:04 PST
(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?
Comment 37 Geoffrey Garen 2012-11-15 11:16:47 PST
> 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.
Comment 38 Adam Barth 2012-11-15 11:23:16 PST
> 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.
Comment 39 Adam Klein 2012-11-15 15:34:09 PST
(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.
Comment 40 Adam Barth 2012-11-15 15:38:52 PST
v8::Persistent is poorly named.  v8::Persistent just means "raw pointer to a V8 object".  It doesn't have any lifetime implications.
Comment 41 Elliott Sprehn 2012-11-28 22:50:09 PST
Someone needs to fix JSC for circular references and this is resolved.
Comment 42 Adam Barth 2012-11-29 08:43:50 PST
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.
Comment 43 Adam Barth 2012-11-29 08:44:07 PST
I can take a look at this today.
Comment 44 Adam Barth 2012-11-29 16:07:10 PST
> I can take a look at this today.

I've decided not to work on this bug today.
Comment 45 Geoffrey Garen 2013-01-07 11:55:08 PST
<rdar://problem/12967528>
Comment 46 Adam Klein 2013-01-07 16:08:52 PST
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.
Comment 47 Adam Klein 2013-01-08 14:27:41 PST
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.
Comment 48 Adam Klein 2013-01-28 13:59:53 PST
Created attachment 185055 [details]
WIP
Comment 49 Adam Klein 2013-01-28 14:06:15 PST
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).
Comment 50 Adam Klein 2013-01-28 14:28:39 PST
Created attachment 185063 [details]
WIP: PrivateName alternative
Comment 51 Adam Klein 2013-01-28 14:30:07 PST
The PrivateName implementation seems noticeably simpler, fwiw.
Comment 52 Adam Barth 2013-01-28 15:00:41 PST
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).
Comment 53 Adam Klein 2013-01-28 15:04:17 PST
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?
Comment 54 Build Bot 2013-01-28 15:04:40 PST
Comment on attachment 185063 [details]
WIP: PrivateName alternative

Attachment 185063 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16152812
Comment 55 Adam Barth 2013-01-28 15:19:12 PST
I'm not sure how to write a test for this patch.
Comment 56 Adam Klein 2013-01-28 16:24:32 PST
Created attachment 185095 [details]
Patch
Comment 57 Adam Barth 2013-01-28 17:14:32 PST
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?
Comment 58 Adam Klein 2013-01-28 17:21:13 PST
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.
Comment 59 Adam Barth 2013-01-28 17:26:34 PST
(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.
Comment 60 Geoffrey Garen 2013-01-30 11:56:02 PST
LGTM.

RefPtr<DOMWrapperWorld> is OK because all references from DOMWrapperWorld to JS are weak / garbage collected.
Comment 61 WebKit Review Bot 2013-01-30 12:24:18 PST
Comment on attachment 185095 [details]
Patch

Clearing flags on attachment: 185095

Committed r141296: <http://trac.webkit.org/changeset/141296>
Comment 62 WebKit Review Bot 2013-01-30 12:24:28 PST
All reviewed patches have been landed.  Closing bug.