Chromium Code Reviews
Help | Chromium Project | Sign in
(581)

Issue 12440027: Do not pass the string device_id via IPC message to create an audio input stream (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by xians1
Modified:
1 year, 1 month ago
CC:
chromium-reviews_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, DaleCurtis
Visibility:
Public.

Description

Do not pass the string device_id via IPC message to create an audio input stream.
Using a string device_id via IPC message from the render client to open the device is not very safe since IPC message is not trusted. Instead, we should just pass an int (session_id) to the browser, and we do the lookup on AudioInputDeviceManager in the browser to re-map the session_id to the device_id.

BUG=179597, 216952
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189342

Patch Set 1 #

Patch Set 2 : ready for review #

Total comments: 18

Patch Set 3 : addressed palmer's comments #

Total comments: 6

Patch Set 4 : fixed the nits. #

Total comments: 9

Patch Set 5 : changed the pepper code to use OpenDevice() for default device, and fixed a minor mistake in MediaS… #

Total comments: 8

Patch Set 6 : addressed miu's comments. #

Patch Set 7 : fixed the tab capture apitests #

Total comments: 6

Patch Set 8 : addressed Per's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+217 lines, -722 lines) Lint Patch
M content/browser/renderer_host/media/audio_input_device_manager.h View 1 2 3 4 5 5 chunks +18 lines, -21 lines 0 comments 1 errors Download
M content/browser/renderer_host/media/audio_input_device_manager.cc View 1 2 3 4 5 7 chunks +49 lines, -81 lines 0 comments 0 errors Download
D content/browser/renderer_host/media/audio_input_device_manager_event_handler.h View 1 chunk +0 lines, -33 lines 0 comments 0 errors Download
M content/browser/renderer_host/media/audio_input_device_manager_unittest.cc View 1 2 3 4 5 9 chunks +24 lines, -210 lines 0 comments 0 errors Download
M content/browser/renderer_host/media/audio_input_renderer_host.h View 1 2 3 4 5 8 chunks +7 lines, -49 lines 0 comments 0 errors Download
M content/browser/renderer_host/media/audio_input_renderer_host.cc View 1 2 3 4 5 7 chunks +18 lines, -79 lines 0 comments 0 errors Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 3 4 5 6 7 1 chunk +12 lines, -1 line 0 comments ? errors Download
M content/common/media/audio_messages.h View 1 2 4 chunks +1 line, -13 lines 0 comments 0 errors Download
M content/content_browser.gypi View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments ? errors Download
M content/renderer/media/audio_input_message_filter.h View 1 2 2 chunks +1 line, -5 lines 0 comments 0 errors Download
M content/renderer/media/audio_input_message_filter.cc View 1 2 4 chunks +3 lines, -20 lines 0 comments 1 errors Download
M content/renderer/media/media_stream_dependency_factory.h View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments ? errors Download
M content/renderer/media/media_stream_dependency_factory.cc View 1 2 3 4 5 6 7 3 chunks +22 lines, -11 lines 0 comments ? errors Download
M content/renderer/media/media_stream_impl.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -6 lines 0 comments ? errors Download
M content/renderer/media/webaudio_capturer_source.h View 1 chunk +1 line, -2 lines 0 comments 0 errors Download
M content/renderer/media/webaudio_capturer_source.cc View 1 chunk +1 line, -1 line 0 comments 0 errors Download
M content/renderer/media/webrtc_audio_capturer.h View 1 2 3 4 5 4 chunks +10 lines, -18 lines 0 comments 0 errors Download
M content/renderer/media/webrtc_audio_capturer.cc View 1 2 3 4 8 chunks +9 lines, -20 lines 0 comments 0 errors Download
M content/renderer/media/webrtc_audio_device_unittest.cc View 1 2 3 1 chunk +1 line, -3 lines 0 comments 0 errors Download
M content/renderer/pepper/pepper_platform_audio_input_impl.h View 1 2 1 chunk +0 lines, -1 line 0 comments 0 errors Download
M content/renderer/pepper/pepper_platform_audio_input_impl.cc View 1 2 3 4 3 chunks +9 lines, -39 lines 0 comments 0 errors Download
M media/audio/audio_input_device.h View 1 2 6 chunks +2 lines, -18 lines 0 comments ? errors Download
M media/audio/audio_input_device.cc View 1 2 6 chunks +10 lines, -56 lines 0 comments ? errors Download
M media/audio/audio_input_ipc.h View 1 2 2 chunks +5 lines, -15 lines 0 comments ? errors Download
M media/base/audio_capturer_source.h View 1 2 2 chunks +5 lines, -19 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 34
xians1
Hi, this patch is mainly removing code and fixed the security concern from the device_id ...
1 year, 1 month ago #1
miu
On 2013/03/11 21:57:10, xians1 wrote: > Hi, this patch is mainly removing code and fixed ...
1 year, 1 month ago #2
xians1
Hi Reviewers, this CL is mostly cleaning up and removing code, and it also fixes ...
1 year, 1 month ago #3
Chris Rogers
On 2013/03/12 10:43:25, xians1 wrote: > Hi Reviewers, this CL is mostly cleaning up and ...
1 year, 1 month ago #4
Chris Rogers
Thinking more about this, it doesn't seem like the right thing to do is use ...
1 year, 1 month ago #5
Chromium Palmer
Thanks for doing this! It's not every day you get to remove more lines than ...
1 year, 1 month ago #6
xians1
Hi Chris, We are using AudioInputDeviceManager::kFakeOpenSessionId (which is 1) for clients to open the stream ...
1 year, 1 month ago #7
Chris Rogers
Shijing, we just need a unique identifier (one for each device) which is an integer ...
1 year, 1 month ago #8
xians1
Hi reviewers, please take another look. SX https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_host/media/audio_input_device_manager.cc File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/4001/content/browser/renderer_host/media/audio_input_device_manager.cc#newcode40 content/browser/renderer_host/media/audio_input_device_manager.cc:40: StreamDeviceMap::iterator device ...
1 year, 1 month ago #9
Chromium Palmer
https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_host/media/audio_input_device_manager.cc File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/12440027/diff/15028/content/browser/renderer_host/media/audio_input_device_manager.cc#newcode200 content/browser/renderer_host/media/audio_input_device_manager.cc:200: for (StreamDeviceList::iterator i(devices_.begin()); i != devices_.end(); Why do a ...
1 year, 1 month ago #10
DaleCurtis
moving myself to cc, this all looks good to me, but Yuri and Chris are ...
1 year, 1 month ago #11
miu
Hi Shijing, Just a friendly notice: Due to scheduling conflicts on my end, I'll probably ...
1 year, 1 month ago #12
xians1
Yuri, it is fine for you to do it when you have time. Palmer, another ...
1 year, 1 month ago #13
jam
On 2013/03/15 14:52:11, xians1 wrote: > Yuri, it is fine for you to do it ...
1 year, 1 month ago #14
Chromium Palmer
IPC security LGTM
1 year, 1 month ago #15
yzshen1 (OOO until Apr 21)
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode156 content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). What do you mean by "Pepper completely ...
1 year, 1 month ago #16
xians1
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode156 content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). On 2013/03/15 18:21:03, yzshen1 wrote: > What ...
1 year, 1 month ago #17
yzshen1 (OOO until Apr 21)
LGTM Thanks! https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode156 content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). Just out of curiosity: Do ...
1 year, 1 month ago #18
xians1
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode156 content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). On 2013/03/15 21:47:48, yzshen1 wrote: > Just ...
1 year, 1 month ago #19
yzshen1 (OOO until Apr 21)
https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right): https://codereview.chromium.org/12440027/diff/12004/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode156 content/renderer/pepper/pepper_platform_audio_input_impl.cc:156: // OpenDevice(). Either way. It sounds good if you ...
1 year, 1 month ago #20
xians1
yzshen, FYI, I have already changed the code to use OpenDevice when the device_id.empty() is ...
1 year, 1 month ago #21
yzshen1 (OOO until Apr 21)
Thanks! Still LGTM
1 year, 1 month ago #22
miu
lgtm Great work! It's very nice to see a ton of complexity removed and simplified. ...
1 year, 1 month ago #23
xians1
https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_host/media/audio_input_device_manager.h File content/browser/renderer_host/media/audio_input_device_manager.h (right): https://codereview.chromium.org/12440027/diff/37001/content/browser/renderer_host/media/audio_input_device_manager.h#newcode43 content/browser/renderer_host/media/audio_input_device_manager.h:43: StreamDeviceInfo GetOpenedDeviceInfoById(int session_id); On 2013/03/18 23:19:05, Yuri wrote: > ...
1 year, 1 month ago #24
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
1 year, 1 month ago #25
I haz the power (commit-bot)
Presubmit check for 12440027-46001 failed and returned exit status 1. INFO:root:Found 22 file(s). Running presubmit ...
1 year, 1 month ago #26
xians1
Dale, can I have your owner stamp for media/base/audio_capturer_source.h? Thanks, SX
1 year, 1 month ago #27
DaleCurtis
lgtm
1 year, 1 month ago #28
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
1 year, 1 month ago #29
perkj
https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/media_stream_impl.cc File content/renderer/media/media_stream_impl.cc (right): https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/media_stream_impl.cc#newcode498 content/renderer/media/media_stream_impl.cc:498: MediaStreamExtraData* extra_data = static_cast<MediaStreamExtraData*>( Looks like all of this ...
1 year, 1 month ago #30
xians1
Per, could you please take another look at the media_stream_impl and media_stream_dependency_factory? Thanks, SX https://codereview.chromium.org/12440027/diff/65001/content/renderer/media/media_stream_impl.cc ...
1 year, 1 month ago #31
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/73002
1 year, 1 month ago #32
perkj
MediastreamImpl and MediaStreamDependencyFactory lgtm.
1 year, 1 month ago #33
I haz the power (commit-bot)
1 year, 1 month ago #34
Message was sent while issue was closed.
Change committed as 189342
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6