Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(232)

Issue 1692403002: MSE - Fix crash caused by incorrect GC of GOP with next buffer position (Closed)

Created:
4 years, 10 months ago by wolenetz
Modified:
4 years, 10 months ago
Reviewers:
chcunningham, servolk
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611, 569755, 584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 Committed: https://crrev.com/228eb2e7e66e83b3b767625fc13dbeebf2118050 Cr-Commit-Position: refs/heads/master@{#375732}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Stick with DCHECKs for this to ease merging. +unit test #

Total comments: 16

Patch Set 3 : Fixed typo in new unit test's comment per CR #

Patch Set 4 : Increased hardening + more code comments per chat w/chcunningham@ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -4 lines) Patch
M media/filters/source_buffer_range.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M media/filters/source_buffer_range.cc View 1 2 3 3 chunks +3 lines, -1 line 0 comments Download
M media/filters/source_buffer_stream.cc View 1 2 3 2 chunks +14 lines, -3 lines 0 comments Download
M media/filters/source_buffer_stream_unittest.cc View 1 2 1 chunk +26 lines, -0 lines 0 comments Download

Messages

Total messages: 37 (14 generated)
wolenetz
Please take a look. I'd like to land this, then bake and merge back as ...
4 years, 10 months ago (2016-02-12 23:43:25 UTC) #2
DaleCurtis
Do you always want these CHECKs enabled? Earlier I was just talking about a temporary ...
4 years, 10 months ago (2016-02-12 23:44:48 UTC) #4
wolenetz
On 2016/02/12 23:43:25, wolenetz wrote: > Please take a look. I'd like to land this, ...
4 years, 10 months ago (2016-02-12 23:44:59 UTC) #6
wolenetz
On 2016/02/12 23:44:48, DaleCurtis wrote: > Do you always want these CHECKs enabled? Earlier I ...
4 years, 10 months ago (2016-02-12 23:57:41 UTC) #7
wolenetz
https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer_range.cc File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer_range.cc#newcode358 media/filters/source_buffer_range.cc:358: return (GetEndTimestamp() < media_time); Alternatively, s/GetEndTimestamp()/GetBufferedEndTimestamp()/ here might similarly ...
4 years, 10 months ago (2016-02-13 00:51:06 UTC) #8
wolenetz
Please take a look at patch set 2. Thanks! https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode859 media/filters/source_buffer_stream.cc:859: ...
4 years, 10 months ago (2016-02-13 01:35:07 UTC) #11
wolenetz
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream_unittest.cc File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream_unittest.cc#newcode2460 media/filters/source_buffer_stream_unittest.cc:2460: NewSegmentAppend("1000K 1010 1020 1030 1040"); On 2016/02/13 01:35:07, wolenetz ...
4 years, 10 months ago (2016-02-13 01:52:02 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/20001
4 years, 10 months ago (2016-02-13 02:29:24 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-13 03:54:31 UTC) #16
servolk
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode859 media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/13 01:35:07, wolenetz wrote: > This ...
4 years, 10 months ago (2016-02-16 19:28:38 UTC) #18
wolenetz
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode859 media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/16 19:28:38, servolk wrote: > On ...
4 years, 10 months ago (2016-02-16 20:12:47 UTC) #19
chcunningham
1 significant comment, 1 nit. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode845 media/filters/source_buffer_stream.cc:845: if (reverse_direction) { I ...
4 years, 10 months ago (2016-02-16 21:27:02 UTC) #20
wolenetz
Please take a look at patch set 3. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode845 media/filters/source_buffer_stream.cc:845: if ...
4 years, 10 months ago (2016-02-16 22:05:05 UTC) #21
chcunningham
On 2016/02/16 22:05:05, wolenetz wrote: > Please take a look at patch set 3. > ...
4 years, 10 months ago (2016-02-16 23:01:40 UTC) #22
servolk
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_buffer_stream.cc#newcode845 media/filters/source_buffer_stream.cc:845: if (reverse_direction) { On 2016/02/16 21:27:02, chcunningham wrote: > ...
4 years, 10 months ago (2016-02-16 23:11:24 UTC) #23
chcunningham
errr... sorry, these comments didn't post last time. Still LGTM https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer_range.cc File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer_range.cc#newcode358 ...
4 years, 10 months ago (2016-02-16 23:36:25 UTC) #24
wolenetz
servolk@, did you want to CR this or just be cc'ed? If the former, it's ...
4 years, 10 months ago (2016-02-16 23:37:55 UTC) #25
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/60001
4 years, 10 months ago (2016-02-16 23:39:37 UTC) #27
servolk
On 2016/02/16 23:37:55, wolenetz wrote: > servolk@, did you want to CR this or just ...
4 years, 10 months ago (2016-02-16 23:52:30 UTC) #28
wolenetz
Thank you for the detailed review.
4 years, 10 months ago (2016-02-16 23:57:57 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/60001
4 years, 10 months ago (2016-02-16 23:59:43 UTC) #33
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 10 months ago (2016-02-17 00:48:55 UTC) #35
commit-bot: I haz the power
4 years, 10 months ago (2016-02-17 00:50:55 UTC) #37
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/228eb2e7e66e83b3b767625fc13dbeebf2118050
Cr-Commit-Position: refs/heads/master@{#375732}

Powered by Google App Engine
This is Rietveld 408576698