Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075193002:
Fixes use-after-free in MojoDemuxerStreamImpl. (Closed)
|
Created:
4 years, 6 months ago by alokp Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, qsr+mojo_chromium.org, sandersd (OOO until July 31), viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, zhengxiong1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes use-after-free in MojoDemuxerStreamImpl.
BUG=621141
Committed: https://crrev.com/61fdb768d7395fbb73daff12d9b38100df6a044f
Cr-Commit-Position: refs/heads/master@{#400615}
Patch Set 1 #
Total comments: 3
Patch Set 2 : addressed comments #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : addressed comments #
Total comments: 2
Messages
Total messages: 20 (5 generated)
alokp@chromium.org changed reviewers: + xhwang@chromium.org
The change looks good with one comment. Do you feel it's easy to add a unittest? The MojoRendererTest CL is in CQ: https://chromiumcodereview.appspot.com/2070753002/ https://chromiumcodereview.appspot.com/2075193002/diff/1/media/mojo/clients/m... File media/mojo/clients/mojo_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2075193002/diff/1/media/mojo/clients/m... media/mojo/clients/mojo_renderer_impl.cc:67: } In theory, if the remote renderer doesn't care about either audio or video, it can simply drop the interface request, causing a connection error on audio_stream_ or video_stream_. In this case we can delete audio_stream_ or video_stream_ to save some resource. But this will break HasAudio() and HasVideo() in this CL.
BTW, as always, thank you so much for the bug and fix!
https://chromiumcodereview.appspot.com/2075193002/diff/1/media/mojo/clients/m... File media/mojo/clients/mojo_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2075193002/diff/1/media/mojo/clients/m... media/mojo/clients/mojo_renderer_impl.cc:67: } On 2016/06/17 18:47:00, xhwang wrote: > In theory, if the remote renderer doesn't care about either audio or video, it > can simply drop the interface request, causing a connection error on > audio_stream_ or video_stream_. In this case we can delete audio_stream_ or > video_stream_ to save some resource. But this will break HasAudio() and > HasVideo() in this CL. If we decide to support this use case it would be more efficient to not create audio/video demuxer streams to start with. I would change mojom::Renderer::Initialize from: Initialize(RendererClient, DemuxerStream?, DemuxerStream?) => (bool success); to: Initialize(RendererClient, AudioDecoderConfig?, VideoDecoderConfig?) => ( bool success, handle<data_pipe_producer>? audio_pipe, handle<data_pipe_producer>? video_pipe); That way Service can create whatever stream it is interested in rendering instead of client unconditionally creating all streams. In addition it also reduces one roundtrip currently required to initialize DemuxerStream. It might also be efficient for mojo because IIRC creating a data pipe from a sandboxed process requires a SYNC IPC? The new API will move the creation of data pipe to privileged process. WDYT?
+zhengxiong who has complained (to me) about multiple IPC calls currently required to setup the media service.
https://codereview.chromium.org/2075193002/diff/1/media/mojo/clients/mojo_ren... File media/mojo/clients/mojo_renderer_impl.cc (right): https://codereview.chromium.org/2075193002/diff/1/media/mojo/clients/mojo_ren... media/mojo/clients/mojo_renderer_impl.cc:67: } On 2016/06/17 20:16:37, alokp wrote: > On 2016/06/17 18:47:00, xhwang wrote: > > In theory, if the remote renderer doesn't care about either audio or video, it > > can simply drop the interface request, causing a connection error on > > audio_stream_ or video_stream_. In this case we can delete audio_stream_ or > > video_stream_ to save some resource. But this will break HasAudio() and > > HasVideo() in this CL. > > If we decide to support this use case it would be more efficient to not create > audio/video demuxer streams to start with. I would change > mojom::Renderer::Initialize from: > Initialize(RendererClient, DemuxerStream?, DemuxerStream?) => (bool success); > > to: > Initialize(RendererClient, AudioDecoderConfig?, VideoDecoderConfig?) => ( > bool success, > handle<data_pipe_producer>? audio_pipe, > handle<data_pipe_producer>? video_pipe); > > That way Service can create whatever stream it is interested in rendering > instead of client unconditionally creating all streams. In addition it also > reduces one roundtrip currently required to initialize DemuxerStream. It might > also be efficient for mojo because IIRC creating a data pipe from a sandboxed > process requires a SYNC IPC? The new API will move the creation of data pipe to > privileged process. > > WDYT? Whoops I did not think this through. You would still need to create a mojom::DemuxerStream and pass its proxy to the service somehow. Let me incorporate your suggestions in the next patch.
PTAL
Please add a TODO somewhere to add unittests for this ( MojoDemuxerStreamImpl and MojoRendererImpl). Otherwise LGTM. Thanks for the fix! https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... File media/mojo/clients/mojo_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... media/mojo/clients/mojo_renderer_impl.cc:64: base::Unretained(this), DemuxerStream::AUDIO)); please add a comment about why Unretained() is safe. https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... media/mojo/clients/mojo_renderer_impl.cc:225: } Just to be safe, } else if (type == VIDEO) { ... } else { NOTREACHED() << "wrong type.."; }
https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... File media/mojo/clients/mojo_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... media/mojo/clients/mojo_renderer_impl.cc:64: base::Unretained(this), DemuxerStream::AUDIO)); On 2016/06/18 16:46:16, xhwang wrote: > please add a comment about why Unretained() is safe. Done. https://chromiumcodereview.appspot.com/2075193002/diff/20001/media/mojo/clien... media/mojo/clients/mojo_renderer_impl.cc:225: } On 2016/06/18 16:46:16, xhwang wrote: > Just to be safe, > > } else if (type == VIDEO) { > ... > } else { > NOTREACHED() << "wrong type.."; > } Done.
The CQ bit was checked by alokp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2075193002/#ps60001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075193002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fixes use-after-free in MojoDemuxerStreamImpl. BUG=621141 ========== to ========== Fixes use-after-free in MojoDemuxerStreamImpl. BUG=621141 Committed: https://crrev.com/61fdb768d7395fbb73daff12d9b38100df6a044f Cr-Commit-Position: refs/heads/master@{#400615} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/61fdb768d7395fbb73daff12d9b38100df6a044f Cr-Commit-Position: refs/heads/master@{#400615}
Message was sent while issue was closed.
rockot@chromium.org changed reviewers: + rockot@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... File media/mojo/clients/mojo_demuxer_stream_impl.h (right): https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... media/mojo/clients/mojo_demuxer_stream_impl.h:45: mojo::Binding<mojom::DemuxerStream> binding_; Some tests now leak a MojoDemxuerStreamImpl: https://cs.chromium.org/chromium/src/media/mojo/services/media_mojo_unittest....
Message was sent while issue was closed.
https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... File media/mojo/clients/mojo_demuxer_stream_impl.h (right): https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... media/mojo/clients/mojo_demuxer_stream_impl.h:45: mojo::Binding<mojom::DemuxerStream> binding_; On 2016/06/19 14:45:22, Ken Rockot wrote: > Some tests now leak a MojoDemxuerStreamImpl: > https://cs.chromium.org/chromium/src/media/mojo/services/media_mojo_unittest.... Memory leak fix here: https://codereview.chromium.org/2087473002/ Out of curiosity, how did you discover the memory leak? Did it trigger any memory bot failure? I do not see any failure on the bots.
Message was sent while issue was closed.
On 2016/06/20 at 16:44:48, alokp wrote: > https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... > File media/mojo/clients/mojo_demuxer_stream_impl.h (right): > > https://codereview.chromium.org/2075193002/diff/60001/media/mojo/clients/mojo... > media/mojo/clients/mojo_demuxer_stream_impl.h:45: mojo::Binding<mojom::DemuxerStream> binding_; > On 2016/06/19 14:45:22, Ken Rockot wrote: > > Some tests now leak a MojoDemxuerStreamImpl: > > https://cs.chromium.org/chromium/src/media/mojo/services/media_mojo_unittest.... > > Memory leak fix here: https://codereview.chromium.org/2087473002/ > > Out of curiosity, how did you discover the memory leak? Did it trigger any memory bot failure? I do not see any failure on the bots. I was landing the mojo::Callback CL and had to update MojoDemuxerStreamImpl since it raced with your CL. I just noticed it after looking at the code, not from specific failures. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
