Intervals::operator-= may miss small parts of the interval to remove

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks: 1 bug)

1.0 Branch
mozilla43
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Following bug 1191142: With a small eviction threshold (e.g. 9,000,000), youtube videos may gather "holes", eventually leading to a crash in TrackBuffersManager::CheckNextInsertionIndex when seeking and changing resolution repeatedly.

The root cause is actually the '-=' operation on buffered ranges when removing frames, which may leave some buffered range intervals that are not actually present in the frames. CheckNextInsertionIndex relies on a 1:1 correspondance between buffered-ranges and actual frames to work properly.

|i1 -= i2| operates by creating an opposite of i2 and doing an intersection with i1.
Unfortunately when building the complementary of i2, the |+| operator collapses close-enough intervals together according to the fuzz. E.g [4,6 +/-1] becomes [-inf,4 +/-1]+[6 +/-1, +inf], which collapses into [-inf, +inf].

The solution should be to simply ignore the fuzz factor when building the complementary of i2. To be properly vetted and tested!
(Assignee)

Comment 1

4 years ago
When building complementary interval (to intersect with 1st argument), don't use fuzz.
Added unit tests.
Assignee: nobody → gsquelart
Attachment #8645684 - Flags: review?(jyavenard)
Comment on attachment 8645684 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch

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

::: dom/media/gtest/TestIntervalSet.cpp
@@ +789,5 @@
> +  EXPECT_EQ(10, i0[1].mEnd);
> +
> +  i0 = IntIntervals();
> +  i0 += IntInterval(0, 10);
> +  i2 = IntInterval(4, 6);

probably would be more obvious on what is being test with 4,5 as interval.

@@ +790,5 @@
> +
> +  i0 = IntIntervals();
> +  i0 += IntInterval(0, 10);
> +  i2 = IntInterval(4, 6);
> +  i2.SetFuzz(1);

what about an extra test with setting fuzz on i0?
Attachment #8645684 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 3

4 years ago
Better tests as per comment 2. Carrying r+.
Attachment #8645684 - Attachment is obsolete: true
Attachment #8645699 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aab36e44ccf4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 7

4 years ago
Reminder to uplift to 42, as it fixes potential crashes of MSE code there.
Flags: needinfo?(gsquelart)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8645699 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch v2

Approval Request Comment
[Feature/regressing bug #]: MSE

[User impact if declined]:
When seeking around videos (e.g. YouTube videos), Firefox may crash.
This bug is probably not critical though, as it only happens when heavy eviction happens, probably because the user would have modified the eviction threshold in about:config.

[Describe test coverage new/current, TreeHerder]:
Lots of mochitests covering video playback.
Also this patch adds a unit test for this particular failure.

[Risks and why]: Very low.
The unit test demonstrated the issue before the patch.
And this patch just fixes the way subtracting an interval from an interval set works, but suppressing the 'fuzziness' that would wrongly join nearby intervals, which should just not happen in a subtraction.

[String/UUID change made/needed]: None.
Flags: needinfo?(gsquelart)
Attachment #8645699 - Flags: approval-mozilla-aurora?
status-firefox42: --- → affected
Comment on attachment 8645699 [details] [diff] [review]
1192791-disregard-fuzz-in-subtraction.patch v2

Fix a crash, low risk, taking it!
Attachment #8645699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.