Closed Bug 1460346 Opened 7 years ago Closed 6 years ago

Verify the methods being called on audio thread exclusively.

Categories

(Core :: Audio/Video: MediaStreamGraph, enhancement, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: achronop, Assigned: achronop)

References

Details

Attachments

(8 files, 14 obsolete files)

Bug 1460346 - Move checks for NextDriver member in the setter method.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1460346 - Create a method to check whether GraphDriver's thread is running.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1460346 - Assert that driver switch request comes only from audio thread.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1460346 - Assert that GraphDriver methods are called from graph thread.
59 bytes, text/x-review-board-request
pehrsons
: review+
padenot
: review+
Details
Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1460346 - Assert in SetCurrentDriver method of MSG that is called from audio thread or when the thread is not running.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1460346 - Remove unnecessary locking on an atomic value.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver.
59 bytes, text/x-review-board-request
padenot
: review+
Details
This is about creating some more asserts to verify the methods which are executed exclusively on audio tread. This is aiming mainly to GraphDriver's methods but it's extended a little on some MediaStremGraph's methods.
Assignee: nobody → achronop
Rank: 19
Priority: -- → P2
Blocks: 1459255
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review249100 ::: dom/media/GraphDriver.cpp:123 (Diff revision 1) > void GraphDriver::SetNextDriver(GraphDriver* aNextDriver) > { > + MOZ_ASSERT(aNextDriver); > GraphImpl()->GetMonitor().AssertCurrentThreadOwns(); > + > + if (mNextDriver && returning early in case the two values are equal would avoid the unnecessary store
Comment on attachment 8974450 [details] Bug 1460346 - Keep on thread method to verify when on audio thread and create a new method for audio thread not running. pehrsons https://reviewboard.mozilla.org/r/242796/#review249102 ::: commit-message-006d9:1 (Diff revision 1) > +Bug 1460346 - Update the OnThread method of GraphDriver to return true when thread is not running. r?pehrsons to me thats dangerous here, because it makes it appear that somehow the code becomes threadsafe when the thread isnt running which is definitely not true. additionally, you're not "OnThread" if it is not running. the assertions should be modified, not work around the OnThread method to make the assertion code simpler.
Attachment #8974450 - Flags: review-
Comment on attachment 8974451 [details] Bug 1460346 - Assert that driver switch request comes only from audio thread. https://reviewboard.mozilla.org/r/242798/#review249104 ::: dom/media/MediaStreamGraph.cpp:802 (Diff revision 1) > > void > MediaStreamGraphImpl::OpenAudioInputImpl(int aID, > AudioDataListener *aListener) > { > - MOZ_ASSERT(OnGraphThreadOrNotRunning()); > + MOZ_ASSERT(OnGraphThread()); what about offline mode? ::: dom/media/MediaStreamGraph.cpp:884 (Diff revision 1) > } > > void > MediaStreamGraphImpl::CloseAudioInputImpl(AudioDataListener *aListener) > { > - MOZ_ASSERT(OnGraphThreadOrNotRunning()); > + MOZ_ASSERT(OnGraphThread()); same here
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review249106 ::: dom/media/GraphDriver.cpp:114 (Diff revision 1) > } > > GraphDriver* GraphDriver::NextDriver() > { > + MOZ_ASSERT(OnThread()); > GraphImpl()->GetMonitor().AssertCurrentThreadOwns(); if you're onthread, and mNextDriver only ever gets modified when OnThread, why do we need to make use of the monitor?
Comment on attachment 8974453 [details] Bug 1460346 - Verify offline driver is not started more than once. https://reviewboard.mozilla.org/r/242802/#review249108 ::: dom/media/MediaStreamGraphImpl.h:484 (Diff revision 1) > * We can also switch from Revive() (on MainThread). Monitor must be held. > */ > void SetCurrentDriver(GraphDriver* aDriver) > { > #ifdef DEBUG > + AssertOnGraphThreadOrNotRunning(); same deal, if we are on graph thread, and mdriver only gets modified when on graph thread, why use a monitor
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review249160 ::: dom/media/GraphDriver.cpp:75 (Diff revision 1) > ("Switching to new driver: %p (%s)", > aNextDriver, > aNextDriver->AsAudioCallbackDriver() ? "AudioCallbackDriver" > : "SystemClockDriver")); > - if (mNextDriver && > - mNextDriver != GraphImpl()->CurrentDriver()) { > + > + GraphImpl()->GetMonitor().AssertCurrentThreadOwns(); It's nice if we are consistent on having thread assertions at the top of functions and methods. That said I guess this method doesn't strictly need it since it's not accessing any of the members protected by the monitor. It could still be good form. ::: dom/media/GraphDriver.cpp:124 (Diff revision 1) > { > + MOZ_ASSERT(aNextDriver); > GraphImpl()->GetMonitor().AssertCurrentThreadOwns(); > + > + if (mNextDriver && > + mNextDriver != GraphImpl()->CurrentDriver()) { Like jya said, this line would serve better as a guard.
Attachment #8974449 - Flags: review?(apehrson) → review+
Comment on attachment 8974450 [details] Bug 1460346 - Keep on thread method to verify when on audio thread and create a new method for audio thread not running. pehrsons https://reviewboard.mozilla.org/r/242796/#review249164 I agree with jya that this is changing the asserts in the wrong place. Clearing the r? until this is solved. ::: dom/media/GraphDriver.cpp:1049 (Diff revision 1) > + // If state is stopped, drained or error clear the flag > + mStarted = (aState != CUBEB_STATE_STOPPED) && > + (aState != CUBEB_STATE_DRAINED) && > + (aState != CUBEB_STATE_ERROR); To me, `mStarted` reads as it should be true once it has been started and to eternity, because at any point in the future it had been started in the past. This seems to match the current meaning of `mStarted` by reading the code. If you're changing its meaning to `mRunning` or so, we should update the naming and documentation and go through all users of it to see whether they need updating too.
Attachment #8974450 - Flags: review?(apehrson)
Comment on attachment 8974451 [details] Bug 1460346 - Assert that driver switch request comes only from audio thread. https://reviewboard.mozilla.org/r/242798/#review249104 > what about offline mode? What's the concern? Can we even get here in offline mode? FWIW OnGraphThread() is valid in offline mode too, https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/dom/media/GraphDriver.h#264.
Comment on attachment 8974451 [details] Bug 1460346 - Assert that driver switch request comes only from audio thread. https://reviewboard.mozilla.org/r/242798/#review249172 r+ with jya's concern resolved. ::: dom/media/MediaStreamGraph.cpp:3336 (Diff revision 1) > + > + graphImpl->AssertOnGraphThread(); > + Not sure this is really necessary and warranting `AssertOnGraphThread`. The calls you make have the appropriate assertions, no? And a ControlMessage always runs on the graph thread anyway.
Attachment #8974451 - Flags: review?(apehrson) → review+
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review249176 r- for the contradiction ::: dom/media/GraphDriver.cpp:798 (Diff revision 1) > + MOZ_ASSERT(NS_IsMainThread()); > // Note: only called on MainThread, without monitor > // We know were weren't in a running state > LOG(LogLevel::Debug, ("AudioCallbackDriver reviving.")); > // If we were switching, switch now. Otherwise, start the audio thread again. > MonitorAutoLock mon(mGraphImpl->GetMonitor()); > if (NextDriver()) { You assert main thread here, yet you call NextDriver() which asserts OnThread(). That's contradictive.
Attachment #8974452 - Flags: review?(apehrson) → review-
Comment on attachment 8974453 [details] Bug 1460346 - Verify offline driver is not started more than once. https://reviewboard.mozilla.org/r/242802/#review249180 I'd like to see the response to jya's monitor concern before r+. It's not exactly part of this bug, but I think we need to understand why or why not the monitor is needed before this lands. ::: dom/media/MediaStreamGraph.cpp:4020 (Diff revision 1) > // This runs on the graph thread, so when this runs, and the current > // driver is an AudioCallbackDriver, we know the audio hardware is > // started. If not, we are going to switch soon, keep reposting this > // ControlMessage. > MediaStreamGraphImpl* graphImpl = mStream->GraphImpl(); > + graphImpl->AssertOnGraphThread(); No need to assert where a ControlMessage runs. It's basically the equivalent of ``` mMainThread->Dispatch(NS_NewRunnableFunction[mainThread = mMainThread]() { MOZ_ASSERT(mainThread->IsOnCurrentThread()); ... }); ```
Attachment #8974453 - Flags: review?(apehrson)
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review249160 > It's nice if we are consistent on having thread assertions at the top of functions and methods. > > That said I guess this method doesn't strictly need it since it's not accessing any of the members protected by the monitor. It could still be good form. I was trying to reflect that the above steps do not need the mutex locked but I understand your point. > Like jya said, this line would serve better as a guard. We do not need to change that line. This checks that the current `mNextDriver` of this GraphDriver has become the current driver of the MSG so it's not a discarding driver.
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review249100 > returning early in case the two values are equal would avoid the unnecessary store I am assuming you mean `aNextDriver` and `mNextDriver`. In this case, it does not make sense to end up with the same values. I restricted the call of `SetNextDriver()` to make it impossible.
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review249106 > if you're onthread, and mNextDriver only ever gets modified when OnThread, why do we need to make use of the monitor? The monitor assert is not being added in this patch. It was added (with at one extra mutex) in https://hg.mozilla.org/mozilla-central/rev/25b10c010e66#l1.85 As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1203585#c20 I doubt that was fixing the problem reported in that bug. It is quite possible the monitor is not necessary, but I haven't audited the code.
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review249106 > The monitor assert is not being added in this patch. > It was added (with at one extra mutex) in > https://hg.mozilla.org/mozilla-central/rev/25b10c010e66#l1.85 > As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1203585#c20 > I doubt that was fixing the problem reported in that bug. > > It is quite possible the monitor is not necessary, but I haven't audited > the code. I am not planning to change the monitor's locking model in that patch. I will audit the code for that in Bug 1459255. About this case, in addition to the setter method, mNextDriver value is queried in an number of place and we need to take also that into consideration when monitor is removed.
Comment on attachment 8974453 [details] Bug 1460346 - Verify offline driver is not started more than once. https://reviewboard.mozilla.org/r/242802/#review249108 > same deal, if we are on graph thread, and mdriver only gets modified when on graph thread, why use a monitor I am not planning to change any monitor locking with that bug. Also mDriver is touched in many places and we need to review them carefully in order to remove the lock. I am focusing more on GraphDriver for now. It would be a good idea to do something similar in MSG though.
Comment on attachment 8974451 [details] Bug 1460346 - Assert that driver switch request comes only from audio thread. https://reviewboard.mozilla.org/r/242798/#review249104 > What's the concern? Can we even get here in offline mode? > > FWIW OnGraphThread() is valid in offline mode too, https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/dom/media/GraphDriver.h#264. This method and the one bellow are accessible from `MediaManager::GetUserMedia()`. The MSG used, in that case, is always real time. In general, the `ThreadedDriver::OnThread()`, which used in OfflineClockDriver, is updated follows the same logic.
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review251374 ::: dom/media/GraphDriver.cpp:120 (Diff revisions 1 - 2) > return mPreviousDriver; > } > > void GraphDriver::SetNextDriver(GraphDriver* aNextDriver) > { > - MOZ_ASSERT(aNextDriver); > + MOZ_ASSERT(aNextDriver != this); Don't you want to assert `aNextDriver != mNextDriver` too? ::: dom/media/GraphDriver.h:206 (Diff revision 2) > > virtual bool OnThread() = 0; > > protected: > GraphTime StateComputedTime() const; > + // Setting the associated pointer, assert that lock is held s/Setting/Sets/ s/, assert that/ asserting that the/
Comment on attachment 8974450 [details] Bug 1460346 - Keep on thread method to verify when on audio thread and create a new method for audio thread not running. pehrsons https://reviewboard.mozilla.org/r/242796/#review251376 ::: commit-message-006d9:1 (Diff revision 2) > +Bug 1460346 - Keep on thread method to verify when on audio thread and create a new method for audio thread not running. pehrsons s/Keep on thread method to verify when on audio thread and c/C/ (since it explains something that you haven't touched) s/pehrsons/r?pehrsons/ ::: dom/media/GraphDriver.h:240 (Diff revision 2) > // This is non-null only when this driver is going to switch to an other > // driver at the end of this iteration. > // This must be accessed using the {Set,Get}NextDriver methods. > RefPtr<GraphDriver> mNextDriver; > + // This is true if the thread is running. It is false > + // before starting the thread or after stopping it. s/or/and/ ::: dom/media/GraphDriver.h:271 (Diff revision 2) > + bool ThreadNotRunning() override > + { > + return !mThreadRunning; > + } Don't implement a subclass method based on state in a base class. This method should just go straight into the base class.
Attachment #8974450 - Flags: review?(apehrson) → review+
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review251382 ::: commit-message-d4da1:1 (Diff revision 2) > +Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. r?pehrsons This could say "Assert that GraphDriver methods are called from graph thread" to be a bit simpler. ::: dom/media/GraphDriver.cpp:748 (Diff revision 2) > } > > void > AudioCallbackDriver::Start() > { > + MOZ_ASSERT(!IsStarted()); Should this assert the thread too? ::: dom/media/GraphDriver.cpp:1052 (Diff revision 2) > AudioCallbackDriver::StateCallback(cubeb_state aState) > { > LOG(LogLevel::Debug, ("AudioCallbackDriver State: %d", aState)); What thread would this be called on? If consistent it would be good to see asserts added for this method and the others that currently don't assert its calling thread. ::: dom/media/GraphDriver.cpp:1056 (Diff revision 2) > + // If state is stopped, drained or error clear the flag > + mThreadRunning = (aState == CUBEB_STATE_STARTED); This comment says "stopped, drained or error" but we check against "started". It's not clear to the reader whether these four are all the states possible.
Attachment #8974452 - Flags: review?(apehrson) → review+
Comment on attachment 8974453 [details] Bug 1460346 - Verify offline driver is not started more than once. https://reviewboard.mozilla.org/r/242802/#review251396 r- pending clarification on commit message vs content. ::: commit-message-ffd87:1 (Diff revision 2) > +Bug 1460346 - Verify offline driver is not started more than once. r?pehrsons I don't see how the content of this patch matches the commit message. ::: dom/media/GraphDriver.cpp:233 (Diff revision 2) > }; > > void > ThreadedDriver::Start() > { > + MOZ_ASSERT(ThreadNotRunning()); Should this be in the previous part, "Create asserts for GraphDriver's methods being called from audio thread"? ::: dom/media/MediaStreamGraph.cpp:1408 (Diff revision 2) > - if (LifecycleStateRef() == LIFECYCLE_THREAD_NOT_STARTED) { > + if (LifecycleStateRef() == LIFECYCLE_THREAD_NOT_STARTED && > + !mNonRealtimeProcessing) { Isn't this a behavior change? Why is this needed now?
Attachment #8974453 - Flags: review?(apehrson) → review-
Comment on attachment 8975766 [details] Bug 1460346 - Assert one MediaStreamGraph method that is called from audio thread or when the thread is not running. https://reviewboard.mozilla.org/r/243976/#review251398 ::: commit-message-cbe03:1 (Diff revision 1) > +Bug 1460346 - Assert one MediaStreamGraph method that is called from audio thread or when the thread is not running. r?pehrsons Maybe call the method out explicitly? ::: dom/media/MediaStreamGraphImpl.h:485 (Diff revision 1) > */ > void SetCurrentDriver(GraphDriver* aDriver) > { > #ifdef DEBUG > mMonitor.AssertCurrentThreadOwns(); > + MOZ_ASSERT(mDriver->OnThread() || mDriver->ThreadNotRunning()); This doesn't need to go inside the `#ifdef DEBUG`. That ifdef probably doesn't need to exist at all, but that could be another bug.
Attachment #8975766 - Flags: review?(apehrson) → review+
Comment on attachment 8975767 [details] Bug 1460346 - Remove unnecessary locking on an atomic value. https://reviewboard.mozilla.org/r/243978/#review251402
Attachment #8975767 - Flags: review?(apehrson) → review+
Comment on attachment 8974449 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/242794/#review251374 > Don't you want to assert `aNextDriver != mNextDriver` too? Good idea. I am also moving the asserts below mutex assert.
Comment on attachment 8974452 [details] Bug 1460346 - Create asserts for GraphDriver's methods being called from audio thread. https://reviewboard.mozilla.org/r/242800/#review251382 > Should this assert the thread too? This is called from 3 different threads: the main thread, the audio thread, the AsyncCubebTask threads. I will invent something.
Comment on attachment 8974453 [details] Bug 1460346 - Verify offline driver is not started more than once. https://reviewboard.mozilla.org/r/242802/#review251396 > I don't see how the content of this patch matches the commit message. You are right, it's not very good, I will update it. > Should this be in the previous part, "Create asserts for GraphDriver's methods being called from audio thread"? This is here in order to pair it with the fix bellow. > Isn't this a behavior change? Why is this needed now? This is because of an error found on try [1]. In offline case flag `mNonRealtimeProcessing` reflects if the driver is started. We used to start the ThreadedDriver even when it was already started. [1] https://treeherder.mozilla.org/#/jobs?repo=try&author=achronop@gmail.com&selectedJob=178369609
Attachment #8974449 - Attachment is obsolete: true
Attachment #8974450 - Attachment is obsolete: true
Attachment #8974451 - Attachment is obsolete: true
Attachment #8974452 - Attachment is obsolete: true
Attachment #8974453 - Attachment is obsolete: true
Attachment #8975766 - Attachment is obsolete: true
Attachment #8975767 - Attachment is obsolete: true
Attachment #8979842 - Attachment is obsolete: true
Attachment #8979842 - Flags: review?(apehrson)
Attachment #8979843 - Attachment is obsolete: true
Attachment #8979843 - Flags: review?(apehrson)
Attachment #8979844 - Attachment is obsolete: true
Attachment #8979844 - Flags: review?(apehrson)
Attachment #8979845 - Attachment is obsolete: true
Attachment #8979845 - Flags: review?(apehrson)
Attachment #8979846 - Attachment is obsolete: true
Attachment #8979846 - Flags: review?(apehrson)
Attachment #8979847 - Attachment is obsolete: true
Attachment #8979847 - Flags: review?(apehrson)
Attachment #8979848 - Attachment is obsolete: true
Attachment #8979848 - Flags: review?(apehrson)
Attachment #8974449 - Attachment is obsolete: false
Attachment #8974450 - Attachment is obsolete: false
Attachment #8974451 - Attachment is obsolete: false
Attachment #8974452 - Attachment is obsolete: false
Attachment #8974453 - Attachment is obsolete: false
Attachment #8975766 - Attachment is obsolete: false
Attachment #8975767 - Attachment is obsolete: false
Attachment #8974449 - Attachment is obsolete: true
Attachment #8974450 - Attachment is obsolete: true
Attachment #8974452 - Attachment is obsolete: true
Attachment #8974453 - Attachment is obsolete: true
Attachment #8975766 - Attachment is obsolete: true
Attachment #8975767 - Attachment is obsolete: true
Attachment #8974451 - Attachment is obsolete: true
Comment on attachment 8980654 [details] Bug 1460346 - Move checks for NextDriver member in the setter method. https://reviewboard.mozilla.org/r/246818/#review253522
Attachment #8980654 - Flags: review?(apehrson) → review+
Comment on attachment 8980655 [details] Bug 1460346 - Create a method to check whether GraphDriver's thread is running. https://reviewboard.mozilla.org/r/246820/#review253528 It's basically there, we just need to fix the state sharing between base and sub classes. ::: dom/media/GraphDriver.h:203 (Diff revision 1) > + virtual bool ThreadNotRunning() > + { > + return !mThreadRunning; > + } Hmm, this is now defined in the base class and depends on state in the base class. But this state is set from subclasses. That makes it hard to understand desired behavior from reading code. And even if this one is simple enough, once there's precedence new methods and state like this easily gets added and it goes bad fast. See VideoTrackEncoder/VP8TrackEncoder for a case where this has gotten pretty bad (though it used to be even worse). So either put it completely in the base class, or a pure virtual method in the base class and state and impl completely in the subclass. Sorry for swaying on this. ::: dom/media/GraphDriver.h:203 (Diff revision 1) > MediaStreamGraphImpl* GraphImpl() { > return mGraphImpl; > } > > virtual bool OnThread() = 0; > + virtual bool ThreadNotRunning() It easily gets confusing when names contain negations. If I want to check whether the thread is running I need to `!ThreadNotRunning()` which is not trivial to read. Please swap this around to ThreadRunning() instead. ::: dom/media/GraphDriver.cpp:1051 (Diff revision 1) > + // If state is stopped, drained or error clear the flag > + mThreadRunning = (aState == CUBEB_STATE_STARTED); Code doesn't do what comment says. Well it does implicitly I guess, but I find it confusing.
Attachment #8980655 - Flags: review?(apehrson) → review-
Comment on attachment 8980656 [details] Bug 1460346 - Assert that driver switch request comes only from audio thread. https://reviewboard.mozilla.org/r/246822/#review253542
Attachment #8980656 - Flags: review?(apehrson) → review+
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review253546 r- for now for thread safety concerns. ::: dom/media/GraphDriver.h:502 (Diff revision 1) > /** > * Fall back to a SystemClockDriver using a normal thread. If needed, > * the graph will try to re-open an audio stream later. */ > void FallbackToSystemClockDriver(); > > + bool OnCubebOperationThread() Needs docs. ::: dom/media/GraphDriver.h:504 (Diff revision 1) > * the graph will try to re-open an audio stream later. */ > void FallbackToSystemClockDriver(); > > + bool OnCubebOperationThread() > + { > + return mInitShutdownThread && mInitShutdownThread->IsOnCurrentThreadInfallible(); Which threads can call this? mInitShutdownThread is not atomic. ::: dom/media/GraphDriver.cpp:1064 (Diff revision 1) > + // Clear the flag for the not running > + // states: stopped, drained, error. > + mThreadRunning = (aState == CUBEB_STATE_STARTED); Ah, this would have been better in the earlier patch where it was added.
Attachment #8980657 - Flags: review?(apehrson) → review-
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 Not sure I understand the problem this is fixing. r- until my concern is addressed. ::: dom/media/MediaStreamGraph.cpp:1408 (Diff revision 1) > - if (LifecycleStateRef() == LIFECYCLE_THREAD_NOT_STARTED) { > + if (LifecycleStateRef() == LIFECYCLE_THREAD_NOT_STARTED && > + !mNonRealtimeProcessing) { Hmm, how could the offline driver be started but lifecycle state says it has not been started? Or rather, how can this be ok for a clock driver but not an offline driver? Is there a deeper root problem we should fix instead? MSG lifecycle states is not my strong side, perhaps padenot needs to have a look at this as well.
Attachment #8980658 - Flags: review?(apehrson) → review-
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > Hmm, how could the offline driver be started but lifecycle state says it has not been started? > > Or rather, how can this be ok for a clock driver but not an offline driver? > > Is there a deeper root problem we should fix instead? > > > > MSG lifecycle states is not my strong side, perhaps padenot needs to have a look at this as well. Reading your older reply [1] I understand a bit better I think. So ThreadedDriver::Start() is kind of idempotent in that it does nothing if mThread has been set. This patch changes that so it is not idempotent anymore. This should be documented where Start() is declared. Not being idempotent seems fine for Start() in general, especially since it seems true for AudioCallbackDriver, but let's change it to not handle when mThread is set too (i.e., assert !mThread instead of the if in ThreadedDriver::Start). Also let's make the fix a bit more intuitive to read. Perhaps check `!CurrentDriver()->ThreadRunning()` instead of `!mNonRealtimeProcessing`. Does that fix it? [1] https://reviewboard.mozilla.org/r/242802/#comment322008
Comment on attachment 8980659 [details] Bug 1460346 - Assert in SetCurrentDriver method of MSG that is called from audio thread or when the thread is not running. https://reviewboard.mozilla.org/r/246828/#review253576 ::: dom/media/MediaStreamGraphImpl.h:483 (Diff revision 1) > * should return and pass the control to the new driver shortly after. > * We can also switch from Revive() (on MainThread). Monitor must be held. > */ > void SetCurrentDriver(GraphDriver* aDriver) > { > + MOZ_ASSERT(mDriver->OnThread() || mDriver->ThreadNotRunning()); Is there any case where this is called not on mDriver's thread but off main thread? Otherwise it would be a stricter check to `MOZ_ASSERT(mDriver->OnThread() || NS_IsMainThread())`.
Attachment #8980659 - Flags: review?(apehrson) → review+
Comment on attachment 8980660 [details] Bug 1460346 - Remove unnecessary locking on an atomic value. https://reviewboard.mozilla.org/r/246830/#review253578
Attachment #8980660 - Flags: review?(apehrson) → review+
Attachment #8980658 - Flags: review?(padenot)
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > Reading your older reply [1] I understand a bit better I think. > > So ThreadedDriver::Start() is kind of idempotent in that it does nothing if mThread has been set. This patch changes that so it is not idempotent anymore. This should be documented where Start() is declared. > > Not being idempotent seems fine for Start() in general, especially since it seems true for AudioCallbackDriver, but let's change it to not handle when mThread is set too (i.e., assert !mThread instead of the if in ThreadedDriver::Start). > > Also let's make the fix a bit more intuitive to read. > Perhaps check `!CurrentDriver()->ThreadRunning()` instead of `!mNonRealtimeProcessing`. Does that fix it? > > > [1] https://reviewboard.mozilla.org/r/242802/#comment322008 Since try logs are not available any more the command to repro this locally is (if you remove the line 1409): `./mach mochitest --keep-open=false dom/media/webaudio/test/blink/test_iirFilterNodeGetFrequencyResponse.html` I will add a second assert and document the change. Using `!CurrentDriver()->ThreadRunning()` instead of `!mNonRealTimeProcessing` is working too (if you think about it the assert does the same check), but I would prefer to avoid the redirection to current driver since there is a class member for that.
Comment on attachment 8980659 [details] Bug 1460346 - Assert in SetCurrentDriver method of MSG that is called from audio thread or when the thread is not running. https://reviewboard.mozilla.org/r/246828/#review253576 > Is there any case where this is called not on mDriver's thread but off main thread? > > Otherwise it would be a stricter check to `MOZ_ASSERT(mDriver->OnThread() || NS_IsMainThread())`. This can be called from `FallbackToSystemClockDriver()` which can be called outside the main thread from CubebOperation thread and StateCallback thread.
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review253546 > Which threads can call this? mInitShutdownThread is not atomic. A monitor added to protect it.
Comment on attachment 8980655 [details] Bug 1460346 - Create a method to check whether GraphDriver's thread is running. https://reviewboard.mozilla.org/r/246820/#review254998 r=me with nits fixed ::: dom/media/GraphDriver.h:203 (Diff revision 2) > MediaStreamGraphImpl* GraphImpl() { > return mGraphImpl; > } > > virtual bool OnThread() = 0; > + virtual bool ThreadRunning() = 0; Needs docs. ::: dom/media/GraphDriver.h:267 (Diff revision 2) > uint32_t IterationDuration() override { > return MEDIA_GRAPH_TARGET_PERIOD_MS; > } > > - bool OnThread() override { return !mThread || mThread->EventTarget()->IsOnCurrentThread(); } > + bool OnThread() override { return mThread && mThread->EventTarget()->IsOnCurrentThread(); } > + bool ThreadRunning() override {return mThreadRunning;} Coding style says to use newlines for opening/closing method brackets, but at the very least match the line above's spaces inside the brackets. ::: dom/media/GraphDriver.h:465 (Diff revision 2) > bool OnThread() override > { > return mAudioThreadId.load() == std::this_thread::get_id(); > } > > + bool ThreadRunning() override {return mAudioThreadRunning;} Same here re spaces. OnThread follows the coding style properly. ::: dom/media/GraphDriver.h:563 (Diff revision 2) > + /* This is true if the thread is running. It is false > + * before starting the thread and after stopping it. */ What is "the thread" for an audio callback driver? I suppose we don't manage the thread, so this should probably be reformulated.
Attachment #8980655 - Flags: review?(apehrson) → review+
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review255004 There are still inherent threading issues from this design (r-). ::: dom/media/GraphDriver.h:565 (Diff revisions 1 - 2) > * initialization and shutdown of the audio stream via AsyncCubebTask. */ > RefPtr<SharedThreadPool> mInitShutdownThread; > + /* It synchronises the access to the class member mInitShutdownThread which > + * points to a static ThreadPool. A static lock is not necessary because > + * it protects the access to the class member and not the ThreadPool itself. */ > + Monitor mInitShutdownThreadMonitor; No need for a Monitor here, a Mutex is sufficient. ::: dom/media/GraphDriver.cpp:493 (Diff revisions 1 - 2) > SharedThreadPool* > AudioCallbackDriver::GetInitShutdownThread() If someone calls GetInitShutdownThread() and stores the rawptr there's a risk of a UAF. It would be triggered if another thread replaces mInitShutdownThread causing the shutdown thread to be destructed, while the current thread is not done using it yet. -- Instead of constructing it lazily maybe we could assign it in the ctor and make it a `const RefPtr`? That would take care of these issues.
Attachment #8980657 - Flags: review?(apehrson) → review-
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > Since try logs are not available any more the command to repro this locally is (if you remove the line 1409): > `./mach mochitest --keep-open=false dom/media/webaudio/test/blink/test_iirFilterNodeGetFrequencyResponse.html` > > I will add a second assert and document the change. > > Using `!CurrentDriver()->ThreadRunning()` instead of `!mNonRealTimeProcessing` is working too (if you think about it the assert does the same check), but I would prefer to avoid the redirection to current driver since there is a class member for that. Hmm, add a second assert where? Well, so that test uses an OfflineAudioContext. Are you saying it's impossible that we'll ever need this check in realtime mode? Because mNonRealtimeProcessing is only ever set to true in offline mode. (this contradicts your "if you think about it the assert does the same check" so I want to make sure we both understand the issue equally well.
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review255010 I'll keep the r- pending clarification in the older open issue.
Attachment #8980658 - Flags: review?(apehrson) → review-
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review255004 > If someone calls GetInitShutdownThread() and stores the rawptr there's a risk of a UAF. > > It would be triggered if another thread replaces mInitShutdownThread causing the shutdown thread to be destructed, while the current thread is not done using it yet. > > -- > > Instead of constructing it lazily maybe we could assign it in the ctor and make it a `const RefPtr`? That would take care of these issues. I am in favor of `cosnt RefPtr` since it will eliminate the need for a mutex, and sound like a simpler design. However, I am not sure about the purpose of this lazy instantiation of the thread pool. I will add Paul in the review in case he has an opinion about it.
Attachment #8980657 - Flags: review?(padenot)
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > Hmm, add a second assert where? > > Well, so that test uses an OfflineAudioContext. Are you saying it's impossible that we'll ever need this check in realtime mode? > > Because mNonRealtimeProcessing is only ever set to true in offline mode. (this contradicts your "if you think about it the assert does the same check" so I want to make sure we both understand the issue equally well. The assert added in `ThreadedDriver::Start()`. My fix is about the offline case. I cannot comment why the `LifeCycleState` is not similar to the real time case and weather this is expected. The `MediaStreamGraph::StartNonRealtimeProcessing` does not set the `LifeCycleState` to running and I do not know if this is intentional. Instead, it makes use of that flag and that's why I am using it. Maybe Paul can shed some light on that.
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review255454 r=me with nits fixed ::: dom/media/GraphDriver.h:493 (Diff revisions 2 - 3) > - /* Fetch, or create a shared thread pool with up to one thread for > - * AsyncCubebTask. */ > - SharedThreadPool* GetInitShutdownThread(); > - > private: > + void InitializeInitShutdownThread() const; Ignore if you remove this method. Why const? It does affect state. ::: dom/media/GraphDriver.cpp:496 (Diff revisions 2 - 3) > } > > -SharedThreadPool* > -AudioCallbackDriver::GetInitShutdownThread() > +void > +AudioCallbackDriver::InitializeInitShutdownThread() const > { > - MonitorAutoLock lock(mInitShutdownThreadMonitor); > + MOZ_ASSERT(mInitShutdownThread); No need for this. ::: dom/media/GraphDriver.cpp:498 (Diff revisions 2 - 3) > - const uint32_t kIdleThreadTimeoutMs = 2000; > + const uint32_t kIdleThreadTimeoutMs = 2000; > - > - mInitShutdownThread-> > + mInitShutdownThread-> > SetIdleThreadTimeout(PR_MillisecondsToInterval(kIdleThreadTimeoutMs)); Put this in the ctor and remove the method.
Attachment #8980657 - Flags: review?(apehrson) → review+
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > The assert added in `ThreadedDriver::Start()`. > > My fix is about the offline case. I cannot comment why the `LifeCycleState` is not similar to the real time case and weather this is expected. The `MediaStreamGraph::StartNonRealtimeProcessing` does not set the `LifeCycleState` to running and I do not know if this is intentional. Instead, it makes use of that flag and that's why I am using it. Maybe Paul can shed some light on that. Ok, I'm happy to defer this patch to padenot altogether. Clearing my r?.
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review255460
Attachment #8980658 - Flags: review?(apehrson)
Comment on attachment 8983365 [details] Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver. https://reviewboard.mozilla.org/r/249274/#review256458 ::: dom/media/GraphDriver.cpp:1028 (Diff revision 3) > // or another driver has control of the graph. > mShouldFallbackIfError = false; > // Enter shutdown mode. The stable-state handler will detect this > // and complete shutdown if the graph does not get restarted. > mGraphImpl->SignalMainThreadCleanup(); > + RemoveMixerCallback(); This hunk looks good to me, thanks. Can the associated RemoveCallback() call in Revive() be removed?
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review257256 ::: dom/media/GraphDriver.cpp:1133 (Diff revision 5) > } > > void > -AudioCallbackDriver::DeviceChangedCallback() { > +AudioCallbackDriver::DeviceChangedCallback() > +{ > + MOZ_ASSERT(!OnThread()); Is that something that really is a guarantee ? I think I still need to implement it for Windows. I'll fix this assert if needed.
Attachment #8980657 - Flags: review?(padenot) → review+
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review257260
Attachment #8980658 - Flags: review?(padenot) → review+
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review257266
Attachment #8980658 - Flags: review+ → review?(padenot)
Comment on attachment 8980657 [details] Bug 1460346 - Assert that GraphDriver methods are called from graph thread. https://reviewboard.mozilla.org/r/246824/#review257256 > Is that something that really is a guarantee ? I think I still need to implement it for Windows. I'll fix this assert if needed. Thanks! This is guaranteed in the way that OnThread() method [1] and mAudioThreadId work [2]. This method returns true only if it is called within the audio callback. Even if the state callback is on the same thread the OnThread method will return false. Do you call state change callback within the audio callback? [1] https://searchfox.org/mozilla-central/source/dom/media/GraphDriver.h#455 [2] https://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#860-870
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review258158 Let's try a simpler approach.
Attachment #8980658 - Flags: review?(padenot) → review-
Comment on attachment 8983365 [details] Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver. https://reviewboard.mozilla.org/r/249274/#review258172 ::: dom/media/GraphDriver.h:579 (Diff revision 3) > * initialization and shutdown of the audio stream via AsyncCubebTask. */ > const RefPtr<SharedThreadPool> mInitShutdownThread; > /* This must be accessed with the graph monitor held. */ > AutoTArray<StreamAndPromiseForOperation, 1> mPromisesForOperation; > /* Used to queue us to add the mixer callback on first run. */ > - bool mAddedMixer; > + Atomic<bool> mAddedMixer; Please describe why this needs to be atomic.
Attachment #8983365 - Flags: review?(padenot) → review+
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review258158 Do you have anything in your mind? Is the whole thing that you want me to change or something in particular? There is an open issue in this review that it would be nice to know your opinion about it.
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > Ok, I'm happy to defer this patch to padenot altogether. Clearing my r?. This is because when shutting down the OfflineAudioContext, we're calling `MediaStreamGraph::StartNonRealtimeProcessing` with a length of 0, and then we're calling `ForceShutdown` on the MSG. I think it's better to make this simpler altogether, unclear how yet.
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review253548 > This is because when shutting down the OfflineAudioContext, we're calling `MediaStreamGraph::StartNonRealtimeProcessing` with a length of 0, and then we're calling `ForceShutdown` on the MSG. I think it's better to make this simpler altogether, unclear how yet. I am going to use the `MediaStreamGraph::StartNonRealtimeProcessing` inside `ForceShutdown` in case this is simpler.
Comment on attachment 8983365 [details] Bug 1460346 - Verify that AudioMixer'callback is removed before the destruction of GraphDriver. https://reviewboard.mozilla.org/r/249274/#review256458 > This hunk looks good to me, thanks. > > Can the associated RemoveCallback() call in Revive() be removed? Yes, we do not need that any more.
Comment on attachment 8980658 [details] Bug 1460346 - Fix a case that ThreadedDriver is started when it is already working. https://reviewboard.mozilla.org/r/246826/#review260084 ::: dom/media/MediaStreamGraph.cpp:1410 (Diff revision 6) > mForceShutdownTicket = aShutdownTicket; > MonitorAutoLock lock(mMonitor); > mForceShutDown = true; > - if (LifecycleStateRef() == LIFECYCLE_THREAD_NOT_STARTED) { > + if (IsNonRealTime()) { > + // Start the graph, but don't produce anything > + StartNonRealtimeProcessing(0); Change the comment to make it obvious that this will return early if we're already started, and that we just want to start it if it's not been started.
Attachment #8980658 - Flags: review?(padenot) → review+
Pushed by achronop@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7902372b2299 Move checks for NextDriver member in the setter method. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/544b198624c0 Create a method to check whether GraphDriver's thread is running. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/cdb41ec1ead0 Assert that driver switch request comes only from audio thread. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/9927463769f6 Assert that GraphDriver methods are called from graph thread. r=padenot,pehrsons https://hg.mozilla.org/integration/autoland/rev/24a720d9e4dd Fix a case that ThreadedDriver is started when it is already working. r=padenot https://hg.mozilla.org/integration/autoland/rev/ae5aec80f42b Assert in SetCurrentDriver method of MSG that is called from audio thread or when the thread is not running. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/6105079ea2ac Remove unnecessary locking on an atomic value. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/656a0868f578 Verify that AudioMixer'callback is removed before the destruction of GraphDriver. r=padenot
See Also: → 1415755
Depends on: 1497751
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: