This is necessary to then compute AudioParam values for the right time.

This covers only the main thread part of this attribute.

With respect to the rooting hazard at AudioWorkletNode.cpp:471: the difficulty is that you've declared that nothing will GC (via AutoCheckCannotGC at line 462), which is good because dest might be stored directly within a GC thing and therefore be invalidated by the GC. I'll note that the scope of that AutoCheckCannotGC is too large; it prevents any GC from that point in the function to the end, which seems difficult to enforce. You'll probably want to move it within the for loop body. It won't do anything at all in a non-DEBUG build, so it's not a performance concern.

But the actual problem is that GetValuesAtTime() is eager to expire all events older than the queried one (!?), which freaks out the analysis because it sees ~AudioTimelineEvent calling ~AudioNodeStream calling ~MediaStream calling all kinds of crazy stuff. I'm not entirely sure why it thinks that call chain is possible -- it says ~AudioTimelineEvent calls ~RefPtr<AudioNodeStream>, which seems odd since an AudioTimelineEvent has a RefPtr<AudioNodeTrack> not RefPtr<AudioNodeStream>, but whatever. I think you can safely say that the refcount will not drop to zero and so this case will not trigger a GC. The most straightforward way to do that would be to make a small scope around the CleanupEventsOlderThan call:

   {
     // Removing events will not trigger the (stream? track?) refcount to drop to zero because all of the events will
     // be for the same (stream? track?) and we will leave at least one behind, so we won't trigger GC from here.
     JS::AutoSuppressGCAnalysis nogc;
     CleanupEventsOlderThan(aTime);
    }

...except make the comment say something that makes sense and is true. :-)

Then again, I'm not confident I'm following this code correctly. Because it seems weird that something called GetValuesAtTime would be destructive, and in fact could trigger a seemingly O(n^2) destruction process (the cleanup scans oldest first, and if it finds something to expire does an O(n) RemoveElementAt(0) call.) But maybe you just never build up stale stuff? Still, it seems weird that the loop in the original code would call GetValuesAtTime() for every element, thus guaranteeing that you're discarding all but one? I'm probably just misreading things.

Thank you for having a look, Steve.

safely say that the refcount will not drop to zero and so this case will not trigger a GC.

The refcount can drop to zero, so I assume the choice is one of:

1. We need to be sure that ~MediaStream can't trigger GC,
2. We copy from a temporary intermediate buffer,
3. We use a different kind of array buffer with client-provided contents so that the buffer can be accessed via means other than JS_GetFloat32ArrayData(), or
4. Redesign GetValuesAtTime() to avoid side effects, and use a different method to purge old events.

For the rooting hazard issue, we can also never destroy AudioEventTimeline containing mTrack on the render thread, this is rather easy and has the right property, but this property cannot be checked statically so we'd have to use the thing Steve mentioned above. AudioTimelineEvent doesn't need to keep a reference to the AudioNodeTrack, it's only to pass the track to the AudioParamTimeline. Another way would be to stop overloading AudioTimelineEvent to pass the stream (it's not really an event on the AudioParam timeline, it was just convenient to do it like that at the time). This would have the benefit of silencing the analysis.

Generally the AudioParam code is slow, incomplete/wrong and complicated, so I'm not really willing to do a fix that is too invasive, and we're not going to rewrite it now, because we're trying to ship AudioWorklet, but we should.

(In reply to Paul Adenot (:padenot) from comment #10)

AudioTimelineEvent doesn't need to keep a reference to the AudioNodeTrack, it's only to pass the track to the AudioParamTimeline.

Ah, good point. AudioParamTimeline::InsertEvent() does not actually save the event when it contains a track

So the RemoveElementAt() in CleanupEventsOlderThan() could follow an assert !mEvents[0].mTrack and be wrapped in AutoSuppressGCAnalysis.

Another way would be to stop overloading AudioTimelineEvent to pass the stream (it's not really an event on the AudioParam timeline, it was just convenient to do it like that at the time). This would have the benefit of silencing the analysis.

Yes, that would work too and sounds good, but I haven't looked into what that would involve.

https://hg.mozilla.org/mozilla-central/rev/be37fac7bc9e
https://hg.mozilla.org/mozilla-central/rev/8cb2c6c40eac
https://hg.mozilla.org/mozilla-central/rev/0561d9bf849f
https://hg.mozilla.org/mozilla-central/rev/95fddf6af53d
https://hg.mozilla.org/mozilla-central/rev/96e097707886
https://hg.mozilla.org/mozilla-central/rev/3f06a2523af9
https://hg.mozilla.org/mozilla-central/rev/03bb577930c8
https://hg.mozilla.org/mozilla-central/rev/2e184c82e740

Thank you for landing this, Paul.

I wonder whether the last changeset was the version that you intended to land given further changes described in https://phabricator.services.mozilla.com/D64759#1991643 ?

Indeed. It's functionally equivalent however, I'll fix that later.