Closed Bug 1022917 Opened 8 years ago Closed 8 years ago

Web Audio Editor should hold only weak refs

Categories

(DevTools Graveyard :: Web Audio Editor, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
No longer blocks: 1019964
Summary: Use ChromeOnly IDs instead of WeakMap for tracking AudioNodes in Web Audio Editor → Web Audio Editor should hold only weak refs
Attached patch 1022917-wae-weak-refs-only.patch (obsolete) — Splinter Review
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
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8437284 - Flags: review?(vporof)
Try passing
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?
Attachment #8437284 - Flags: review?(vporof)
Attached patch 1022917-wae-weak-refs-only.patch (obsolete) — Splinter Review
* 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
Attachment #8437284 - Attachment is obsolete: true
Attachment #8438543 - Flags: review?(vporof)
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'
Attachment #8438543 - Flags: review?(vporof) → review+
Rock!
Attachment #8438543 - Attachment is obsolete: true
Attachment #8438602 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/254a1f0dc641
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.