Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133143003:
Allow multiple URL.createObjectURL invocations for MediaSource (Closed)
|
Created:
4 years, 5 months ago by servolk Modified:
4 years, 5 months ago CC:
chromium-reviews, blink-reviews, feature-media-reviews_chromium.org, haraken, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow multiple URL.createObjectURL invocations for MediaSource
Currently calling URL.createObjectURL for a MediaSource more than once
causes an assert/crash due to MediaSource::m_isAddedToRegistry flag
being already set. But since calling createObjectURL is actually a
valid thing to do and should just generate new unique URLs, let's
replace the m_isAddedToRegistry flag with a counter.
BUG=604425
Committed: https://crrev.com/014758724726a61d5a54df88e4fe993436a17442
Cr-Commit-Position: refs/heads/master@{#404879}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR feedback #
Total comments: 2
Patch Set 3 : nit #
Messages
Total messages: 33 (12 generated)
servolk@chromium.org changed reviewers: + foolip@chromium.org, wolenetz@chromium.org
Looking pretty good. Just a couple comments. https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html (right): https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html:118: }, 'Calling URL.createObjectURL multiple times should generate unique URLs each time'); I like this new test. Please also consider adding coverage that verifies that url1, if revoked, doesn't prevent url2 from being used to dereference mediaSource (and that url1, once revoked, can't be used to dereference mediaSource). The latter might overlap existing coverage a little, but I think would still add value given that we're testing more than one objectURL for the same mediaSource here. https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:305: ++m_addedToRegistryCounter; A web app would have to work really hard to overflow the int type here (e.g. 10,000 createObjectURL/second for about 60 hours straight), but that leaves open the possibility for hasPendingActivity() to return false incorrectly and for this object to be GC'ed prematurely, leading to potential use-after-free vulnerability. Please protect against this (perhaps a simple CHECK_GT(m_addedToRegistryCounter,0), not a DCHECK, here would suffice (after doing the increment)).
https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html (right): https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html:118: }, 'Calling URL.createObjectURL multiple times should generate unique URLs each time'); On 2016/07/11 18:03:17, wolenetz wrote: > I like this new test. Please also consider adding coverage that verifies that > url1, if revoked, doesn't prevent url2 from being used to dereference > mediaSource (and that url1, once revoked, can't be used to dereference > mediaSource). The latter might overlap existing coverage a little, but I think > would still add value given that we're testing more than one objectURL for the > same mediaSource here. Done. https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/2133143003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:305: ++m_addedToRegistryCounter; On 2016/07/11 18:03:17, wolenetz wrote: > A web app would have to work really hard to overflow the int type here (e.g. > 10,000 createObjectURL/second for about 60 hours straight), but that leaves open > the possibility for hasPendingActivity() to return false incorrectly and for > this object to be GC'ed prematurely, leading to potential use-after-free > vulnerability. Please protect against this (perhaps a simple > CHECK_GT(m_addedToRegistryCounter,0), not a DCHECK, here would suffice (after > doing the increment)). Done.
lgtm % nit https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html (right): https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html:121: test.expectEvent(mediaSource, 'sourceopen', 'Source reopened'); nit: s/reopened/opened/
https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html (right): https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html:121: test.expectEvent(mediaSource, 'sourceopen', 'Source reopened'); On 2016/07/11 18:54:41, wolenetz wrote: > nit: s/reopened/opened/ Done.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2133143003/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/11 19:54:10, servolk wrote: > https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html > (right): > > https://codereview.chromium.org/2133143003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-multiple-attach.html:121: > test.expectEvent(mediaSource, 'sourceopen', 'Source reopened'); > On 2016/07/11 18:54:41, wolenetz wrote: > > nit: s/reopened/opened/ > > Done. Woops - reopened was correct. I didn't notice this was a "mediasource_test()". No worries. "opened" is also correct. Go ahead and land PS3 that's in CQ already.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/07/12 19:28:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Hmm. servolk@, ASAN test timeout might be due to crashing due to regression, I suppose. Can you try a local asan run (or at least try another CQ).
On 2016/07/12 19:34:15, wolenetz wrote: > On 2016/07/12 19:28:26, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Hmm. servolk@, ASAN test timeout might be due to crashing due to regression, I > suppose. Can you try a local asan run (or at least try another CQ). Looking at https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium..., this seems like a bot problem, possibly not a regression in this CL. Try CQ again.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/12 19:35:53, wolenetz wrote: > On 2016/07/12 19:34:15, wolenetz wrote: > > On 2016/07/12 19:28:26, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > Hmm. servolk@, ASAN test timeout might be due to crashing due to regression, I > > suppose. Can you try a local asan run (or at least try another CQ). > > Looking at > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium..., > this seems like a bot problem, possibly not a regression in this CL. Try CQ > again. Yeah, I already looked at linux_chromium_asan_rel_ng failures yesterday and they all seemed just infrastructure problems and not due to any regressions. The linux_chromium_asan_rel_ng trybot just seems to be in a bad state.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow multiple URL.createObjectURL invocations for MediaSource Currently calling URL.createObjectURL for a MediaSource more than once causes an assert/crash due to MediaSource::m_isAddedToRegistry flag being already set. But since calling createObjectURL is actually a valid thing to do and should just generate new unique URLs, let's replace the m_isAddedToRegistry flag with a counter. BUG=604425 ========== to ========== Allow multiple URL.createObjectURL invocations for MediaSource Currently calling URL.createObjectURL for a MediaSource more than once causes an assert/crash due to MediaSource::m_isAddedToRegistry flag being already set. But since calling createObjectURL is actually a valid thing to do and should just generate new unique URLs, let's replace the m_isAddedToRegistry flag with a counter. BUG=604425 Committed: https://crrev.com/014758724726a61d5a54df88e4fe993436a17442 Cr-Commit-Position: refs/heads/master@{#404879} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/014758724726a61d5a54df88e4fe993436a17442 Cr-Commit-Position: refs/heads/master@{#404879}
Message was sent while issue was closed.
CQ bit was unchecked. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
