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

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=37849497&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/GNbsvNhZS56AYEIggv_czw/runs/0/artifacts/public%2Flogs%2Flive_backing.log

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

Review commit: https://reviewboard.mozilla.org/r/86754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86754/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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()'.

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8802344 [details]
Bug 1311137: [MSE] Ensure buffered range doesn't get modified during run.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86754/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df1b230e2444
[MSE] Ensure buffered range doesn't get modified during run. r=gerald

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Jean-Yves Avenard [:jya]]

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

Comment 8

[Gerry Chang [:gchang]]

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.

Comment 9

[Carsten Book [:Tomcat]]

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')

Comment 10

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/518df730e6bafdca866c69a7beddb0b3c0b7d116