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

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:
2 years, 3 months ago by no longer working on chromium
Modified:
2 years, 3 months ago
CC:
chromium-reviews, 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) 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 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 Download
D content/browser/renderer_host/media/audio_input_device_manager_event_handler.h View 1 chunk +0 lines, -33 lines 0 comments 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 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 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 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 Download
M content/common/media/audio_messages.h View 1 2 4 chunks +1 line, -13 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/media/audio_input_message_filter.h View 1 2 2 chunks +1 line, -5 lines 0 comments Download
M content/renderer/media/audio_input_message_filter.cc View 1 2 4 chunks +3 lines, -20 lines 0 comments 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 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 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 Download
M content/renderer/media/webaudio_capturer_source.h View 1 chunk +1 line, -2 lines 0 comments Download
M content/renderer/media/webaudio_capturer_source.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/webrtc_audio_capturer.h View 1 2 3 4 5 4 chunks +10 lines, -18 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer.cc View 1 2 3 4 8 chunks +9 lines, -20 lines 0 comments Download
M content/renderer/media/webrtc_audio_device_unittest.cc View 1 2 3 1 chunk +1 line, -3 lines 0 comments Download
M content/renderer/pepper/pepper_platform_audio_input_impl.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/pepper/pepper_platform_audio_input_impl.cc View 1 2 3 4 3 chunks +9 lines, -39 lines 0 comments Download
M media/audio/audio_input_device.h View 1 2 6 chunks +2 lines, -18 lines 0 comments Download
M media/audio/audio_input_device.cc View 1 2 6 chunks +10 lines, -56 lines 0 comments Download
M media/audio/audio_input_ipc.h View 1 2 2 chunks +5 lines, -15 lines 0 comments Download
M media/base/audio_capturer_source.h View 1 2 2 chunks +5 lines, -19 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 34 (0 generated)
no longer working on chromium
Hi, this patch is mainly removing code and fixed the security concern from the device_id ...
2 years, 3 months ago (2013-03-11 21:57:10 UTC) #1
miu
On 2013/03/11 21:57:10, xians1 wrote: > Hi, this patch is mainly removing code and fixed ...
2 years, 3 months ago (2013-03-12 07:04:52 UTC) #2
no longer working on chromium
Hi Reviewers, this CL is mostly cleaning up and removing code, and it also fixes ...
2 years, 3 months ago (2013-03-12 10:43:25 UTC) #3
Chris Rogers
On 2013/03/12 10:43:25, xians1 wrote: > Hi Reviewers, this CL is mostly cleaning up and ...
2 years, 3 months ago (2013-03-12 17:17:44 UTC) #4
Chris Rogers
Thinking more about this, it doesn't seem like the right thing to do is use ...
2 years, 3 months ago (2013-03-12 17:37:02 UTC) #5
palmer
Thanks for doing this! It's not every day you get to remove more lines than ...
2 years, 3 months ago (2013-03-12 17:45:11 UTC) #6
no longer working on chromium
Hi Chris, We are using AudioInputDeviceManager::kFakeOpenSessionId (which is 1) for clients to open the stream ...
2 years, 3 months ago (2013-03-12 17:58:31 UTC) #7
Chris Rogers
Shijing, we just need a unique identifier (one for each device) which is an integer ...
2 years, 3 months ago (2013-03-12 19:52:22 UTC) #8
no longer working on chromium
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 ...
2 years, 3 months ago (2013-03-14 10:47:32 UTC) #9
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 ...
2 years, 3 months ago (2013-03-14 21:05:14 UTC) #10
DaleCurtis
moving myself to cc, this all looks good to me, but Yuri and Chris are ...
2 years, 3 months ago (2013-03-14 21:53:20 UTC) #11
miu
Hi Shijing, Just a friendly notice: Due to scheduling conflicts on my end, I'll probably ...
2 years, 3 months ago (2013-03-15 09:50:26 UTC) #12
no longer working on chromium
Yuri, it is fine for you to do it when you have time. Palmer, another ...
2 years, 3 months ago (2013-03-15 14:52:11 UTC) #13
jam
On 2013/03/15 14:52:11, xians1 wrote: > Yuri, it is fine for you to do it ...
2 years, 3 months ago (2013-03-15 16:11:12 UTC) #14
palmer
IPC security LGTM
2 years, 3 months ago (2013-03-15 18:08:14 UTC) #15
yzshen1
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 ...
2 years, 3 months ago (2013-03-15 18:21:02 UTC) #16
no longer working on chromium
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 ...
2 years, 3 months ago (2013-03-15 19:16:48 UTC) #17
yzshen1
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 ...
2 years, 3 months ago (2013-03-15 21:47:47 UTC) #18
no longer working on chromium
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 ...
2 years, 3 months ago (2013-03-15 21:57:43 UTC) #19
yzshen1
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 ...
2 years, 3 months ago (2013-03-15 22:06:04 UTC) #20
no longer working on chromium
yzshen, FYI, I have already changed the code to use OpenDevice when the device_id.empty() is ...
2 years, 3 months ago (2013-03-18 14:18:29 UTC) #21
yzshen1
Thanks! Still LGTM
2 years, 3 months ago (2013-03-18 16:55:31 UTC) #22
miu
lgtm Great work! It's very nice to see a ton of complexity removed and simplified. ...
2 years, 3 months ago (2013-03-18 23:19:05 UTC) #23
no longer working on chromium
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: > ...
2 years, 3 months ago (2013-03-19 14:06:49 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
2 years, 3 months ago (2013-03-19 14:07:14 UTC) #25
commit-bot: I haz the power
Presubmit check for 12440027-46001 failed and returned exit status 1. INFO:root:Found 22 file(s). Running presubmit ...
2 years, 3 months ago (2013-03-19 14:07:27 UTC) #26
no longer working on chromium
Dale, can I have your owner stamp for media/base/audio_capturer_source.h? Thanks, SX
2 years, 3 months ago (2013-03-19 14:32:09 UTC) #27
DaleCurtis
lgtm
2 years, 3 months ago (2013-03-19 18:10:15 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/46001
2 years, 3 months ago (2013-03-19 18:21:49 UTC) #29
perkj_ooo_until_september
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 ...
2 years, 3 months ago (2013-03-20 10:39:52 UTC) #30
no longer working on chromium
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 ...
2 years, 3 months ago (2013-03-20 15:10:19 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/12440027/73002
2 years, 3 months ago (2013-03-20 15:36:47 UTC) #32
perkj_ooo_until_september
MediastreamImpl and MediaStreamDependencyFactory lgtm.
2 years, 3 months ago (2013-03-20 15:46:14 UTC) #33
commit-bot: I haz the power
2 years, 3 months ago (2013-03-20 18:01:52 UTC) #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 1f9106d