980506 - Implement AudioNode destruction events for WebAudioActor

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

This is needed so the web audio editor can remove nodes created via the actor, especially useful in environments where a lot of BufferSourceNodes are created and immediately GC'd

Comment 1

[Paul Adenot (:padenot)]

roc suggested implementing this by having a new kind of js object you would set on AudioNodes as an expando, that would call a user-specified callback when the node is about the be collected.

Something like (bikeshed welcome for the names and api):

> var n = ac.createBufferSource();
> n.buffer = lalala;
> n.__oncollection = new CollectionObserver(n, oncollection);
> function oncollection(obj) { console.log(obj + " is being collected."); }
> n.start();

This would be Chrome only. It would also be observable from content, but I think it's acceptable.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Paul, I think that's a fine solution. As it's chrome only, for my use case I'm not too worried about names or API. Do you have a timeline for this GC observer?

Comment 3

[Paul Adenot (:padenot)]

Not really.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

I can take care of this if you can point me in the right direction on where/how to add this!

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Smaug, was told you may be the right person to ask for this :)

With a rough first pass of the web audio editor landed in devtools (bug 980503, bug 980502), looking at ways to make the experience more usable and provide more insight in an audio context. The tool currently lays out all the audio nodes used in the context, and connects them together, in a SVG graph, and provides inspecting tools for the audio nodes, etc..

...downside with this approach is there is no explicit "delete node" function for the Web Audio API, and mostly rely on GC to determine if a node is no longer used in an environment. For common uses of AudioBufferSourceNodes, this is usually a show-stopper, as they can be used many times a second just for one quick sound, and the graph is polluted with now-dead audio nodes that are very short lived.

So that's the background! Do you have any insight on how or what to implement to get a chrome-only GC callback/event when an AudioNode is collected for use with dev tools?

Comment 6

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

Didn't Yoric implement a service for that - to know when a gc thing is deleted.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Perhaps http://dxr.mozilla.org/mozilla-central/source/toolkit/components/finalizationwitness/nsIFinalizationWitnessService.idl

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

First draft on some GC work.. Very out of my league for implementing it in the webidl/C++ side, so using the FinalizationWitness in the tooling only. Not yet working, suspect maybe there's something keeping in reference.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-event-audionodes.patch

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

So currently using FinalizationWitness attached as an expando to an -unwrapped- AudioNode. This fires an observer notification that can be listened to (emits the actorID of the actor that weakly held the AudioNode so the front end can clean it up) when it becomes GC'd, so when the AudioNode becomes collected, the witness object too will be collected, giving us an event when AudioNode is collected.

That being said, went through the current code, removing anything that would be holding a strong reference to the unwrapped AudioNode, preventing GC (and nothing holding the XrayWrapper AudioNode either). I could be preventing GC somewhere, and unable to get this GC notification, but interested in knowing more about how AudioNode GC works:

when does a node become collected? can it become collected falling out of scope, and when it's no longer able to receive audio data? Can you imagine scenarios where an AudioNode will become GC'd sooner than expected (i know there's a webkit bug with `onaudioprocess` falling out of scope and stops firing)?

Would this create nodes that become subsequently GC'd?
`for (let i = 10; --i;) ctx.createOscillator()`

Pretty much looking for info on how AudioNode GC works generally speaking, and anything I should be looking out for :)

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Pinging Team Audio regarding the above GC/AudioNode questions :)

Comment 12

[(no longer active)]

Can you first explain to me what you're trying to do with this JS finalization witness thing?  That only notifies you when the JS wrapper goes away.  Why are you interested in knowing that?

As to your question about AudioNode lifetime rules, we basically implement <http://webaudio.github.io/web-audio-api/#lifetime-1>.  In short, we let native AudioNode objects live as long as they can play back anything.  I have no idea why you think that this finalization witness service can give you anything useful (having looked at its implementation).

Comment 13

[(no longer active)]

In order to actually suggest a solution, depending on what exactly you're trying to do, you're going to need to hook into places such as AudioNode::DestroyMediaStream().

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

This is for the web audio editor dev tool, which renders all AudioNodes in a directed graph, and allows you to modify their AudioParam values. There are many use cases where AudioBufferSourceNode's are rapid-fire created, wired to the destination, and then no longer used, so in the editor tool, many buffer nodes render on the graph, but aren't used for longer than a second, and we need a way to alert the front end of the tool that this node is dead/no-longer used.

With the finalization witness, it's attached to the AudioNode, so that when the AudioNode is GC'd, so is the witness, providing a notification. If there's another/better way, maybe in the C++ side of things, to have a chrome-only callback before GC, that'd be more straight forward I think.

Comment 15

[(no longer active)]

First of all, so happy to hear about the plans for a Web Audio devtool!  :-)

So, let's first fix our terminology.  When you talk about GC, the first thing that comes to mind is Javascript GC.  For a pure JS object, when GC kills it, that is the end of the story, but for DOM objects that are backed by a C++ object, the story is a bit more complicated.  In general, C++ DOM objects are reference counted, which means that an object gets freed when the last strong reference to it is released.  When we hand off one of these C++ objects to JS, we create a JS object which "wraps" the C++ DOM object.  That is the JS object that you get back from things such as AudioContext.createFoo().  Each JS wrapper object holds one strong reference to the underlying C++ object, and when the JS GC kills one of these objects, they release their strong reference.  But other things inside Gecko may still have strong references to these objects, so the JS wrapper dying doesn't necessarily mean that the native C++ object would immediately die too (and in practice that almost never happens.)  In some cases we might even "resurrect" the JS wrapper for a native object if the script has a way to get access to the same object again after its wrapper was GCed once.  To give you an example of that case, for example if you call getElementById() and assign the result to a local variable, that local JS object may be GCed when the JS function returns (because the variable goes out of scope and is no longer accessible) but that doesn't kill the underlying native object, and later on if you do getElementById() again, we "resurrect" the JS wrapper for the same underlying native object.

Now, the finalization witness can *only* tell you when one of these wrapper objects is killed, which is not very interesting for the purposes of your tool as the underlying object will live on until it's still needed (and remember that the side effects of this are observable because the audio will keep playing back even after the JS wrapper dies).  So you don't want to change the shape of the graph in your tool when the JS wrapper dies.

If you want to keep the shape of your graph updated according to what's happening inside Gecko, we need to design an API for you (I'm assuming that you can't just access the underlying C++ objects directly.)  Now, one huge pain will be the fact that we can't just add [ChromeOnly] properties and methods for you, since those methods and properties will still be defined on the JS wrapper, so by definition they will all become inaccessible when the JS wrapper dies.  And you don't want to deal with the content facing JS objects any way, since as I mentioned above, holding one of those objects alive will keep that strong reference alive, so it will affect the lifetime of the underlying native object.  Unfortunately we don't have weak references in JS and a native object cannot have one chrome only JS wrapper in addition to the content facing JS wrapper, so we need to basically invent a solution.

In order to do that, I would like to know more about how your tool is constructed.  So some background info on that would be super helpful.  Also, I would like to know what kind of an API you would ultimately like to have, given what I said above (let's for now pretend we're in the land of rainbows and unicorns, so don't worry if what you're asking for is not possible to implement yet! we'll fix that.  ;-)

Also, one important question.  How much C++ code are you willing to have in your tool?  And how feasible is having some C++ code?  I'm assuming the code that constructs the visual graph is all JS + HTML/XUL?

Comment 16

[(no longer active)]

Oh, one thing that I forgot to say: we do some sophisticated tricks inside Gecko to manage the lifetime of the C++ AudioNode objects, but it's not technically GC.  Not sure if you were referring to that in comment 14 or not.  But exposing any kind of JS object which refers to those C++ objects will change the results of those tricks, so we need to be really careful to make sure that observing this system doesn't change its state.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

The trick we used with the FinalizationWitness to ensure that we don't change state is to send a notification at some point after the end of the GC phase that collects the object (well, truly, at the next tick). Don't know if this can work here.

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Thanks for all the info Ehsan and Yoric!

For some more background in the tool, a rough version is in Aurora 31, with a few changes coming down the pipe for 32 -- here's a quick gif[1] of the tool in action. Opening the tool on a page when the audio API is used will scaffold the context graph with nodes, and updates on node creation/connecting, and you can click a node to further inspect its properties and modify. Lots of features planned/discussed[2] in the future after some brainstorming, some which may require more structural changes[3]. How the tool itself works is, we have a web audio actor[3] that listens for a `content-document-global-created` event on a page and instruments the AudioContext and AudioNodes and fires events to the front end (the dev tools panel) on events like, AudioContext created, node A connected to node B, and can send setParam events back to the actor to assign values onto AudioParams. Pretty straight forward, but you can see without some kind of 'destroy' event, any usage of the API that involves a lot of node creation for quick first sounds (games?), we'll never remove the nodes from the front end without knowing when something is no longer used.

So the components of this tool is an actor (in JS which instruments the AudioContext and fires events) and a front end (which receives the events and draws the charts, just a XUL page with JS). In terms of what properties I have access to, it's no different than what the DOM has access to (although with chrome privs), so no direct access to the C++ AudioNodes, unfortunately. ChromeOnly APIs could work for other feature enhancements (listed in [4]), but for the GC portion, yeah, even having some API on the JS Wrapper would need to get collected, unless it could fire right before it gets collected, but not sure how all that works. So to answer your question of how much C++ would I like, it's a bit out of my element, but the tool itself doesn't use any C++ directly, just the AudioNode's from the DOM. And definitely wouldn't want to break any of the strict lifetime rules governing the AudioNode's :) Is there some sort of master Web Audio object that keeps track of all the nodes that could maybe have an API? Fires events when things drop from the lifetime? Not really sure what those options are..


[1] http://i.imgur.com/It5YmGR.gif
[2] https://gist.github.com/jsantell/ee1be41bc87968d7b170
[3] The structural changes for things like, muting a node or disabling/bypassing a node or tapping into a node along a chain could be done with graph rerouting, but if there's an easier way we can add it to a ChromeOnly API, that'd be far preferable from my end.

[4] https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/webaudio.js

Comment 19

[(no longer active)]

Thanks for the explanation, I think I understand your setup better now.

I think one big issue with this setup is that things are tied to the JS wrapper objects we return to Web content.  Because the graph can outlive those JS wrappers, that is the wrong place to hook things up I think.

So how about this idea: Let's try to maintain a shadow graph on the client side, and just send out appropriate notifications from Gecko when it creates AudioContexts/AudioNodes, etc.  I think a setup like this would work:

* Give each AudioContext per window one monotonically increasing integer ID.
* Give each AudioNode a unique ID which is a pair of (AudioContext ID, monotonically increasing integer ID per AudioContext)
* Every time we create an AudioContext, or an AudioNode, or connect/disconnect AudioNodes, we send out notifications from inside Gecko which you can listen and act on.
* Then in your code, you won't need to instrument the AudioContext ctor and its node creating functions any more, you'll just listen to these notifications and build your graph on top of them.
* We can add an API to query information specific to each node which you'd like to know in the tool.
* We'll send out a notification when each node is killed, so that you can drop it from the graph in "real time".

Does this sound good?

(Note that none of these notifications exist today, mostly because we haven't needed them yet.)

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

That sounds perfect! The audio actor pretty much just does notifications like that, and all those would work as the actor could listen to those events from the AudioObserver (what you described), and then emit to the front end dev tools when needed. If we can also query information, like 'give me the value of this property' on a node, that's needed too.

The only thing I think is missing, and not sure if possible, are modification requests, like changing the gain on a node -- would it be possible to also have those requests? Or maybe a request that returns the wrapper node, and the actor can handle the modification? That way the wrapper objects aren't stored weakly in the actor and only used temporarily:

function setProperty (id, prop, value) {
  let node = AudioObserver.getNode(id)
  node[prop].value = value; // If an AudioParam
}

If that's not possible, maybe just a way to set properties via this observer. Down the road, would love other abilities like mentioned in comment 18, and maybe this is the road to that, but the main thing right now in terms of modifications are just simple property changes.

I haven't worked with any C++ in Gecko, and am a bit rusty on that, but would love to help and could probably do some stuff once the 'base' is in there all hooked up. Shooting to have a publically announcable version of the tool for Firefox 32, and if we don't have the GC deletion notifications by then (would be very ambitious!) that'll totally be okay. Do you want to make a bug for this or do you want to use this one?

Comment 21

[(no longer active)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #20)
> That sounds perfect! The audio actor pretty much just does notifications
> like that, and all those would work as the actor could listen to those
> events from the AudioObserver (what you described), and then emit to the
> front end dev tools when needed. If we can also query information, like
> 'give me the value of this property' on a node, that's needed too.
> 
> The only thing I think is missing, and not sure if possible, are
> modification requests, like changing the gain on a node -- would it be
> possible to also have those requests? Or maybe a request that returns the
> wrapper node, and the actor can handle the modification? That way the
> wrapper objects aren't stored weakly in the actor and only used temporarily:

We can expose whatever information you need, but not based on the JS wrapper (as that messes with the lifetime of things.)  We can give you the ID of the node or whatever and you can figure out which node it's related to in your code.  (Basically the IDs I talked about earlier are used instead of references to the JS wrappers themselves, in order to handle the case where the wrapper object might be dead.

> function setProperty (id, prop, value) {
>   let node = AudioObserver.getNode(id)
>   node[prop].value = value; // If an AudioParam
> }
> 
> If that's not possible, maybe just a way to set properties via this
> observer. Down the road, would love other abilities like mentioned in
> comment 18, and maybe this is the road to that, but the main thing right now
> in terms of modifications are just simple property changes.

Can you please give a detailed list of everything you need so that we know exactly what to do here?

> I haven't worked with any C++ in Gecko, and am a bit rusty on that, but
> would love to help and could probably do some stuff once the 'base' is in
> there all hooked up. Shooting to have a publically announcable version of
> the tool for Firefox 32, and if we don't have the GC deletion notifications
> by then (would be very ambitious!) that'll totally be okay. Do you want to
> make a bug for this or do you want to use this one?

Hmm, we can morph this bug, or you can file another one.  I don't care where the work happens as long as it does.  :-)

FWIW implementing the C++ side of things is not difficult at all, it's just work.  If Paul or someone else doesn't have the time to write the patch, I'd be happy to help out, but we'll need this list first.  :-)

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

Let's use this bug -- feel free to rename, as I'm not sure of how to describe the work necessary.
As of now, the tool only works for one AudioContext. With this enhancement, it looks possible to be able to use two (although won't be supported for now, but useful to have the info there, nonetheless). Some below are marked with (v2) for things that would be nice to have now, but the tool does not yet support it, so can be done later than the others.

Will also have to keep in mind how this is initiated -- right now, it listens for a specific tab's DOM events to instrument, so not sure if each instance of the server tools needs to keep track of the current window, with the notification passing in what window it's referring to:

AudioObserver.on('context-create', (id, window) => {
  if (this.window === window)
    this.startTools();
});

Notifications needed:
* AudioContext created
* AudioNode created
* AudioNode connected to another AudioNode
* AudioNode disconnected
* AudioNode destroyed/GC'd
* AudioNode connected to an AudioParam (v2) (would we need to tag the AudioParams with an ID as well?)
* AudioNode property change, maybe just AudioParam change (v2)
* AudioParam automation triggered -- another nice to have (v2), but anytime a method is called on an AudioParam, with what kind of automation is called, and the values used (ultimately will have an automation graph view)

Commands:
* getContextProperties(ID) - properties of the AudioContext
* getNodeProperties(ID) - returns key-value pair of it's properties. Would have to be deep for a node's audio params... gain for example: `{ gain: { value: 0.5 } }`. Could be trickier for complex types.
* getNodeType(ID) - Return what kind of AudioNode the ID is related to, like "OscillatorNode", "GainNode", etc.
* setNodeProperty(ID, prop, value) - this would have to support both properties on the node, as well as an AudioParam -- so that `setNodeProperty(ID, 'gain', 1)` would actually set `node.gain.value = 1` on the node if `node[prop] instanceof AudioParam`, or something like that.
* start(ID, time)/stop(ID, time) - Start/stop AudioSourceNodes (v2)

For the getter/setters, I'm not sure what information can be passed across these channels for non-primitives, like an AudioBuffer for example, or TypedArrays. I can't think of any valuable non-primitives used in this case other than AudioBuffer, where it can be referenced (played back) outside of the current AudioContext for debugging, and loading up your own (with file select GUI) for game developers, things like that. If something like that's possible, great, but probably (v2) as well :)

Comment 23

[Paul Adenot (:padenot)]

Ehsan answered everything, I guess. I can certainly write some code, when the C++ needs here are cleared up.

Comment 24

[Jordan Santell [:jsantell] [@jsantell]]

For designing the audio observer (or whatever it'll be called), here are some features collected up of various implementation difficulty as well as usefulness, if it influences design at all: https://gist.github.com/jsantell/ee1be41bc87968d7b170

Comment 25

[(no longer active)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #22)
> Let's use this bug -- feel free to rename, as I'm not sure of how to
> describe the work necessary.
> As of now, the tool only works for one AudioContext. With this enhancement,
> it looks possible to be able to use two (although won't be supported for
> now, but useful to have the info there, nonetheless).

Yes, sounds good.

> Some below are marked
> with (v2) for things that would be nice to have now, but the tool does not
> yet support it, so can be done later than the others.
> 
> Will also have to keep in mind how this is initiated -- right now, it
> listens for a specific tab's DOM events to instrument, so not sure if each
> instance of the server tools needs to keep track of the current window, with
> the notification passing in what window it's referring to:
> 
> AudioObserver.on('context-create', (id, window) => {
>   if (this.window === window)
>     this.startTools();
> });

I was thinking about an nsIObserverService notifications which sets aSubject to the nsIDOMWindow corresponding to the originating window.  Then you should be able to create the above JS abstraction on top of it if needed.  Sounds good?

> Notifications needed:
> * AudioContext created
> * AudioNode created
> * AudioNode connected to another AudioNode
> * AudioNode disconnected
> * AudioNode destroyed/GC'd

Note to the implementer: AudioNode::DestroyMediaStream, I think.

> * AudioNode connected to an AudioParam (v2) (would we need to tag the
> AudioParams with an ID as well?)

Yes.

> * AudioNode property change, maybe just AudioParam change (v2)

You need to specify which properties for which nodes.  ;-)  But please file a new bug for that anyway.

> * AudioParam automation triggered -- another nice to have (v2), but anytime
> a method is called on an AudioParam, with what kind of automation is called,
> and the values used (ultimately will have an automation graph view)

This is trickier since the automation gets "triggered" on the background thread.  Can you please move this part to a new bug and specify what you want to build on top of it?

(I think the rest of the above looks fine as a starting point, so let's do those parts here.  Everything else needs to go in its own bug.)

> Commands:
> * getContextProperties(ID) - properties of the AudioContext
> * getNodeProperties(ID) - returns key-value pair of it's properties. Would
> have to be deep for a node's audio params... gain for example: `{ gain: {
> value: 0.5 } }`. Could be trickier for complex types.

These sound good, but you need to be more precise about which properties for which nodes and which properties for AudioContext (follow-up bug please.)

> * getNodeType(ID) - Return what kind of AudioNode the ID is related to, like
> "OscillatorNode", "GainNode", etc.

Hmm.  I was kind of expecting this info to be encoded in the observer notification when they're created, but returning them again sounds fine too.

> * setNodeProperty(ID, prop, value) - this would have to support both
> properties on the node, as well as an AudioParam -- so that
> `setNodeProperty(ID, 'gain', 1)` would actually set `node.gain.value = 1` on
> the node if `node[prop] instanceof AudioParam`, or something like that.
> * start(ID, time)/stop(ID, time) - Start/stop AudioSourceNodes (v2)

Again, there is a good chunk of detail missing here.  Note that things such as AudioParam are *extremely* complicated little packages so "setting a param on AudioParam" for example doesn't mean anything since you might be talking about automation values, curves, and whatnot.

Also, I'm slightly interested in not ending up inventing a shadow Web Audio API here.  ;-)  So I would really like you to think about the kinds of things you can actually build on top of these APIs before we add them.  For example, building a good AudioParam editing tool can be a good amount of work, do we have plans to do that?

> For the getter/setters, I'm not sure what information can be passed across
> these channels for non-primitives, like an AudioBuffer for example, or
> TypedArrays. I can't think of any valuable non-primitives used in this case
> other than AudioBuffer, where it can be referenced (played back) outside of
> the current AudioContext for debugging, and loading up your own (with file
> select GUI) for game developers, things like that. If something like that's
> possible, great, but probably (v2) as well :)

Actually since AudioBuffer is not goverened by similar weird rules as AudioNodes, you should be fine with the existing shim approach to get your hands on the AudioBuffer objects.  Would that be good enough?

Comment 26

[Jordan Santell [:jsantell] [@jsantell]]

Going to sketch out some APIs based on your comments. I didn't want to get into too much detail, just core of what was needed, but you're right, this should be more explicit. And yes, avoiding building a new shadow API wouldn't be good :) Will follow up with some example code.

Comment 27

[(no longer active)]

Sounds great!

Paul, do you have cycles to work on this?  I think we're ready to start the implementation for at least the first parts of comment 25.  CCing Maire to keep her in the loop for staffing reasons.

Comment 28

[Jordan Santell [:jsantell] [@jsantell]]

Hacking together a consumer of this interface[1], ran into some issues and thoughts.

* Observer Notifications[2] require `data` to be a string. If we have an `nsIDOMWindow` as the subject, could we have a JSON.parse'able string as the data? I've never seen it in my (limited) time working with observers, so not sure if there's an idiom for returning more than one point of data, like connecting an audio node (need the context ID, source node ID, target audioparam/node ID, output index, input index). Maybe there's a better way for this.

* The most simple use case for being able to use this currently in our tools are the first batch of notifications listed (not the v2 ones) in comment #22, as well as the get/set AudioNode properties, which can be in a different bug. How would the setting of a value work? Send an observer notification to set property `x` to value `y` on node `z`, and/or set AudioParam `x`'s value property to `y`, and need to know which node the param belongs to? How will we know if and when that setting was successful, unless we also had an 'onchange' notification as well?

* The more I list out, the more it seems like a notification-based shadow API. The advantage to the shim/wrapper approach is having direct access to the objects themselves to perform any action on them, so maybe we can have a generalized approach on notifications on object creation (Context, Node, Param), and a generalized property setter (as only primitives can be set for now), so setting prop x to value y on object z, which will work for any of the Audio objects (context, node, param). If we have creation notification and basic get/set abilities, then lots of things become possible just from flexibility on the front end without additional back end work, outside of specific events like connect/disconnect, and down the road, things like automation being scheduled (like param.exponentialRampToValueAtTime). Almost all of the relevant properties are shallow from it's parent (an Oscillator's `type` value, or an AudioParam's `value` value), so it may just be a simple setter notifier. Still, would need to know if and when a setting was successful.

* Note on the AudioBuffer way of still being shimmed -- if we do that, I wonder how we'd be able to associate the shimmed AudioBuffer with which AudioBufferSourceNode it's associated with. This is another future/down the road feature, however.

[1] https://gist.github.com/jsantell/7d526a051aa03e7ed8c7
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserver

Comment 29

[Paul Adenot (:padenot)]

Ehsan, my first priority is to bring bug 848954 to completion (it benefits WebRTC, Web Audio API, and is _really_ needed). Then, I think I might be able to help, but maybe Maire has something else in mind?

Comment 30

[(no longer active)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #28)
> Hacking together a consumer of this interface[1], ran into some issues and
> thoughts.
> 
> * Observer Notifications[2] require `data` to be a string. If we have an
> `nsIDOMWindow` as the subject, could we have a JSON.parse'able string as the
> data? I've never seen it in my (limited) time working with observers, so not
> sure if there's an idiom for returning more than one point of data, like
> connecting an audio node (need the context ID, source node ID, target
> audioparam/node ID, output index, input index). Maybe there's a better way
> for this.

Hmm, yeah, you're right...  Having to parse stuff out of the data string is a pain.  Constructing JSON strings in C++ is also a pain.  Perhaps we can use some WebIDL dictionaries so that we can construct the data in a structured way easily in C++ and then you can consume it as a normal object in JS.  That would require us to invent a new-ish API though.  Here's what I'm thinking right now:

dictionary AudioNodeInfo {
  long id;
  DOMString type;
  // More stuff can go here
};
callback NewNodeCreated = void(AudioNodeInfo);
partial interface AudioContext {
  [ChromeOnly]
  NewNodeCreated nodeCreatedCallback;
};

This way, in JS you'd intercept the AudioContext's ctor as you would (and it's fine to hold on to its JS wrapper since the object stays alive as long as the window would anyways) and do something like this:

function AudioContextShim() {
  var realCtx = createRealAudioContext();
  realCtx.nodeCreatedCallback = function(nodeInfo) {
    alert(nodeInfo.id); // numerical ID for the node
    alert(nodeInfo.type); // For example, "GainNode"
  };
  return realCtx;
}

How does that sound?

> * The most simple use case for being able to use this currently in our tools
> are the first batch of notifications listed (not the v2 ones) in comment
> #22, as well as the get/set AudioNode properties, which can be in a
> different bug. How would the setting of a value work?

I doubt that we can figure out a generic interface for setting node properties which is useful enough.  What properties/params do you actually need to set?

> Send an observer
> notification to set property `x` to value `y` on node `z`, and/or set
> AudioParam `x`'s value property to `y`, and need to know which node the
> param belongs to?

With the idea of the above API, you would do something like:

  realCtx.setGainValue(3 /* ID of the node */, 0.5 /* Gain value */);

The only sucky part of this solution is going to be the fact that we'd have to track the GainNode internally and map it to the ID.  And we're going to have to hold on to a weak reference to a node, so if the node has been deleted (which you'll get a notification about) we'll need to throw here.

> How will we know if and when that setting was successful,
> unless we also had an 'onchange' notification as well?

The modifications will be instant, no need for an onchange notification.

> * The more I list out, the more it seems like a notification-based shadow
> API. The advantage to the shim/wrapper approach is having direct access to
> the objects themselves to perform any action on them, so maybe we can have a
> generalized approach on notifications on object creation (Context, Node,
> Param), and a generalized property setter (as only primitives can be set for
> now), so setting prop x to value y on object z, which will work for any of
> the Audio objects (context, node, param). If we have creation notification
> and basic get/set abilities, then lots of things become possible just from
> flexibility on the front end without additional back end work, outside of
> specific events like connect/disconnect, and down the road, things like
> automation being scheduled (like param.exponentialRampToValueAtTime). Almost
> all of the relevant properties are shallow from it's parent (an Oscillator's
> `type` value, or an AudioParam's `value` value), so it may just be a simple
> setter notifier. Still, would need to know if and when a setting was
> successful.

The issue is that we can't actually give you a clean shadow API even if we wanted to, since that would reintroduce the problem of the JS wrappers keeping the nodes alive and thus modifying the behavior of the program.  So, the inspection API has to avoid giving you real objects back, and it will need to imitate referenced to those objects through these made-up IDs.  Now, maintaining the internal information to map those IDs to the objects costs both performance and memory, so I'm *very* hesitant to try to give you a generic and fully flexible API here since that would be a lot of work to implement and it won't be efficient.

Which is why I keep asking, what *actual* things do you need to do with the API?  I really suggest to stop thinking in terms of a magical shadow API which gives you the exact same functionality of Web Audio, that's not the right direction to move towards.

> * Note on the AudioBuffer way of still being shimmed -- if we do that, I
> wonder how we'd be able to associate the shimmed AudioBuffer with which
> AudioBufferSourceNode it's associated with. This is another future/down the
> road feature, however.

Again, why do you need to do that?  And what would you be exactly trying to achieve?

Perhaps we should take a step back, and get a list of use cases which we agree on and try to build something that satisfies those use cases (and leave future extension points open where they don't incur any costs) instead of trying to build an excessively powerful API and then see what interesting problems we can solve with it.  :-)

Comment 31

[(no longer active)]

(In reply to Paul Adenot (:padenot) from comment #29)
> Ehsan, my first priority is to bring bug 848954 to completion (it benefits
> WebRTC, Web Audio API, and is _really_ needed). Then, I think I might be
> able to help, but maybe Maire has something else in mind?

Sorry, false alarm, looks like we're still not agreeing over what the API needs to do.  So let's hold off on writing code yet.  (And I do agree that bug 848954 is way more important!)

Comment 32

[Jordan Santell [:jsantell] [@jsantell]]

After meeting with Ehsan today, we summarized the current issues:

* Notification on all events needed to support current and future features in audio tooling would require significant work, and performance overhead, and require lots of back and forth between teams
* The current proxy method isn't flawless, but works, and is constrained to when the audio tool tab is open, and will work for most features

So we think continuing to do the current proxy implementation for most features, and only use notifications when necessary is the best course of action. As this spawned from GC/destruction notification support, we decided that it'd be possible to do that with our current implementation provided we add:

Audio Core:
* Add NativeNodeID to AudioNode (chrome only)
* Add nsISupportsWeakReference interface in AudioNode
* Implement ObserverService for NativeNode destruction, emits NativeNodeID

Dev Tools:
* On Node Creation, store NativeNodeID
* ensure DevTools stores no strong references to AudioNodes (Cu.getWeakReference)
* Hook into the notifcation on NativeNode destruction, using the NativeNodeID as reference to the DOM AudioNode.

Ehsan will prototype the AudioCore features and I will scaffold a test case for ensuring that this solution will work, and we'll make new bugs for those features once confirming the prototype works (we are optimistic), and for future features that aren't possible to implement on the tools side, we can implement notifcations/chrome-only features as needed when they arise, keeping this extra interface minimal as much as possible.

Comment 33

[(no longer active)]

Attachment: Prototype

Jordan, can you please take this for a spin?  This patch:

1) Adds AudioBufferSourceNode.id, a unique, chrome-only integer ID for each AudioBufferSourceNode.
2) Makes AudioBufferSourceNode support nsISupportsWeakReference, so you would hopefully be able to use Components.utils.getWeakReference() on it.
3) Adds a "webaudio-node-demise" observer notification with aData set to a string containing the above integer ID which gets fired as the native AudioNode is about to go away.  Note that this patch will fire that notification for all AudioNode's, so please ignore everything except for AudioBufferSourceNodes that you know the ID of for now in your tests.

Let me know how this works out!

Comment 34

[Jordan Santell [:jsantell] [@jsantell]]

Ehsan, this is amazing, thank you.

Implemented the server side of the tool listening to "webaudio-node-demise", ensuring all references were weakly held to the node, and I was able to get the event firing after a few seconds of unused nodes (creating gain nodes within a for loop, never stored), emitting their IDs. I'll have to test with source nodes and store the IDs and wire up the front end to handle this, but this is very promising.

Comment 35

[Jordan Santell [:jsantell] [@jsantell]]

Having some problems accessing the ID on AudioBufferSourceNodes (it's undefined), but I think it's something with XPCNativeWrappers. It looks as though the GCing of nodes is working, but will have to do several tests for this (the use case where buffer nodes play audio and are discarded afterwards, non-source nodes, make sure nothing gets removed if it shouldn't be). I think this'll work though once I fix the wrapper issues (ChromeOnly props should be accessible to the wrapper)

Comment 36

[(no longer active)]

Cool, let me know once you get to a point where you believe this would be a good strategy.

Comment 37

[Jordan Santell [:jsantell] [@jsantell]]

Getting this now, where I create 10 buffer nodes, but only store the last. I also create an actor for the destination node immediately (so no ID there). Using the Xraywrapper object to actually get the ID, it looks as if I'm getting a new ID, as when objects get GC'd later on, they emit different IDs. Is there any sort of trick that would need to be done for the wrapper objects to share the same ID as their underlying component?

console.log: Creating node with ID:  undefined [object XrayWrapper [object AudioDestinationNode]]
console.log: Creating node with ID:  12 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  13 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  14 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  15 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  16 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  17 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  18 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  19 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  20 [object XrayWrapper [object AudioBufferSourceNode]]
console.log: Creating node with ID:  21 [object XrayWrapper [object AudioBufferSourceNode]]

... few seconds later upon GC

console.log: ON DESTROY NODE 10
console.log: ON DESTROY NODE 9
console.log: ON DESTROY NODE 8
console.log: ON DESTROY NODE 7
console.log: ON DESTROY NODE 6
console.log: ON DESTROY NODE 5
console.log: ON DESTROY NODE 4
console.log: ON DESTROY NODE 3
console.log: ON DESTROY NODE 2
console.log: ON DESTROY NODE 1
console.log: ON DESTROY NODE 0

Comment 38

[(no longer active)]

Do you have a test case that I can run locally (a patch to apply would be great)?

One thing which might be a bit odd in my patch is that I'm internally creating IDs for *all* node types, but have only enabled the ChromeOnly accessor for AudioBufferSourceNode for now.  Also, the "webaudio-node-demise" notification is dispatched for all node types as well, which could explain why you're seeing IDs in "ON DESTROY NODE" which do not correspond to any AudioBufferSourceNode.  Looking at the test case can help me confirm whether this hypothesis is true.

Comment 39

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: Prototype of GCing with Devtools

Comment 40

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: Destroying Nodes Test Case

Comment 41

[Jordan Santell [:jsantell] [@jsantell]]

Here's a rough patch (that includes your platform changes as well) -- it's not hooked into the GUI yet, but the notification console log (ON DESTROY NODE $ID) is listened to on the devtools server, emitted to devtools client, where it's then logged (so what I'm saying is, watch the console rather than the rendered SVG).

The test case I'm using is also attached which creates 10 buffer source nodes, and only stores one. I was seeing notifications for all nodes (in other test cases), but in this case, they're all buffer source nodes except the destination.

Comment 42

[Jordan Santell [:jsantell] [@jsantell]]

As a note, to use the tool, enable it in the devtools options, go to the page, and refresh while on the web audio tab, since it has to listen for the original page load, like the network tab.

Currently, the tool will only work once, and then you have to close and reopen devtools and listen for the load event again, but the patch to fix that should land next week.

Comment 43

[(no longer active)]

As far as I can tell, the _handleCreationCall bits of your patch are wrong, as they still store a strong reference to the node in the AudioNodes array through the call to _onCreateNode.  One thing that we agreed on earlier was to not do this and instead use Components.utils.getWeakReference for all references, right?  I see that you're doing this in AudioNodeActor but not in _handleCreationCall.  Note that this is where I stopped debugging this, there might be more of these, but I can't really understand the devtools side of the code.  Too much JS for my little brain.  :-)  But this is the call stack to where the strong reference is generated which should be helpful to you:

0 anonymous(conn = [object Object], node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":136]
    this = [object Object]
1 constructor(instance = [object Object], arguments = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ehsan/Library/Application%20Support/Firefox/Profiles/f42unlva.pb/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js":145]
    this = [object Object]
2 anonymous(node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":439]
    this = [object Object]
3 anonymous(node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":510]
    this = [object Object]
4 anonymous(functionCall = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":375]
    this = [object Object]
5 anonymous(functionCall = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":334]
    this = [object Object]
6 anonymous(details = [object Window],AudioContext,[object XrayWrapper [object AudioContext]],0,createBufferSource,[object Object],[object Object],[object Object],,[object AudioBufferSourceNode], functionCall = "AudioContext", [object XrayWrapper [object AudioContext]], 0, "createBufferSource", [object Object],[object Object],[object Object], , [object AudioBufferSourceNode]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/call-watcher.js":507]
    this = [object Object]

Anyways, what's happening with the current version of the patch is that the devtools holds these nodes alive for the lifetime of the page, and then if you reload or navigate away, then Gecko kills the nodes and you'll see the console logs some time after the page was navigated away from when the cycle collector runs.  IOW, the platform side of things is working as expected.

Comment 44

[(no longer active)]

Also, note that this experimental patch that I provided is extremely simple and while it has not been well tested of course, I can hardly see any potential bugs in it, so as you're tweaking things on your side, if you cannot get the demise notifications to fire, that's a strong indication that either the test case or the devtools code is holding the nodes alive.  :-)

Comment 45

[(no longer active)]

Also, another very important thing to verify would be that you can use the stored weak reference later on to check to see if the node is alive, and if yes, get a strong ref to it, get some info from the node through the Web Audio API, drop the strong reference and let the weak reference stay alive, and also verify that you can do this over and over on the same node.  Based on my testing of your patch, that is really the only bit of info that I need before I can proceed to write a complete patch for the platform side of things.

Comment 46

[Jordan Santell [:jsantell] [@jsantell]]

The nodes are only stored in a weak map on the server as `_nodeActors`[1], and still being weakly held via WeakMap. I could use the Cu.getWeakReference here as well, but the Weakmap shouldn't prevent collection.

So the test case creates 10 buffer nodes (and the default destination node) -- I *do* get 9 demise notifications, they just don't have the same ID, but what nodes could these refer to? So it looks like the GC is working, I just don't think the IDs are matching up properly (unless these are some underlying audio nodes that are firing). The logging in comment 37 is the same test case, where the demise event is fired, just not matching the ID.

To answer your second question, getting the strong reference from the weak reference seems to work just fine -- I can set/get properties no problem.

What I'm seeing is demise events for all nodes. And now I understand, because I refresh the page before loading the tool, I'm seeing the events from the previous load page maybe, which includes demise notifications for all nodes? But that seems pretty delayed.

Pretty confused right now, so will have to do more digging :)


[1]toolkit/devtools/server/actors/webaudio.js

Comment 47

[(no longer active)]

(In reply to comment #46)
> The nodes are only stored in a weak map on the server as `_nodeActors`[1], and
> still being weakly held via WeakMap. I could use the Cu.getWeakReference here
> as well, but the Weakmap shouldn't prevent collection.

Hmm, I don't know which part of this picture is falling down exactly, but I think we're talking about different parts of the code perhaps?  I'm talking about |AudioNodes| here <http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webaudioeditor/webaudioeditor-controller.js#77> which is definitely a normal JS array which stores strong references.

The stack in comment 43 should show you where this strong reference is being allocated.  Note that the stack is not merely my guess, I set a breakpoint on AudioBufferSourceNode::AddRef which is the function that gets called when you hold a strong reference to this object and captured the JS stack I pasted there.

> So the test case creates 10 buffer nodes (and the default destination node) --
> I *do* get 9 demise notifications, they just don't have the same ID, but what
> nodes could these refer to? So it looks like the GC is working, I just don't
> think the IDs are matching up properly (unless these are some underlying audio
> nodes that are firing). The logging in comment 37 is the same test case, where
> the demise event is fired, just not matching the ID.

I did verify that the IDs are correct.  It's just that it's possible for a node created in page A to be destroyed after page A has been unloaded and perhaps even replaced by page B.  Which means that a short amount of time after the first time that you refresh the page to get it hooked up to the devtools panel, you'll see demise notifications from the _previous_ page (with IDs less than the one that you've seen in the active page.  And because of the issue I mentioned in comment 43, you won't see any notifications for the active page, which is probably what is confusing you.

This is something that your code needs to prepare for, and it's actually quite simple to do so, just ignore the demise notifications for any nodes with IDs that you don't know of.

The reason you're seeing 9 demise notification is that the test case is holding the 9th created node alive for the lifetime of the page.

> To answer your second question, getting the strong reference from the weak
> reference seems to work just fine -- I can set/get properties no problem.

Awesome!

> What I'm seeing is demise events for all nodes.

Yes, I mentioned that in comment 38.  Sorry if I didn't make it clear enough.

> And now I understand, because I
> refresh the page before loading the tool, I'm seeing the events from the
> previous load page maybe, which includes demise notifications for all nodes?

*Exactly*!

> But that seems pretty delayed.

Oh, yes.  Note that these objects are deleted by the cycle collector at its leisure, which may mean that they might easily survive several JS GCs depending the heuristics of the cycle collector which you should not rely on.  So there will always be some delay involved, but that's OK, since the delay is actually how long these objects are staying alive.  I believe this is a detail that we should not hide in the devtools since the number of nodes alive at any given point in time can sometimes give some clue to them why the browser is behaving poorly or why they're getting audio glitches etc.

> Pretty confused right now, so will have to do more digging :)

Sorry.  :-)  I hope the explanation above helps clarify things.  This stuff is second nature to me so sometimes I may fail to explain things properly, in which case, *please* ask questions.  Soon it will all make sense to you, at least somewhat!

Comment 48

[(no longer active)]

You know what?  Let me write you a more complete patch...  I think we can go ahead with this!

Comment 49

[Jordan Santell [:jsantell] [@jsantell]]

Yes, it's very promising, I think this'll all work once a few kinks are ironed out!

The strong reference in webaudioeditor-controller.js is for the AudioNodeActor, which is the wrapper used to communicate async between server/client ends. The AudioNodeActor (defined in toolkit/devtools/server/actors/webaudio.js) holds the weak reference (`this.node`) in itself, so shouldn't be a strong reference, but it looks like something is holding onto it somewhere, so I'll check that out more. As a quick reference, anything in browser/* is 'front end' and the stuff in toolkit/* is the back 'end' -- the front end only deals with these actor representations and notifications from the back end.

And sounds great! I'm working on the devtools code that relies on this in bug 1008497, so once this is landed, all the pressure will be on me :)

Comment 50

[Jordan Santell [:jsantell] [@jsantell]]

Ooog, the _nodeActors.set(node, actor) isn't using the weak reference, so that'd be it ;)

Comment 51

[Jordan Santell [:jsantell] [@jsantell]]

But nm, that is the WeakMap, which I think is better to use for storing it, than the Reference object, as that'd last.. would AudioNodes have any problem being stored as the key in a WeakMap? Would that prevent collection somehow? Specifically the Xraywrapped version.

Comment 52

[Jordan Santell [:jsantell] [@jsantell]]

Yeah, looking at your stack, it looks like the WeakMap is holding onto the AudioNode, which it shouldn't..

Comment 53

[(no longer active)]

I don't know why WeakMap is having this behavior, and what it's behavior should be, FWIW.  My suggestion is to file a bug in Core::DOM about that, and use the getWeakReference() trick instead.

Comment 54

[(no longer active)]

Comment on attachment 8428144 [details] [diff] [review]
Prototype

OK, I filed bug 1015783 for the platform side changes.  Please look at the commit message of my patch there for a description of what it does.  You can use this patch instead of the one I'm marking as obsolete in order to build your stuff on top of before it lands.

Comment 55

[Jordan Santell [:jsantell] [@jsantell]]

Great, I'll use the patch in bug 1015783! Doing a simple setInterval tick creating AudioNodes and storing them in a WeakMap didn't seem to create a spike in memory, so maybe there's something else holding on to it other than the WeakMap, I find it more likely I'm doing something wrong than the WeakMap implementation, but will check this more later after switching over to the Cu.getWeakReference.

Comment 56

[(no longer active)]

(In reply to comment #55)
> Great, I'll use the patch in bug 1015783! Doing a simple setInterval tick
> creating AudioNodes and storing them in a WeakMap didn't seem to create a spike
> in memory, so maybe there's something else holding on to it other than the
> WeakMap, I find it more likely I'm doing something wrong than the WeakMap
> implementation, but will check this more later after switching over to the
> Cu.getWeakReference.

OK, sounds good.  Good luck!

Comment 57

[Jordan Santell [:jsantell] [@jsantell]]

Even after not using the WeakMap, still not getting the desired demise notification until document reload, I think something might be up. Unless you're able to test and see that you get these notifications outside of document wipe (which should work, but not sure if you have tests for that), I think best thing to do would be to hold off on more work for bug 1015783, until I merge in some more outstanding patches waiting for review and start the devtools side from scratch again to see what's preventing this event.

Comment 58

[(no longer active)]

Can you please rebase what you have on top of bug 1015783?  I'd be happy to test to see what's causing the strong reference.  (Or if you're familiar with gdb, it's a matter of breaking on mozilla::dom::AudioBufferSourceNode::AddRef and issuing the js command in gdb.)

Comment 59

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

mccr8, any feedback to https://bugzilla.mozilla.org/show_bug.cgi?id=980506&mark=49-53#c49 ?

Comment 60

[Andrew McCreight [:mccr8]]

> so maybe there's something else holding on to it other than the WeakMap, I find it more likely I'm doing something wrong than the WeakMap implementation

WeakMap have a really weird implementation, so if you see some persistent odd behavior please do file a bug and CC me.  Usually, though, the bugs in WeakMaps take the form of their being too weak rather than too strong.

Comment 61

[Jordan Santell [:jsantell] [@jsantell]]

I'll look into it more, but I need to make a simple test case to see what is actually holding strong references -- after removing the WeakMap and working around it, I still wasn't able to get collections, so I think it may be some other implementation in the dev tools. Will let you know if I discover any bugs with AudioNodes and WeakMaps though!

Comment 62

[Jordan Santell [:jsantell] [@jsantell]]

Ehsan, I'll check the references again sometime this week, on a fresh patch and a leaner use case. Thanks!

Comment 63

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-test.patch

Comment 64

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-test.patch

Comment 65

[Jordan Santell [:jsantell] [@jsantell]]

After much debugging, Ehsan and I found that `call-watcher` is storing a strong reference, tracked in bug 1019964.

This bug will also have to remove WeakMaps, as it seems that storing a weak reference via Cu.getWeakReference within a WeakMap creates a strong reference(?)
Also, side issue to remove WeakMaps from implementation, because issue WeakMap/Cu.getWeakReference where it holds a strong reference, I believe this can be fixed by removing WeakMaps since we have IDs:

0 anonymous(conn = [object Object], node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":133]
    this = [object Object]
1 constructor(instance = [object Object], arguments = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ehsan/Library/Application%20Support/Firefox/Profiles/f42unlva.pb/extensions/[e-mail]!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/core/heritage.js":145]
    this = [object Object]
2 anonymous(node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":441]
    this = [object Object]
3 anonymous(node = [object XrayWrapper [object AudioBufferSourceNode]]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":527]
    this = [object Object]
4 anonymous(functionCall = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":376]
    this = [object Object]
5 anonymous(functionCall = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webaudio.js":339]
    this = [object Object]
6 anonymous(details = [object Window],AudioContext,[object XrayWrapper [object AudioContext]],0,createBufferSource,[object Object],[object Object],[object Object],,[object AudioBufferSourceNode], functionCall = "AudioContext", [object XrayWrapper [object AudioContext]], 0, "createBufferSource", [object Object],[object Object],[object Object], , [object AudioBufferSourceNode]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/call-watcher.js":507]
    this = [object Object]

Comment 66

[(no longer active)]

Can you please file a different bug for the WeakMap issue in comment 65 and CC :mccr8 on it?  Thanks!

Comment 67

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-event-audionodes.patch

Still getting local intermittent process crashes, but wanted to run this by you, Victor, as it's pretty large. The call-watcher weak ref patch (bug 1019964) is included in here and will be pulled out. This is only for the server-side notifications that a node has been destroyed -- front end work is in bug 994263

Comment 68

[Jordan Santell [:jsantell] [@jsantell]]

Also, you can see if exposing the ContentObserver's events to the CallWatcherActor is useful or not (bug 1021321), as the CO is being access from the CWA, which I'd rather not do as it's a "private" property.

Comment 69

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8437138 [details] [diff] [review]
980506-gc-event-audionodes.patch

Removing r? for now, moving most of the weak reference work to bug 1022917

Comment 70

[Jordan Santell [:jsantell] [@jsantell]]

https://tbpl.mozilla.org/?tree=Try&rev=2ddcdb7d0f62

Comment 71

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-event-audionodes.patch

Comment 72

[Victor Porof [:vporof][:vp]]

Comment on attachment 8441626 [details] [diff] [review]
980506-gc-event-audionodes.patch

Review of attachment 8441626 [details] [diff] [review]:
-----------------------------------------------------------------

Loving all these tests.

::: browser/devtools/webaudioeditor/test/doc_destroy-nodes.html
@@ +12,5 @@
> +
> +    <script type="text/javascript;version=1.8">
> +      "use strict";
> +	  (function () {
> +      let ctx = new AudioContext();

This is some funky ass indentation.

::: toolkit/devtools/server/actors/call-watcher.js
@@ -80,5 @@
>  
>      // Store a weak reference to all objects so we don't
>      // prevent natural GC if `holdWeak` was passed into
>      // setup as truthy. Used in the Web Audio Editor.
> -    if (this._holdWeak) {

Ooops.

Comment 73

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-gc-event-audionodes.patch

Ah, fixed the weird indentation in the html. My editor loves throwing tabs in html.

Comment 74

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/332504a9d638

Comment 75

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/3a4d57044461 for devtools bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=42090964&tree=Fx-Team

Maybe some conflict with Bug 1026766?

Comment 76

[Jordan Santell [:jsantell] [@jsantell]]

Looks like it.. looks like the `head.js` changes didn't get applied..

Comment 77

[Jordan Santell [:jsantell] [@jsantell]]

Once the git repo is updated I'll rebase these changes and resubmit

Comment 78

[Jordan Santell [:jsantell] [@jsantell]]

Got that sorted, repushing to try after rebasing and fixing up: https://tbpl.mozilla.org/?tree=Try&rev=3c5d705fb3c2

Comment 79

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 980506-wae-destroy-notification.patch

Comment 80

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/fx-team/rev/d3415bb2b811

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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