1088481 - ASSERTION: Progress timer should've been stopped (mse/webaudio)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Steps to reproduce:

* Run Firefox (with MSE enabled)
* Watch a video (check stats for nerds Dash: Yes)
* Exit

Expected results:

Nothing special

Actual results:

[Child 23404] ###!!! ASSERTION: Progress timer should've been stopped.: '!mProgressTimer', file /home/ajones/src/mozilla-central/content/media/MediaDecoder.cpp, line 503

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1088481_fix_MediaDecoder_Progress_time.patch

Since MediaShutdownManager will call MediaDecoder::Shutdown() without checking network status as the media element does, I will remove the assertion and stop the progress timer if necessary.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c4835a9d974
Most green.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1088481_fix_MediaDecoder_Progress_time-v2.patch

Since MediaShutdownManager will call MediaDecoder::Shutdown() without checking network status as the media element does, I will remove the assertion and stop the progress timer if necessary. Since HTMLMediaElement::~HTMLMediaElement() could come after MediaShutdownManager::Shutdown, HTMLMediaElement::ShutdownDecoder() should not call mDecoder->StopProgress either.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8511881 [details] [diff] [review]
1088481_fix_MediaDecoder_Progress_time-v2.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ -609,5 @@
> -  // TODO: This should be handled by ChangeNetworkState() so we have only one
> -  // place to call StopProgress().
> -  if (mNetworkState == nsIDOMHTMLMediaElement::NETWORK_LOADING) {
> -    mDecoder->StopProgress();
> -  }

HTMLMediaElement::ShutdownDecoder could happen after MediaDecoder::Shutdown which has stopped the progress timer. So we remove the code and let MediaDecoder::Shutdown handle it.

::: dom/media/MediaDecoder.cpp
@@ +498,5 @@
>    }
>  
>    ChangeState(PLAY_STATE_SHUTDOWN);
>  
> +  if (mProgressTimer) {

The progress timer should be associated with the network state. However, MediaDecoder::Shutdown could be called by MediaShutdownManager without checking network status as the media element. So we stop it anyway if necessary.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=44a63ff6e518
Most green.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b51eff4a2ec0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b51eff4a2ec0