1022917 - Web Audio Editor should hold only weak refs

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

Once call-watcher no longer stores strong references (bug 1019964), there are no strong refs to AudioNodes stored in the weak map of the web audio editor. We need to store a `Set` of the `id` propeties available to Chrome on the AudioNodes themselves added in bug 1015783. 

Using a set with these ChromeOnly ids rather than a WeakMap will still allow connections to be made between the content AudioNodes and their corresponding actors, and allow the GC tracking to continue.

Comment 1

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

Attachment: 1022917-wae-weak-refs-only.patch

This uses the ChromeOnly IDs on audio nodes for tracking, and ensures this tool holds no strong references. The CallWatcherActor releasing strong refs is in bug 1019964, and requires this patch to land

Comment 2

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

 https://tbpl.mozilla.org/?tree=Try&rev=52430cac2339

Comment 3

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

Try passing

Comment 4

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

Comment on attachment 8437284 [details] [diff] [review]
1022917-wae-weak-refs-only.patch

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

There's a few things I don't understand, like the wrap/unwrap change and this.conn = conn which looks like it's supposed to be in a different bug. Ask me again for review after addressing the comments below.

::: toolkit/devtools/server/actors/webaudio.js
@@ +167,5 @@
>     * @param String value
>     *        Value to change AudioParam to.
>     */
>    setParam: method(function (param, value) {
> +    let node = this.node.get();

Might be good to check for null, here and everywhere else.

@@ +192,5 @@
>     * @param String param
>     *        Name of the AudioParam to fetch.
>     */
>    getParam: method(function (param) {
> +    let node = this.node.get();

Especially here.

@@ +267,5 @@
>    typeName: "webaudio",
>    initialize: function(conn, tabActor) {
>      protocol.Actor.prototype.initialize.call(this, conn);
>      this.tabActor = tabActor;
> +    this.conn = conn;

This should be in a different patch in a different bug.

@@ +304,5 @@
>  
> +    // Store ChromeOnly ID (`nativeID` property on AudioNodeActor) mapped
> +    // to the associated actorID, so we don't have to expose `nativeID`
> +    // to the client in any way.
> +    this._nativeToActorID = new Map();

Why not instantiate this in the `initialize` function, to skip the if (this._nativeToActorID) check above?

@@ +338,5 @@
>  
>    _handleRoutingCall: function(functionCall) {
>      let { caller, args, window, name } = functionCall.details;
> +    let source = wrap(caller);
> +    let dest = wrap(args[0]);

Didn't we have a different bug for better handling this wrap/unwrap stuff? Is it still necessary now that we have weak references to those?

@@ +370,3 @@
>        this._firstNodeCreated = true;
>      }
> +    this._onCreateNode(wrap(result));

There's a lot of wrapping going on. Can you please explain why?

@@ +380,5 @@
>    finalize: method(function() {
>      if (!this._initialized) {
>        return;
>      }
> +    this.conn = null;

Different bug.

@@ +448,5 @@
> +    // return via notification.
> +    nativeID = ~~nativeID;
> +
> +    // Ensure that this doesn't attempt to fetch actors after
> +    // clean up.

When can this happen?

Comment 5

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

Attachment: 1022917-wae-weak-refs-only.patch

* Added checking when getting an AudioNode from a weak ref -- this will have to be tested when we have more events for GCing in bug 980506
* Removed all wraps/unwraps except one that's necessary for getting ChromeOnly properties, as WeakMaps are no longer used. Allowing CallWatcherActor to natively expose wrapped objects will remove the need for this, in bug 1015665
* Removed this.conn setting; redundant with the protocol.js, as this does it anyway.
* Removed safety check that the map of node ids is available, unneeded at this point.
* Moved node id Map to instantiation rather than setup

Comment 6

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

Comment on attachment 8438543 [details] [diff] [review]
1022917-wae-weak-refs-only.patch

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

Much better!

::: toolkit/devtools/server/actors/webaudio.js
@@ +453,5 @@
>     */
> +  _getActorByNativeID: function (nativeID) {
> +    // Ensure we have a Number, rather than a string
> +    // return via notification.
> +    nativeID = ~~nativeID;

Nit: could just use +nativeID for coercion, instead of ~~.

@@ +563,5 @@
> + * attempt to access an AudioNode that has been GC'd.
> + *
> + * @return Object
> + */
> +function CollectedAudioNodeError () {

We definitely need a CollectedAudioNodeExceptionFactoryBean.
Kiddin'

Comment 7

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

Attachment: 1022917-wae-weak-refs-only.patch

Rock!

Comment 8

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

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

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/254a1f0dc641