If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla36

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
(Assignee)

Updated

3 years ago
Assignee: nobody → jwwang
(Assignee)

Comment 1

3 years ago
Created attachment 8510832 [details] [diff] [review]
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.
Attachment #8510832 - Flags: review?(cpearce)
(Assignee)

Comment 2

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

Updated

3 years ago
Attachment #8510832 - Flags: review?(cpearce)
(Assignee)

Comment 3

3 years ago
Created attachment 8511881 [details] [diff] [review]
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.
Attachment #8510832 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
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.
Attachment #8511881 - Flags: review?(cpearce)
Attachment #8511881 - Flags: review?(cpearce) → review+
(Assignee)

Comment 5

3 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=44a63ff6e518
Most green.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b51eff4a2ec0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b51eff4a2ec0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.