We need this for Web Audio in order to be able to manage the lifetime of the graph objects properly.

Boris volunteered.

I missed some of the discussion preceding this, but after getting more info from bz I am wondering if a finalizer hook won't be enough for Web Audio's use case?

(In reply to comment #3) > I missed some of the discussion preceding this, but after getting more info > from bz I am wondering if a finalizer hook won't be enough for Web Audio's use > case? What is a finalizer hook?

A function that's called when the JS binding object is finalized by the GC.

So basically, we'd flag your object as not wrappercached, but add a new annotation that would cause a method like JSObjectFinalized() or something to be called on your object when that happens.

(In reply to comment #6) > So basically, we'd flag your object as not wrappercached, but add a new > annotation that would cause a method like JSObjectFinalized() or something to > be called on your object when that happens. Yeah I think that would be enough for our needs.

(In reply to Boris Zbarsky (:bz) from comment #6) > but add a new > annotation that would cause a method like JSObjectFinalized() or something > to be called on your object when that happens. Or use our HAS_MEMBER templates to detect the existence of the method.

This calls a JSBindingFinalized() on the native if the native's class has one. Let me know if this works for you, I'd rather do this than attachment 682816 [details] [diff] [review]. We might be able to migrate worker's customFinalize hook to this instead.

Comment on attachment 685135 [details] [diff] [review] Call JSBindingFinalized() if provided v1 This looks good to me, but I think roc has the unfinished patches which would actually use this. roc, if you have those patches, can you please try this patch? If not, I can knock up a patch to use this and then verify that it works as expected (although I need to simulate the playback end status somehow arbitrarily...). Thanks!

Over to the man with the patch...

We can't really use this until we implement GainNode (or some other node that has inputs and can only be destroyed when the JS wrapper has been removed). The only two nodes I have implemented right now are AudioBufferSourceNode (which can remove itself from the graph even if the JS wrapper is alive, because you can't make it play again anyway), and AudioDestinationNode (which is permanently kept alive by its AudioContext anyway).

Comment on attachment 682816 [details] [diff] [review] Add a way to generate WebIDL bindings which assume you have a wrapper cache but do not try to preserve the wrapper. that we require that interfaces with such bindings are only returned from [Creator] methods. Review of attachment 682816 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather not do this if we can avoid it.

Comment on attachment 685135 [details] [diff] [review] Call JSBindingFinalized() if provided v1 This works perfectly when I updated it to apply on trunk.

Comment on attachment 725962 [details] [diff] [review] Call JSBindingFinalized() if provided v2 r=me

I'll land this with the proper author lin assuming that the tree is reopened some day...

Now that Web Audio has switched away from using JSBindingFinalized, do we want to back this patch out?

I would probably be fine either way.

We're going to need the hook for Web Audio anyway.

Actually I'm not sure this JSBindingFinalized hook is going to be enough. The JSBindingFinalized hook only gets called on classes that don't extend nsWrapperCache (since if there is a wrapper cache, it will keep the JS object alive as long as the host object). In bug 853298 we made every AudioNode an EventTarget to follow the spec, making every node inherit from nsWrapperCache. However, most nodes don't fire events per spec, and thus don't need the wrapper cache, and for those that do, we don't need to cache the wrapper after we know the last event has fired. So I think we really still want something like bug 853298 comment #11, allowing us to dynamically disable the wrapper cache on a per-object basis, at least via nsDOMEventTargetHelper. Does that make sense?

A wrapper cache doesn't automatically keep the JS object alive. It only keeps it alive if someone asks it to. Currently addproperty hooks do that automatically. If you don't need wrapper preservation on expando sets, we could resurrect the very first patch in this bug, I would think. Would that give us the behavior we want? Or does nsDETH end up preserving the wrapper itself sometimes?

I guess relying on no one else preserving the wrapper is kinda fragile. :(

(In reply to Boris Zbarsky [:bz] from comment #24) > If you don't need wrapper preservation on expando sets, we could resurrect > the very first patch in this bug, I would think. What does that mean exactly? I don't see how a solution that only affects codegen would work. It seems to me we need a new, runtime API that stuff like AudioBufferSourceNode can indicate that although a wrapper needed to be preserved, it doesn't anymore because we won't fire an event at the object ever again.

> What does that mean exactly? Never mind; for an event target my "if you don't need" is just clearly false. Can audio nodes just ReleaseWrapper(this) at the point when they no longer need to preserve the JS object? Seems like that should do what you want.

OK, that sounds great. Is it safe to call ReleaseWrapper(this) on some nodes immediately after wrapping the node? E.g. in GainNode::WrapObject, immediately after it calls GainNodeBinding::Wrap? If not, how should I handle the case where an object's wrapper can be released immediately?

ReleaseWrapper is safe to call at that point, but not enough on its own, since something can end up preserving the wrapper afterward... I guess what you want is something that both calls ReleaseWrapper and prevents preservation after that point. I _think_ we should have a free flag bit here that we could probably use for that.

(In reply to Boris Zbarsky [:bz] from comment #29) > ReleaseWrapper is safe to call at that point, but not enough on its own, > since something can end up preserving the wrapper afterward... What sort of "something"? > I guess what you want is something that both calls ReleaseWrapper and > prevents preservation after that point. I _think_ we should have a free > flag bit here that we could probably use for that. It would be nice to be able to flip this state on and off. For example, with AudioBufferSourceNode we might have a looping node that is never going to fire the "ended" event if undisturbed, so we need not preserve the wrapper, but if the app calls stop() on it then an "ended" event will fire and we need to start preserving the wrapper.

> What sort of "something"? The most obvious something is the addproperty hook we autogenerate. So the page creates a GainNode, then sets node.someExpando, and that will try to preserve the wrapper. > we might have a looping node that is never going to fire the "ended" event if > undisturbed, so we need not preserve the wrapper, but if the app calls stop() on it then > an "ended" event will fire and we need to start preserving the wrapper. I see. So a function to clear the "prevent wrapper preservation" flag, presumably.

(In reply to Boris Zbarsky [:bz] from comment #24) > Or does nsDETH end up preserving the wrapper itself > sometimes? No, DETH doesn't do any preserved wrapper magic. (except releases wrapper when unlinked/deleted)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > but if the app calls stop() on it then an "ended" event will fire and we > need to start preserving the wrapper. Why we need to preserve wrapper in this case? Can we not just addref when stop is called, and release when 'ended' has been dispatched or something?

We need to keep the JS object alive. Consider this code: var node = createTheNode(); node.addEventListener("ended", function(e) { alert(e.target.foo); }); node.foo = "PASS"; node.stop(); node = null; gc(); This needs to alert "PASS". We _could_ have the C++ object stash the wrapper in a traced Heap member when stop() is called and then null it out after the event fires, I guess, but that effectively manually simulates wrapper preservation/unpreservation, though does have the virtue of being more explicit.

That preservers the wrapper since there is expando object. So if you addref, node isn't collected by cc nor gc. But no manual PreserveWrapper should be needed in that case.

> That preservers the wrapper since there is expando object. Yes, but the point is we want to unpreserve it again once the event has finished firing. And in general, we want to unpreserve up front for that sort of node, but then re-preserve if we end up in the situation above...

You don't want to unpreserve always. Event listener may do something which keeps the event object alive, and that event has .target pointing to node, and node.foo shouldn't disappear.

> Event listener may do something which keeps the event object alive Argh. That's really annoying. So how do we avoid leaking in the case when the event object is _not_ kept alive?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > So I think we really still want something like bug 853298 comment #11, > allowing us to dynamically disable the wrapper cache on a per-object basis, > at least via nsDOMEventTargetHelper. Does that make sense? And because of the event.target like cases, all the event targets needs to be wrappercache objects, and that wrapper caching must not be disabled ever.

(In reply to comment #39) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > > So I think we really still want something like bug 853298 comment #11, > > allowing us to dynamically disable the wrapper cache on a per-object basis, > > at least via nsDOMEventTargetHelper. Does that make sense? > And because of the event.target like cases, all the event targets needs to be > wrappercache objects, and > that wrapper caching must not be disabled ever. What if we know that we will never dispatch an event on that target? (I guess manually calling dispatchEvent on the object will have the same result?) Sigh...

(In reply to Olli Pettay [:smaug] from comment #37) > You don't want to unpreserve always. Event listener may do something which > keeps the event object > alive, and that event has .target pointing to node, and node.foo shouldn't > disappear. If the event object is still alive, then I assume it will keep the JS wrapper alive regardless of whether the C++ object is preserving or not, and that is expected/OK. If the current state of the C++ object is that it won't dispatch the event (at least not unless there is a change performed through the JS wrapper), then the wrapper does not need to be preserved. (In reply to :Ehsan Akhgari (needinfo? me!) from comment #40) > (I guess manually calling dispatchEvent on the object will have the same > result?) I assume something would need to keep the JS wrapper alive to call dispatchEvent on it.

(In reply to Karl Tomlinson (:karlt) from comment #41) > (In reply to Olli Pettay [:smaug] from comment #37) > > You don't want to unpreserve always. Event listener may do something which > > keeps the event object > > alive, and that event has .target pointing to node, and node.foo shouldn't > > disappear. > > If the event object is still alive, then I assume it will keep the JS > wrapper alive regardless of whether the C++ object is preserving or not, and > that is expected/OK. No. The wrapper needs to be preserved to make C++ to keep it alive. Event object is implemented in C++ and it keeps reference to a C++ implemented EventTarget (.target property). It is up to the EventTarget object to make sure its wrapper is preserved when there are expando properties. > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #40) > > (I guess manually calling dispatchEvent on the object will have the same > > result?) > > I assume something would need to keep the JS wrapper alive to call > dispatchEvent on it. The question is what happens after dispatchEvent. You take reference to the event object in the event listener and ask its .target way after dispathEvent has returned and gc has run and what not. event.target.foo should still be there.

(In reply to Boris Zbarsky [:bz] from comment #31) > > What sort of "something"? > > The most obvious something is the addproperty hook we autogenerate. So the > page creates a GainNode, then sets node.someExpando, and that will try to > preserve the wrapper. Ah, I'm getting a better idea of how wrapper preservation works now. So apparently if I create an AudioNode, but no expando is set on it, I can expect that GC will be able to collect the wrapper when all references to the wrapper are dropped. That means we can go ahead and fix bug 897796 for a lot of common cases, while we figure out what to do here for the case of expandos and other wrapper-preservation situations.