1019964 - call-watcher should not store strong references

Status: RESOLVED FIXED

(DevTools Graveyard :: Web Audio Editor, defect)

Description

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

* call-watcher needs to not store strong reference

(gdb) js
0 anonymous() ["resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/call-watcher.js":387]
    this = [object XrayWrapper [object AudioContext]]
1 <TOP LEVEL> ["http://people.mozilla.org/~eakhgari/webaudio/gain.html":25]
    this = [object Window]

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 1

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

Attachment: 1019964-call-watcher-weak-ref.patch

This allows natural GC to occur in webaudio actor, and tests pass for canvas/webgl tools. I still need to fix all of the GC event stuff for web audio tool (bug 980506), but wanted to get this to you to check out.

Comment 2

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

Note, I noticed some callwatcher tests in the canvasdebugger, maybe we should label them as such (rather than canvasdebugger, but maybe still live there), so we can have tests for these shared actors somewhere

Comment 3

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

Comment on attachment 8435236 [details] [diff] [review]
1019964-call-watcher-weak-ref.patch

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

Did you do any manual testing? I'm doing some myself but it's always good to have a few more eyes on these sorts of changes.

::: toolkit/devtools/server/actors/call-watcher.js
@@ +84,5 @@
>        result: result
> +    });
> +
> +    Object.defineProperty(this, "details", {
> +      get: () => weakDetails.get() || {}

Why the need for || {} ?

Comment 4

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

This allowed the audio tools server side AudioNodes to become collected (in bug 980506) -- my only worry is that some things in canvas/audio tools that rely on the call-watcher holding a strong reference, and that we'll see weird results because of it. If that's the case, the canvas/audio tools should be the ones holding the strong reference, not the call-watcher. I'd like to land this after the uplift in a day or two, so we'll have 6 weeks to do more manual testing if the above is the case in any scenario.

The `|| {}` just ensures we don't get null and then accessing properties of `details`, like `details.result` wouldn't get an accessing property on null error.

Maybe the object that is being held by a weak reference should just be a normal object, with certain properties (caller, args, results, anything that could be an object), just being weak references.

let weakResult = Cu.getWeakReference(result);
let details = {
  type: "SomeType",
  name: "MethodName",
  ...
}

Object.defineProperty(details, "result", { get: () => weakResult.get() });

Actually, I like that much more, will update patch.

Comment 5

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

Attachment: 1019964-call-watcher-weak-ref.patch

Updated with more obvious code, and doesn't aggresively use weak references for objects that are unnecessary.

Comment 6

[(no longer active)]

I have one abstract suggestion which I hope is helpful here.  I think it might make sense to use the usual abstractions in the devtools code base a bit more cautiously when dealing with audio nodes.  In my experience, it's too easy to introduce a string reference in large layers of JS abstractions, and this will be something which is going to be hard to test going forward, so perhaps it would make sense a bit to try to use the existing abstractions while thinking about what happens in the future when the assumptions that are true today will no longer be true...

Comment 7

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

Ehsan, I agree when dealing with GC and AudioNodes we should minimize clever/terse abstractions and err on the side of obvious solutions. Although tests in bug 980506 should illuminate any failures occurring (like something holding a strong reference to any audionode). Is this what you mean?

Comment 8

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

With bug 1022917 merged, pushing this to try:
https://tbpl.mozilla.org/?tree=Try&rev=c355749413b4

Comment 9

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

Attachment: 1019964-call-watcher-weak-ref.patch

Comment 10

[(no longer active)]

(In reply to comment #7)
> Ehsan, I agree when dealing with GC and AudioNodes we should minimize
> clever/terse abstractions and err on the side of obvious solutions. Although
> tests in bug 980506 should illuminate any failures occurring (like something
> holding a strong reference to any audionode). Is this what you mean?

Yeah, although I have to admit that when it comes to this stuff, I have little faith in any trest being able to catch all future issues... :/

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1025118. Confirmed by retriggers on the Try push :).
https://hg.mozilla.org/integration/fx-team/rev/60ffe76e0c01

Comment 13

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

Attachment: 1019964-call-watcher-weak-ref.patch

Now with only web audio editor holding weak refs

Comment 14

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

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

Comment 15

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

Comment on attachment 8440163 [details] [diff] [review]
1019964-call-watcher-weak-ref.patch

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

::: toolkit/devtools/server/actors/call-watcher.js
@@ +267,5 @@
>     * Starts waiting for the current tab actor's document global to be
>     * created, in order to instrument the specified objects and become
>     * aware of everything the content does with them.
>     */
> +  setup: method(function({ tracedGlobals, tracedFunctions, startRecording, performReload, holdWeak }) {

Need to add an entry to the request: { ... } params for `method`.

Comment 16

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

Attachment: 1019964-call-watcher-weak-ref.patch

Added the request param

Comment 17

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

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/514cd89b1e44