1373727 - Fix / annotate uses of pointers to RefPtr values in Web Audio

Status: RESOLVED FIXED

Description

[Dan Minor [:dminor]]

There are a number of places where we use pointers to values held by RefPtr in Web Audio. These should be changed to be RefPtrs or verified for safety and then annotated with MOZ_NON_OWNING_REF.

Comment 1

[Dan Minor [:dminor]]

Attachment: Fix / annotate Web Audio pointer to RefPtr held values.

This is largely padenot's work. I've gone through and double-checked things to the best of my knowledge. I'll certainly take the blame for any remaining problems!

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8878579 [details] [diff] [review]
Fix / annotate Web Audio pointer to RefPtr held values.

Please include more context and function names in patches.

>+++ b/dom/media/webaudio/AudioBufferSourceNode.cpp
>@@ -582,8 +582,8 @@ public:
>   int32_t mResamplerOutRate;
>   uint32_t mChannels;
>   float mDopplerShift;
>-  AudioNodeStream* mDestination;
>+  AudioNodeStream* MOZ_NON_OWNING_REF mDestination;

Can you document why the raw pointers are safe at their definition, please?

mSource can be documented with something like "mSource deletes this engine in
its destructor."

>-  AudioNodeStream* mSource;
>+  AudioNodeStream* MOZ_NON_OWNING_REF mSource;

The difference between MOZ_NON_OWNING_REF and MOZ_UNSAFE_REF can be fuzzy, but
I don't think mDestination fits the descriptions in the documentation of
MOZ_NON_OWNING_REF.  There is no "guarantee that the reference will be nulled
before the pointer becomes invalid".  Safety is "dependent on the behaviour of
API consumers", which suggests MOZ_UNSAFE_REF.

Documentation for MOZ_UNSAFE_REF mDestination could be something like "On
LastRelease() or Unlink(), AudioBufferSourceNode sends a destroy message to
the graph for its stream .  This engine is not used after the graph processes
the destroy message.  AudioBufferSourceNode owns mContext, which owns the
AudioDestinationNode that sends the destroy message for mDestination on
LastRelease() or Unlink().  Destroy messages are not sent to the graph thread
until stable state is reached.  When Unlink of the AudioDestinationNode occurs
before Unlink of the AudioBufferSourceNode, there are no messages sent to
this engine between the two stream destroy messages.  The graph will process
both destroy messages before the engine can be used with a dangling
mDestination."

That's rather complex.  I expect a RefPtr would also be an option, as the
AudioDestinationNode stream would not own the AudioBufferSourceNode stream or
engine.  The direction of ownership would be similar to that of the nodes.
The refcount is atomic, but there are similar atomic operations happening
during stream destruction anyway, and so I guess the change in
performance would not be noticeable.

>     // Weak reference.
>-    AudioNode* mInputNode;
>+    AudioNode* MOZ_NON_OWNING_REF mInputNode;

Something like "The InputNode is destroyed when mInputNode is disconnected."

>+++ b/dom/media/webaudio/AudioNodeEngine.h
>@@ -393,7 +393,8 @@ public:
>   }
> 
> private:
>-  dom::AudioNode* mNode; // main thread only
>+  // This is managed manually to break a cycle.
>+  dom::AudioNode* MOZ_NON_OWNING_REF mNode; // main thread only

Something like "Cleared from AudioNode::DestroyMediaStream()."

>+++ b/dom/media/webaudio/AudioNodeStream.cpp
>@@ -146,7 +146,7 @@ AudioNodeStream::SetStreamTimeParameter(
>           SetStreamTimeParameterImpl(mIndex, mRelativeToStream, mStreamTime);
>     }
>     double mStreamTime;
>-    MediaStream* mRelativeToStream;
>+    RefPtr<MediaStream> mRelativeToStream;

The message is a short lived object and so the safety of this pointer is
considerably easier to reason about.  Given the refcount is atomic and the
simpler lifetime, I think it would be nice to keep this as a bare pointer.
Stream time parameters are probably not passed too frequently right now, but
there is potential for them to be more frequent in future Web Audio APIs.
This can be MOZ_UNSAFE_REF("ControlMessages are processed in order.  This
destination stream is not yet destroyed.  Its (future) destroy message will be
processed after this message.")

>+++ b/dom/media/webaudio/blink/HRTFDatabaseLoader.cpp
>@@ -118,7 +118,7 @@ public:
>         return NS_OK;
>     }
> private:
>-    HRTFDatabaseLoader* mLoader;
>+    RefPtr<HRTFDatabaseLoader> mLoader;

I don't see how this can work.  The reference here will mean that
MainThreadRelease() will always find count > 0.  On mLoader RefPtr destruction
ProxyRelease() will repeat the cycle.

mLoader is actually already an owning pointer because the purpose of this
runnable is to remove that ownership, in a way that does not go through proxy
release.  MOZ_OWNING_REF.

I wondered about marking this as RefPtr, initializing with dont_AddRef(), and
mLoader.forget().take()->MainThreadRelease()ing.  The only distinction would
be if Run() is not called before destruction of the Runnable.  That won't
happen, given the way nsThread::DispatchInternal() leaks runnables that can't
be run, but it would be wrong to release on the wrong thread, and so I'm not
so keen on doing this.

>+++ b/dom/media/webaudio/blink/HRTFDatabaseLoader.h
>@@ -126,7 +126,7 @@ private:
>             return mLoader ? mLoader->sizeOfIncludingThis(aMallocSizeOf) : 0;
>         }
> 
>-        HRTFDatabaseLoader* mLoader;
>+        RefPtr<HRTFDatabaseLoader> mLoader;

The mechanism is that the HRTFDatabaseLoader removes itself from the
s_loaderMap hashtable on destruction.  If the hashtable entry owns the
HRTFDatabaseLoader, then the HRTFDatabaseLoader will not be deleted.  This is
MOZ_NON_OWNING_REF.

Comment 3

[Dan Minor [:dminor]]

Attachment: Fix / annotate Web Audio pointer to RefPtr held values.

Thank you for the thorough review!

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8883639 [details] [diff] [review]
Fix / annotate Web Audio pointer to RefPtr held values.

>+++ b/dom/media/webaudio/ConstantSourceNode.cpp
>@@ -134,8 +134,10 @@ public:
>     return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
>   }
> 
>-  AudioNodeStream* mSource;
>-  AudioNodeStream* mDestination;
>+  // The engine lifetime is less than the node lifetime and the node keeps the
>+  // source and destination streams alive.
>+  AudioNodeStream* MOZ_NON_OWNING_REF mSource;
>+  AudioNodeStream* MOZ_NON_OWNING_REF mDestination;

The engine lifetime can actually be longer than the node lifetime, but it is
shorter than the lifetime of the mSource stream.  That is not necessarily true
of the mDestination stream.

mSource and mDestination in all these engines will be similar to
AudioBufferSourceNode.  I suggest following the same pattern with RefPtr for
mDestination and "mSource deletes this engine in its destructor".

>+++ b/dom/media/webaudio/GainNode.cpp
>@@ -101,7 +101,7 @@ public:
>   size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const override
>   {
>     // Not owned:
>-    // - mDestination (probably)
>+    // - mDestination

With RefPtr for mDestination, this is not quite accurate because it is owned,
but it also has another owner.  Something like
"MediaStreamGraphImpl::CollectSizesForMemoryReport() accounts for
mDestination."

Comment 5

[Dan Minor [:dminor]]

Attachment: Fix / annotate Web Audio pointer to RefPtr held values.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8885756 [details] [diff] [review]
Fix / annotate Web Audio pointer to RefPtr held values.

Thanks.
Please update ~/.hgrc to include more context in patches by default.
This might be the relevant portion:

[diff]
git = true
showfunc = 1
unified = 8

Comment 7

[Dan Minor [:dminor]]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> Comment on attachment 8885756 [details] [diff] [review]
> Fix / annotate Web Audio pointer to RefPtr held values.
> 
> Thanks.
> Please update ~/.hgrc to include more context in patches by default.
> This might be the relevant portion:
> 
> [diff]
> git = true
> showfunc = 1
> unified = 8

Done. Fwiw, I was using the mach bootstrap suggested defaults.

Comment 8

[Dan Minor [:dminor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/808e35c5d4260aa04f30483884c4204a9157a785

Comment 9

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/808e35c5d426

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This can safely ride the 56 train I assume?

Comment 11

[Dan Minor [:dminor]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> This can safely ride the 56 train I assume?

I think so. We didn't find any security problems. Adding extra RefPtrs maybe has a performance impact that wouldn't show up in automation, so we might want extra time to detect them. I'll defer to Paul.

Comment 12

[Paul Adenot (:padenot)]

Yes, this can ride the trains. We have regression benchmarks still running on AWFY, and I don't see anything strange there.