Intermittent /media-source/SourceBuffer-abort-updating.html | application crashed [@ mozilla::media::Interval<mozilla::media::TimeUnit>::Interval<mozilla::media::TimeUnit, mozilla::media::TimeUnit&>]

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: intermittent-bug-filer, Assigned: jya)

Tracking

(Depends on 1 bug, {intermittent-failure})

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment)

Depends on: 1311173
Assignee: nobody → jyavenard
Comment on attachment 8802344 [details]
Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

https://reviewboard.mozilla.org/r/86754/#review85706

r+ with commit-description nits, and a suggestion (for now, or FYI for later code).

::: dom/media/mediasource/TrackBuffersManager.h:1
(Diff revision 1)
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

In the commit description:

'modify' -> 'modified'.

Also, "during the run" is a bit ambiguous. How about "during Buffered() call"?

Also also, the bug and its fix are quite subtle, I think it would be nice to add a short explanation in the commit description, e.g.: "Previously two separate monitors were used to find the highest end time, and then work on tracks, introducing the possibility that tracks could be modified between these two operations. Now only one monitor is used to ensure consistency."
(If I'm correct.)

::: dom/media/mediasource/TrackBuffersManager.h:471
(Diff revision 1)
> +  // Monitor must be held.
> +  media::TimeUnit HighestEndTime(nsTArray<const media::TimeIntervals*>& aTracks) const;

"Monitor must be held."
This comment is a bit scary!
The method is private, so hopefully unlikely to get wrongly used.

Suggestion:
If there is any possibility that this may hurt us in the future, a trick I've seen elsewhere is to explicitly require a proof of the lock in the function parameters, e.g.:
> ...HighestEndTime(..., const MonitorAutoLock& aProofOfLock)
(This parameter is not actually used in the definition.)

The extra argument in the caller should be very cheap, or even be optimized away.

And it's done at compile time, which should be cheaper and more reliable than 'mMonitor.AssertCurrentThreadOwns()'.
Attachment #8802344 - Flags: review?(gsquelart) → review+
Comment on attachment 8802344 [details]
Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

https://reviewboard.mozilla.org/r/86754/#review85706

> "Monitor must be held."
> This comment is a bit scary!
> The method is private, so hopefully unlikely to get wrongly used.
> 
> Suggestion:
> If there is any possibility that this may hurt us in the future, a trick I've seen elsewhere is to explicitly require a proof of the lock in the function parameters, e.g.:
> > ...HighestEndTime(..., const MonitorAutoLock& aProofOfLock)
> (This parameter is not actually used in the definition.)
> 
> The extra argument in the caller should be very cheap, or even be optimized away.
> 
> And it's done at compile time, which should be cheaper and more reliable than 'mMonitor.AssertCurrentThreadOwns()'.

no idea what problem you are attempting to prevent, nor what you mean here.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df1b230e2444
[MSE] Ensure buffered range doesn't get modified during run. r=gerald
Component: Audio/Video → Audio/Video: Playback
https://hg.mozilla.org/mozilla-central/rev/df1b230e2444
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1297037
Comment on attachment 8802344 [details]
Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

Approval Request Comment
[Feature/regressing bug #]: 1297037
[User impact if declined]: invalid buffered range returned (non spec compliant), will assert on debug build
[Describe test coverage new/current, TreeHerder]: In central
[Risks and why]: None, we revert to a logically identical code before 1297037.
[String/UUID change made/needed]: None
Attachment #8802344 - Flags: approval-mozilla-aurora?
Comment on attachment 8802344 [details]
Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

Fix an intermittent-failure. Take it in 51 aurora.
Attachment #8802344 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to apply to aurora

grafting 370288:df1b230e2444 "Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run. r=gerald"
merging dom/media/mediasource/TrackBuffersManager.cpp
merging dom/media/mediasource/TrackBuffersManager.h
warning: conflicts while merging dom/media/mediasource/TrackBuffersManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.