Minor bug in “Weak references and finalizers” blog post, it still leaks

[acelent]

In the blog post “Weak references and finalizers”, there's this note:

v8.dev/src/features/weak-references.md

Line 158 in eddc030

**Note:** `WeakRef`s to functions must be treated with caution. JavaScript functions are [closures](https://en.wikipedia.org/wiki/Closure_(computer_programming)) and strongly reference the outer environments which contain the values of free variables referenced inside the functions. These outer environments may contain variables that _other_ closures reference as well. That is, when dealing with closures, their memory is often strongly referenced by other closures in subtle ways. This is the reason `addWeakListener` is a separate function and `wrapper` is not local to the `MovingAvg` constructor. In V8, if `wrapper` were local to the `MovingAvg` constructor and shared the lexical scope with the listener that is wrapped in the `WeakRef`, the `MovingAvg` instance and all its properties become reachable via the shared environment from the wrapper listener, causing the instance to be uncollectible. Keep this in mind when writing code.

Note: WeakRefs to functions must be treated with caution. JavaScript functions are closures and strongly reference the outer environments which contain the values of free variables referenced inside the functions. These outer environments may contain variables that other closures reference as well. That is, when dealing with closures, their memory is often strongly referenced by other closures in subtle ways. This is the reason addWeakListener is a separate function and wrapper is not local to the MovingAvg constructor. In V8, if wrapper were local to the MovingAvg constructor and shared the lexical scope with the listener that is wrapped in the WeakRef, the MovingAvg instance and all its properties become reachable via the shared environment from the wrapper listener, causing the instance to be uncollectible. Keep this in mind when writing code.

The blog post's code (Code 1)

function addWeakListener(socket, listener) {
  const weakRef = new WeakRef(listener);
  const wrapper = (ev) => { weakRef.deref()?.(ev); };
  socket.addEventListener('message', wrapper);
}

class MovingAvg {
  constructor(socket) {
    this.events = [];
    this.listener = (ev) => { this.events.push(ev); };
    addWeakListener(socket, this.listener);
  }
}

If we consider “closures […] strongly reference the outer environments which contain the values of free variables referenced inside the functions.”, then there's still a leak with the seperate addWeakListener.

The code that would cause “the instance to be uncollectible” (Code 2)

class MovingAvg {
  constructor(socket) {
    this.events = [];
    this.listener = (ev) => { this.events.push(ev); };
    const weakRef = new WeakRef(this.listener);
    const wrapper = (ev) => { weakRef.deref()?.(ev); };
    socket.addEventListener('message', wrapper);
  }
}

The wrapper arrow function should capture addWeakListener's environment, where the listener argument points to the listener arrow function, which captures contructor's environment, where the this binding points to the new MovingAvg.

Reference graph for Code 1 from `Socket`

flowchart
    subgraph Socket[:Socket]
        Socket.onmessage([onmessage])
    end
    subgraph envConstructor [":⟪env⟫ constructor()"]
        constructor.this([this])
        constructor.socket([socket])
    end
    subgraph MovingAvg[:MovingAvg]
        MovingAvg.listener([listener])
    end
    subgraph envAddWeakListener [":⟪env⟫ addWeakListener()"]
        addWeakListener.socket([socket])
        addWeakListener.listener([listener])
        addWeakListener.weakRef([weakRef])
        addWeakListener.wrapper([wrapper])
    end
    subgraph Listener [":listener(ev)"]
        Listener.env(["[[Environment]]"])
    end
    subgraph WeakRef [":WeakRef"]
        WeakRef.weakRefTarget(["[[WeakRefTarget]]"])
    end
    subgraph Wrapper [":wrapper(ev)"]
        Wrapper.env(["[[Environment]]"])
    end

    constructor.this == 5 ==> MovingAvg
    constructor.socket --> Socket
    Listener.env == 4 ==> envConstructor
    MovingAvg.listener --> Listener
    addWeakListener.socket --> Socket
    addWeakListener.listener == 3 ==> Listener
    WeakRef.weakRefTarget -.-> Listener
    addWeakListener.weakRef --> WeakRef
    Wrapper.env == 2 ==> envAddWeakListener
    addWeakListener.wrapper --> Wrapper
    Socket.onmessage == 1 ==> Wrapper

    classDef cluster text-decoration:underline
    style Socket stroke-width:4px

Loading

Object diagram for Code 1 from `Socket`

---
config:
    class:
        hideEmptyMembersBox: true
---
classDiagram
    class Socket[":Socket"] {}
    class MovingAvg[":MovingAvg"] {}
    class envConstructor[":⟪env⟫ constructor()"] {}
    class Listener[":listener(ev)"] {}
    class envAddWeakListener[":⟪env⟫ addWeakListener()"] {}
    class WeakRef[":WeakRef"] {}
    class Wrapper[":wrapper(ev)"] {}

    envConstructor --> MovingAvg : 5<br>this
    envConstructor --> Socket : socket
    Listener --> envConstructor: 4<br>[[Environment]]
    MovingAvg --> Listener: listener
    envAddWeakListener --> Socket : socket
    envAddWeakListener --> Listener : 3<br>listener
    WeakRef ..> Listener: [[WeakRefTarget]]
    envAddWeakListener --> WeakRef : weakRef
    Wrapper --> envAddWeakListener : 2<br>[[Environment]]
    envAddWeakListener --> Wrapper : wrapper
    Socket --> Wrapper: 1<br>onmessage

    classDef default :,text-decoration:underline
    style Socket :,stroke-width:4px

Loading

Reference graph for Code 2 from `Socket`

flowchart
    subgraph Socket[:Socket]
        Socket.onmessage([onmessage])
    end
    subgraph envConstructor [":⟪env⟫ constructor()"]
        constructor.this([this])
        constructor.socket([socket])
        constructor.weakRef([weakRef])
        constructor.wrapper([wrapper])
    end
    subgraph MovingAvg[:MovingAvg]
        MovingAvg.listener([listener])
    end
    subgraph Listener [":listener(ev)"]
        Listener.env(["[[Environment]]"])
    end
    subgraph WeakRef [":WeakRef"]
        WeakRef.weakRefTarget(["[[WeakRefTarget]]"])
    end
    subgraph Wrapper [":wrapper(ev)"]
        Wrapper.env(["[[Environment]]"])
    end

    constructor.this == 3 ==> MovingAvg
    constructor.socket --> Socket
    Listener.env --> envConstructor
    MovingAvg.listener --> Listener
    WeakRef.weakRefTarget -.-> Listener
    constructor.weakRef --> WeakRef
    Wrapper.env == 2 ==> envConstructor
    constructor.wrapper --> Wrapper
    Socket.onmessage == 1 ==> Wrapper

    classDef cluster text-decoration:underline
    style Socket stroke-width:4px

Loading

Object diagram for Code 2 from `Socket`

---
config:
    class:
        hideEmptyMembersBox: true
---
classDiagram
    class Socket[":Socket"] {}
    class MovingAvg[":MovingAvg"] {}
    class envConstructor[":⟪env⟫ constructor()"] {}
    class Listener[":listener(ev)"] {}
    class WeakRef[":WeakRef"] {}
    class Wrapper[":wrapper(ev)"] {}

    envConstructor --> MovingAvg : 3<br>this
    envConstructor --> Socket : socket
    Listener --> envConstructor: [[Environment]]
    MovingAvg --> Listener: listener
    WeakRef ..> Listener: [[WeakRefTarget]]
    envConstructor --> WeakRef : weakRef
    Wrapper --> envConstructor : 2<br>[[Environment]]
    envConstructor --> Wrapper : wrapper
    Socket --> Wrapper: 1<br>onmessage

    classDef default :,text-decoration:underline
    style Socket :,stroke-width:4px

Loading

These diagrams are ugly as they are, so I omitted the arguments bindings in both the constructor and addWeakListener. However, this may be an important detail.

In the code with the separate addWeakListener, there's a simple solution: create the WeakRef in the constructor and pass it to addWeakListener, so that the wrapper closure doesn't capture an environment with a strong reference to MovingAvg's listener.

The blog post's code (Code 1) corrected

function addWeakListener(socket, weakRef) {
  const wrapper = (ev) => { weakRef.deref()?.(ev); };
  socket.addEventListener('message', wrapper);
}

class MovingAvg {
  constructor(socket) {
    this.events = [];
    this.listener = (ev) => { this.events.push(ev); };
    const weakRef = new WeakRef(listener);
    addWeakListener(socket, weakRef);
  }
}

Another option is to clear strong references to MovingAvg's listener, by setting the listener argument and arguments[1] (important detail) to null. It works, but from my point of view, it's not as elegant, as it performs otherwise needless assignments to clear the strong references.

Perhaps a hypothetical compiler could capture only the needed bindings for a closure, and somehow get rid of them when no longer needed, and none of this would be needed?

However, it seems that a function referring strongly to its outer environment, which itself refers strongly to all its bindings' values (and so on for outer environments), is not only conforming with, but actually required by the ECMAScript spec.

The blog post's code (Code 1) less elegantly corrected

function addWeakListener(socket, listener) {
  const weakRef = new WeakRef(listener);
  listener = arguments[1] = null;
  const wrapper = (ev) => { weakRef.deref()?.(ev); };
  socket.addEventListener('message', wrapper);
}

class MovingAvg {
  constructor(socket) {
    this.events = [];
    this.listener = (ev) => { this.events.push(ev); };
    addWeakListener(socket, this.listener);
  }
}

There's no solution that keeps all code directly in the constructor, since it's not possible to assign to this to clear it. We need an environment without a strong reference to the new MovingAvg, which can only be done with a function defined outside the constructor.