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

Issue 1987643002: Make default media device ID salts random by default (Closed)

Created:
4 years, 7 months ago by Guido Urdaneta
Modified:
4 years, 6 months ago
CC:
anandc+watch-blimp_chromium.org, android-webview-reviews_chromium.org, Avi (use Gerrit), chromium-reviews, darin-cc_chromium.org, dtrainor+watch-blimp_chromium.org, feature-media-reviews_chromium.org, haibinlu, jessicag+watch-blimp_chromium.org, jochen+watch_chromium.org, khushalsagar+watch-blimp_chromium.org, Kevin M, kmarshall+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, mcasas+watch+vc_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, nyquist+watch-blimp_chromium.org, Peter Beverloo, shaktisahu+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, tommi (sloooow) - chröme, Torne, jochen (gone - plz use gerrit)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This results in different hashed device IDs on each session on embedders that don't have a specialized implementation of device ID salts such as WebView, Blimp and Content Shell. The new default helps prevent user fingerprinting on these embedders. Since the new default logic is basically the same as for Chrome incognito mode, Chrome's implementation of salts has been updated to defer to the new default on incognito mode. BUG=315022 Committed: https://crrev.com/4db1329e005388540eb07429ac97827ca9bc422b Cr-Commit-Position: refs/heads/master@{#399883}

Patch Set 1 #

Total comments: 4

Patch Set 2 : tommi's comment + minor change #

Patch Set 3 : reimplement using jam's proposed approach #

Patch Set 4 : remove obsolete comment #

Patch Set 5 : rebase + minor change #

Patch Set 6 : fix compile error #

Patch Set 7 : better fix for compile error #

Total comments: 2

Patch Set 8 : Replace ResourceContext::SaltCallback with std::string. #

Patch Set 9 : rebase #

Total comments: 8

Patch Set 10 : jam's comments #

Patch Set 11 : Fix threading error #

Patch Set 12 : change return type from const string to string, as it makes no difference #

Patch Set 13 : change return type from const string to string as it makes no difference #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -222 lines) Patch
M chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/media/media_device_id_salt.h View 1 2 2 chunks +1 line, -4 lines 0 comments Download
M chrome/browser/media/media_device_id_salt.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +11 lines, -30 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +10 lines, -6 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.cc View 1 2 3 4 5 6 7 4 chunks +10 lines, -12 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host_unittest.cc View 1 2 3 4 5 6 7 3 chunks +7 lines, -13 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host.cc View 1 2 3 4 5 6 7 4 chunks +9 lines, -9 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.h View 1 2 3 4 5 6 7 6 chunks +12 lines, -14 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 3 4 5 6 7 22 chunks +36 lines, -46 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager_unittest.cc View 1 2 3 4 5 6 7 5 chunks +13 lines, -21 lines 0 comments Download
M content/browser/resource_context_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +14 lines, -13 lines 0 comments Download
M content/public/browser/media_device_id.h View 1 2 3 4 5 6 7 1 chunk +7 lines, -8 lines 0 comments Download
M content/public/browser/media_device_id.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -9 lines 0 comments Download
M content/public/browser/resource_context.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -23 lines 0 comments Download

Messages

Total messages: 68 (28 generated)
Guido Urdaneta
tommi: this is approach 5, as discussed in crbug.com/315022
4 years, 7 months ago (2016-05-17 11:54:58 UTC) #3
tommi (sloooow) - chröme
lgtm. would like the strings to be const whenever possible. https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_resource_context.h File android_webview/browser/aw_resource_context.h (right): https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_resource_context.h#newcode39 ...
4 years, 7 months ago (2016-05-17 12:27:20 UTC) #4
Guido Urdaneta
https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_resource_context.h File android_webview/browser/aw_resource_context.h (right): https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_resource_context.h#newcode39 android_webview/browser/aw_resource_context.h:39: std::string media_device_id_salt_; On 2016/05/17 12:27:19, tommi-chrömium wrote: > can ...
4 years, 7 months ago (2016-05-18 09:40:19 UTC) #5
Guido Urdaneta
dtrainor@chromium.org: Please review changes in blimp/ torne@chromium.org: Please review changes in android_webview/ avi@chromium.org: Please review ...
4 years, 7 months ago (2016-05-18 09:50:04 UTC) #7
Guido Urdaneta
haibinlu@chromium.org: Please review changes in blimp/
4 years, 7 months ago (2016-05-18 09:57:07 UTC) #10
Guido Urdaneta
haibinlu@chromium.org: Please review changes in blimp/
4 years, 7 months ago (2016-05-18 09:58:53 UTC) #13
Torne
android_webview LGTM - thanks for the simpler approach here! :)
4 years, 7 months ago (2016-05-18 14:01:10 UTC) #14
Avi (use Gerrit)
John, do you know about ResourceContext? It's not clear to me why this complicated dance ...
4 years, 7 months ago (2016-05-18 15:15:16 UTC) #16
Guido Urdaneta
On 2016/05/18 15:15:16, Avi wrote: > John, do you know about ResourceContext? > > It's ...
4 years, 7 months ago (2016-05-18 15:34:39 UTC) #17
jam
On 2016/05/18 15:34:39, Guido Urdaneta wrote: > On 2016/05/18 15:15:16, Avi wrote: > > John, ...
4 years, 7 months ago (2016-05-18 16:19:22 UTC) #18
Kevin M
blimp lgtm
4 years, 7 months ago (2016-05-18 16:42:21 UTC) #20
Guido Urdaneta
On 2016/05/18 16:19:22, jam OOO wrote: > On 2016/05/18 15:34:39, Guido Urdaneta wrote: > > ...
4 years, 7 months ago (2016-05-18 18:32:43 UTC) #22
haibinlu
lgtm
4 years, 7 months ago (2016-05-18 20:18:27 UTC) #23
Guido Urdaneta
+jochen: can you take a look?
4 years, 6 months ago (2016-06-01 15:18:05 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/1987643002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987643002/80001
4 years, 6 months ago (2016-06-03 10:12:13 UTC) #27
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/147571) chromeos_x86-generic_chromium_compile_only_ng on ...
4 years, 6 months ago (2016-06-03 10:25:10 UTC) #29
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987643002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987643002/100001
4 years, 6 months ago (2016-06-03 11:17:53 UTC) #31
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/81496)
4 years, 6 months ago (2016-06-03 11:52:31 UTC) #33
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987643002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987643002/120001
4 years, 6 months ago (2016-06-03 12:06:00 UTC) #35
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h File content/public/browser/resource_context.h (right): https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h#newcode79 content/public/browser/resource_context.h:79: static std::string CreateRandomMediaDeviceIDSalt(); as jam@ pointed out, you don't ...
4 years, 6 months ago (2016-06-03 12:50:25 UTC) #38
Guido Urdaneta
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h File content/public/browser/resource_context.h (right): https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h#newcode79 content/public/browser/resource_context.h:79: static std::string CreateRandomMediaDeviceIDSalt(); On 2016/06/03 12:50:25, jochen wrote: > ...
4 years, 6 months ago (2016-06-03 13:09:44 UTC) #39
jochen (gone - plz use gerrit)
On 2016/06/03 at 13:09:44, guidou wrote: > https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h > File content/public/browser/resource_context.h (right): > > https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h#newcode79 ...
4 years, 6 months ago (2016-06-06 14:46:15 UTC) #40
jochen (gone - plz use gerrit)
On 2016/06/03 at 13:09:44, guidou wrote: > https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h > File content/public/browser/resource_context.h (right): > > https://codereview.chromium.org/1987643002/diff/140001/content/public/browser/resource_context.h#newcode79 ...
4 years, 6 months ago (2016-06-06 14:46:15 UTC) #41
Guido Urdaneta
> what about instantiating the media device id salt also for incognito mode? > shouldn't ...
4 years, 6 months ago (2016-06-08 09:52:03 UTC) #42
jochen (gone - plz use gerrit)
On 2016/06/08 at 09:52:03, guidou wrote: > > what about instantiating the media device id ...
4 years, 6 months ago (2016-06-08 15:04:16 UTC) #43
Guido Urdaneta
On 2016/06/08 15:04:16, jochen wrote: > On 2016/06/08 at 09:52:03, guidou wrote: > > > ...
4 years, 6 months ago (2016-06-08 16:14:08 UTC) #44
jam
On 2016/05/18 18:32:43, Guido Urdaneta wrote: > On 2016/05/18 16:19:22, jam OOO wrote: > > ...
4 years, 6 months ago (2016-06-09 15:52:29 UTC) #45
Guido Urdaneta
On 2016/06/09 15:52:29, jam wrote: > On 2016/05/18 18:32:43, Guido Urdaneta wrote: > > On ...
4 years, 6 months ago (2016-06-09 16:07:13 UTC) #46
jam
(sorry for delay, I missed the earlier comment) On 2016/06/09 16:07:13, Guido Urdaneta wrote: > ...
4 years, 6 months ago (2016-06-13 18:19:55 UTC) #47
Guido Urdaneta
> Let's do it here. There aren't that many places (only 11 hits total for ...
4 years, 6 months ago (2016-06-14 15:21:30 UTC) #49
jam
https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/media_device_id_salt.cc File chrome/browser/media/media_device_id_salt.cc (right): https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/media_device_id_salt.cc#newcode20 chrome/browser/media/media_device_id_salt.cc:20: if (media_device_id_salt_.GetValue().empty()) { i'm confused, the cl description says ...
4 years, 6 months ago (2016-06-14 16:20:33 UTC) #50
jam
lgtm Guido clarified over IM that this is for blimp/content_shell/webview and will update the description.
4 years, 6 months ago (2016-06-14 17:09:17 UTC) #51
Guido Urdaneta
https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/media_device_id_salt.cc File chrome/browser/media/media_device_id_salt.cc (right): https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/media_device_id_salt.cc#newcode20 chrome/browser/media/media_device_id_salt.cc:20: if (media_device_id_salt_.GetValue().empty()) { On 2016/06/14 16:20:33, jam wrote: > ...
4 years, 6 months ago (2016-06-14 17:30:57 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987643002/200001
4 years, 6 months ago (2016-06-14 17:32:24 UTC) #57
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/127090)
4 years, 6 months ago (2016-06-14 18:04:59 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987643002/260001
4 years, 6 months ago (2016-06-15 11:00:22 UTC) #62
commit-bot: I haz the power
Committed patchset #13 (id:260001)
4 years, 6 months ago (2016-06-15 11:26:52 UTC) #64
commit-bot: I haz the power
CQ bit was unchecked
4 years, 6 months ago (2016-06-15 11:26:58 UTC) #65
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/4db1329e005388540eb07429ac97827ca9bc422b Cr-Commit-Position: refs/heads/master@{#399883}
4 years, 6 months ago (2016-06-15 11:29:52 UTC) #67
Guido Urdaneta
4 years, 6 months ago (2016-06-15 12:54:57 UTC) #68
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/2065383003/ by guidou@chromium.org.

The reason for reverting is: Reverting because it is breaking the WebRTC
MacTester bot.
https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/55969/.

Powered by Google App Engine
This is Rietveld 408576698