812617 - Add a Web IDL binding configuration to not generate the _addProperty hook

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Boris volunteered.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 3

[Peter Van der Beken [:peterv]]

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?

Comment 4

[(no longer active)]

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

Comment 5

[Peter Van der Beken [:peterv]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[(no longer active)]

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

Comment 8

[Peter Van der Beken [:peterv]]

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

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Call JSBindingFinalized() if provided v1

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 10

[(no longer active)]

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!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Over to the man with the patch...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 13

[Peter Van der Beken [:peterv]]

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 14

[(no longer active)]

Comment on attachment 685135 [details] [diff] [review]
Call JSBindingFinalized() if provided v1

This works perfectly when I updated it to apply on trunk.

Comment 15

[(no longer active)]

Attachment: Call JSBindingFinalized() if provided v2

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 17

[(no longer active)]

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

Comment 18

[(no longer active)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ea947ec162a1

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea947ec162a1

Comment 20

[(no longer active)]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

I would probably be fine either way.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

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.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 40

[(no longer active)]

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

Comment 41

[Karl Tomlinson (:karlt)]

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

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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