Closed
Bug 1373727
Opened 5 years ago
Closed 5 years ago
Fix / annotate uses of pointers to RefPtr values in Web Audio
Categories
(Core :: Web Audio, enhancement, P1)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: dminor, Assigned: dminor)
Details
(Keywords: sec-audit, Whiteboard: [adv-main56-][post-critsmash-triage])
Attachments
(1 file, 2 obsolete files)
8.42 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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!
Attachment #8878579 -
Flags: review?(karlt)
Updated•5 years ago
|
Group: mozilla-employee-confidential → media-core-security
Comment 2•5 years ago
|
||
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.
Attachment #8878579 -
Flags: review?(karlt) → review-
Assignee | ||
Comment 3•5 years ago
|
||
Thank you for the thorough review!
Attachment #8878579 -
Attachment is obsolete: true
Attachment #8883639 -
Flags: review?(karlt)
Comment 4•5 years ago
|
||
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."
Attachment #8883639 -
Flags: review?(karlt)
Assignee | ||
Comment 5•5 years ago
|
||
Attachment #8883639 -
Attachment is obsolete: true
Attachment #8885756 -
Flags: review?(karlt)
Comment 6•5 years ago
|
||
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
Attachment #8885756 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 7•5 years ago
|
||
(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.
Assignee | ||
Comment 8•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/808e35c5d4260aa04f30483884c4204a9157a785
https://hg.mozilla.org/mozilla-central/rev/808e35c5d426
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 11•5 years ago
|
||
(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.
Flags: needinfo?(dminor) → needinfo?(padenot)
Comment 12•5 years ago
|
||
Yes, this can ride the trains. We have regression benchmarks still running on AWFY, and I don't see anything strange there.
Flags: needinfo?(padenot)
Updated•5 years ago
|
Updated•5 years ago
|
Group: media-core-security → core-security-release
Updated•5 years ago
|
Whiteboard: [adv-main56-]
Updated•5 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56-] → [adv-main56-][post-critsmash-triage]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•