Closed Bug 1192791 Opened 5 years ago Closed 5 years ago
Intervals::operator-= may miss small parts of the interval to remove
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!
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.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+
Better tests as per comment 2. Carrying r+.
Reminder to uplift to 42, as it fixes potential crashes of MSE code there.
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.
Attachment #8645699 - Flags: approval-mozilla-aurora?
Aurora try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c60c5d5f9a2
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.