Closed Bug 1246108 Opened 8 years ago Closed 8 years ago

Drained AudioStreams should not be restarted

Categories

(Core :: Audio/Video: Playback, defect, P1)

Other Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 + fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: alwu, Assigned: kinetik)

References

(Blocks 1 open bug)

Details

(Keywords: topcrash)

Crash Data

Attachments

(2 files)

This bug is fork from bug 1242774.

The CUBEB_STATE_DRAINED should only be notified when the audio track is ended, the cubeb backend shouldn't notify CUBEB_STATE_DRAINED more than once.

STR.
1. Run the video-replay-after-audio-end.html (from bug1242774) on Window

Expected.
2. The CUBEB_STATE_DRAINED should only be notified once

Actual.
2. The CUBEB_STATE_DRAINED is notified twice
Sounds like this affects 46 and higher from the comments / flags in bug 1245386. 
If a fix for this lands and appears to fix the crash there, please request uplift to beta. Thanks!
Matthew is taking this, but he's still coming up to speed.  We don't have an estimate on how quickly this can be fixed.

Adding JW to this bug in case he has additional info that can help Matthew come up to speed on this issue more quickly.  (Ref: https://bugzilla.mozilla.org/show_bug.cgi?id=1245386#c4 )
Assignee: nobody → kinetik
Rank: 10
Priority: -- → P1
See Also: → 1245386
This might be fixed by all the new things in cubeb_wasapi, but we might need something for uplift.
(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #2)
> Matthew is taking this, but he's still coming up to speed.  We don't have an
> estimate on how quickly this can be fixed.

I should have a fix to test tomorrow or so, I understand what's going on but need to decide on the correct approach to fix it.
The root cause of this bug, bug 1242774, and bug 1245386 is that DecodedAudioDataSink tries to restart an AudioStream that has completed and drained.  That's not permitted by libcubeb, although the API doesn't defend itself against it in all implementations, which is why we were seeing strange behaviour such as multiple calls to Drained().

This is a regression from bug 948267.  That change removed the state machine from DADS which protected calls to AudioStream::Pause/Resume by returning early once mState was not PLAYING.

This simple fix intended for uplift restores part of that state machine by tracking the drained state of the DADS and returning early from DADS::SetPlaying when the DADS has drained.
Attachment #8733123 - Flags: review?(jwwang)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Summary: cubeb_unit should only call CUBEB_STATE_DRAINED once → Drained AudioStreams should not be restarted
Blocks: 948267
This is the #5 topcrash for 46 beta 2.  
kinetik, if you do land a fix for this, please request uplift to beta, thanks!
Keywords: topcrash
Attachment #8733127 - Flags: review?(jwwang) → review+
Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

Review of attachment 8733123 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasink/DecodedAudioDataSink.cpp
@@ +126,5 @@
>    // pause/resume AudioStream as necessary.
>    if (!aPlaying && !mAudioStream->IsPaused()) {
>      mAudioStream->Pause();
>    } else if (aPlaying && mAudioStream->IsPaused()) {
>      mAudioStream->Resume();

It appears to me that the race is still there and |mAudioStream->Resume()| could still happen after draining with the following call flow:

1. mPlaybackComplete is false on the state machine thread.
2. context switch to cubeb thread to run DecodedAudioDataSink::Drained().
3. context switch to the state machine thread to call |mAudioStream->Resume()|.

However, I believe this patch will reduce the chance of crash greatly and we need to uplift to beta as soon as possible.

Please file a follow-up bug if necessary.
Attachment #8733123 - Flags: review?(jwwang) → review+
Component: Audio/Video: cubeb → Audio/Video: Playback
Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

Approval Request Comment
[Feature/regressing bug #]: bug 948267
[User impact if declined]: frequent crash (beta topcrash) playing media
[Describe test coverage new/current, TreeHerder]: test added in bug 1242774, enabled for all platforms in this bug
[Risks and why]: low risk, simple fix; regressions from fix limited to resuming playback after pause
[String/UUID change made/needed]: n/a
Attachment #8733123 - Flags: approval-mozilla-beta?
Attachment #8733123 - Flags: approval-mozilla-aurora?
Crash Signature: [@ `anonymous namespace''::wasapi_stream_start ]
Blocks: 1246104
Comment on attachment 8733123 [details] [diff] [review]
bug1246108_v0.patch

Fix for topcrash, new tests added. This should land for beta 5 on Thursday.
Attachment #8733123 - Flags: approval-mozilla-beta?
Attachment #8733123 - Flags: approval-mozilla-beta+
Attachment #8733123 - Flags: approval-mozilla-aurora?
Attachment #8733123 - Flags: approval-mozilla-aurora+
This is causing some intermittent crash test failures, https://treeherder.mozilla.org/logviewer.html#?job_id=938110&repo=mozilla-beta#L9348

I'm not sure if that means we need more attention on the patch or if it means the test is buggy.
Flags: needinfo?(kinetik)
I see both the fix and the test-enable patch were uplifted, but I only intended (and requested approval) to uplift the fix because I wasn't sure what the state of that test was with other changes between m-c (where I tested) and earlier versions.  So, one option is to pull the test-enable patch (c9e2b215d29c in beta) and go from there.
Flags: needinfo?(kinetik)
Can you also back out the test from aurora, if it looks to be failing intermittently there?
Flags: needinfo?(wkocher)
Still seeing crashes in beta 5. kinetik can you follow up?  This is marked fixed, but is the #2 topcrash in beta 5.   Re-open, or file a new bug?
Flags: needinfo?(kinetik)
it looks like the fix here landed on the beta branch after beta 5 was built...
(In reply to philipp from comment #24)
> it looks like the fix here landed on the beta branch after beta 5 was
> built...

Indeed... if I grab the source from http://ftp.mozilla.org/pub/firefox/candidates/46.0b5-candidates/build2/source/ the fix isn't present.
Flags: needinfo?(kinetik)
Thanks for catching that philipp and kinetik! Let's see how the crash signature looks after beta 6 then.
Re-setting 46 to fixed since the patch did land, just not in time for beta 5.
Flags: needinfo?(wkocher)
crashes with this signature are still occurring (though in less frequency than before the patch has landed).
(In reply to philipp from comment #29)
> crashes with this signature are still occurring (though in less frequency
> than before the patch has landed).

I guess this is the race described in comment 12.
Flags: needinfo?(kinetik)
(In reply to JW Wang [:jwwang] from comment #30)
> (In reply to philipp from comment #29)
> > crashes with this signature are still occurring (though in less frequency
> > than before the patch has landed).
> 
> I guess this is the race described in comment 12.

Seems likely, any suggestions on a better approach to fixing this?  Otherwise I can take a look, but probably not until next week at this stage.
Flags: needinfo?(kinetik)
You need to log in before you can comment on or make changes to this bug.