Remove PeerConnectionIdentityStore and related messaging/storage code.

Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerConnection-generateCertificate-Promise-RTCCertificate--AlgorithmIdentifier-keygenAlgorithm

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcconfiguration-members

BUG=webrtc:5708, webrtc:5707
Committed: https://crrev.com/faf3baf7db0e031ffaacfa5cf18e0626ab1ed284
Cr-Commit-Position: refs/heads/master@{#403448}

[hbos_chromium, 2016-06-03 09:26:17]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

BUG=
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

Note:

- If identities have been stored in the user profile data, after this change
  that data will no longer be referenced or created, but it will no longer be
  deleted either if already present.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

[hbos_chromium, 2016-06-03 09:30:25]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

Note:

- If identities have been stored in the user profile data, after this change
  that data will no longer be referenced or created, but it will no longer be
  deleted either if already present.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

Note:

- After this change, identities stored in the user profile data will no longer
  be created or referenced, but they will also no longer be deleted if present.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

[hbos_chromium, 2016-06-03 09:30:25]

hbos@chromium.org changed reviewers:
+ jiayl@chromium.org

[hbos_chromium, 2016-06-03 09:33:09]

Please take a look jiayl, I'm removing a lot of code that you originally added.
It is no longer used, see description.

[jiayl, 2016-06-03 16:56:22]

lgtm

[hbos_chromium, 2016-06-04 11:10:38]

hbos@chromium.org changed reviewers:
+ dgozman@chromium.org, iclelland@chromium.org, jochen@chromium.org,
mkwst@chromium.org, tommi@chromium.org

[hbos_chromium, 2016-06-04 11:10:39]

Please take a look, y'all.

Ownerships:

chrome/browser/browsing_data/           mkwst
content/browser/background_sync/        iclelland
content/browser/devtools/protocol/      dgozman
content/browser/media/webrtc/           tommi
content/browser/renderer_host/media/    tommi
content/browser/renderer_host/          jochen (content owner)
content/browser/                        jochen (content owner)
content/common/                         mkwst (*message*)
content/common/media/                   mkwst (*message*), tommi
content/                                * (*.gypi), jochen
content/public/browser/                 jochen (content owner)
content/renderer/media/                 tommi
content/renderer/                       jochen (content owner)

[iclelland, 2016-06-04 12:14:54]

On 2016/06/04 11:10:39, hbos_chromium wrote:
> Please take a look, y'all.
> 
> Ownerships:
> 
> chrome/browser/browsing_data/           mkwst
> content/browser/background_sync/        iclelland
> content/browser/devtools/protocol/      dgozman
> content/browser/media/webrtc/           tommi
> content/browser/renderer_host/media/    tommi
> content/browser/renderer_host/          jochen (content owner)
> content/browser/                        jochen (content owner)
> content/common/                         mkwst (*message*)
> content/common/media/                   mkwst (*message*), tommi
> content/                                * (*.gypi), jochen
> content/public/browser/                 jochen (content owner)
> content/renderer/media/                 tommi
> content/renderer/                       jochen (content owner)

Background sync LGTM

[dgozman, 2016-06-04 14:24:54]

devtools lgtm with comment

https://codereview.chromium.org/2033353002/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/storage_handler.cc (left):

https://codereview.chromium.org/2033353002/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/storage_handler.cc:72: if
(set.count(kWebRTCIdentity))
Could you please remove it from protocol here as well:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

[hbos_chromium, 2016-06-05 11:40:51]

Please take a look, jochen, tommi and mkwst.

https://codereview.chromium.org/2033353002/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/storage_handler.cc (left):

https://codereview.chromium.org/2033353002/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/storage_handler.cc:72: if
(set.count(kWebRTCIdentity))
On 2016/06/04 14:24:53, dgozman_slow wrote:
> Could you please remove it from protocol here as well:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector...

Done.

[jochen (gone - plz use gerrit), 2016-06-06 15:02:00]

lgtm

[Mike West, 2016-06-09 14:01:31]

IPC LGTM.

I have some concerns about the browsing_data bits however. If we're removing the
ability to clear this data via chrome://settings/clearBrowserData and the
related `chrome.browsingData` extension APIs, I'd like to make sure that it's
really very benign. It's not clear to me from this CL that it doesn't leave
traces of a user's browsing on disk (e.g. `example.com => ID #12`), which a user
would reasonably expect to be cleared when they clear their browsing history.
I'd like the privacy team to sign off on this approach before running with it.

[hbos_chromium, 2016-06-21 13:48:26]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

Note:

- After this change, identities stored in the user profile data will no longer
  be created or referenced, but they will also no longer be deleted if present.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files for WebRTCIdentityStore if present.
  These files are no longer created or referenced, but a client upgrading might
  otherwise still have these files on disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

[hbos_chromium, 2016-06-21 13:54:15]

hbos@chromium.org changed reviewers:
+ battre@chromium.org

[hbos_chromium, 2016-06-21 13:54:16]

+battre for browser_prefs.cc and the privacy concern raised in #12.

As discussed over email, the identities generated were associated with URLs and
stored in the user profile. By deleting the SQL database files in the user
profile data if present on startup this should no longer be a concern.

[battre, 2016-06-21 15:59:41]

battre@chromium.org changed reviewers:
+ msramek@chromium.org

[battre, 2016-06-21 15:59:42]

I would like to delegate the privacy review to msramek@ as I won't find time for
this this week.

[msramek, 2016-06-21 16:41:01]

On 2016/06/21 13:54:16, hbos_chromium wrote:
> +battre for browser_prefs.cc and the privacy concern raised in #12.
> 
> As discussed over email, the identities generated were associated with URLs
and
> stored in the user profile. By deleting the SQL database files in the user
> profile data if present on startup this should no longer be a concern.

Is this referring to the entry in MigrateObsoleteProfilePrefs()? That looks fine
to me, this is a standard approach for disused sensitive prefs. Although
technically in your case this is not a pref, so it perhaps shouldn't live in
browser_prefs.cc but somewhere in your own codebase.

If the new rtc::RTCCertificateGenerator does not cache identities, or if it does
and it's already reachable from under REMOVE_CACHE in browsing_data_remover.cc,
then LGTM from privacy side.

[hbos_chromium, 2016-06-22 10:13:15]

On 2016/06/21 16:41:01, msramek wrote:
> On 2016/06/21 13:54:16, hbos_chromium wrote:
> > +battre for browser_prefs.cc and the privacy concern raised in #12.
> > 
> > As discussed over email, the identities generated were associated with URLs
> and
> > stored in the user profile. By deleting the SQL database files in the user
> > profile data if present on startup this should no longer be a concern.
> 
> Is this referring to the entry in MigrateObsoleteProfilePrefs()? That looks
fine

Yes.

> to me, this is a standard approach for disused sensitive prefs. Although
> technically in your case this is not a pref, so it perhaps shouldn't live in
> browser_prefs.cc but somewhere in your own codebase.

It is code that is meant for profile-related cleanup which is run every time the
browser starts so it does the job, but yeah, it's not technically not a pref.
It's a cache/db in the profile data.
Since I don't know where to put it, how about I create a TODO and file a bug?
This being such a large CL and all.

> If the new rtc::RTCCertificateGenerator does not cache identities, or if it
does
> and it's already reachable from under REMOVE_CACHE in
browsing_data_remover.cc,
> then LGTM from privacy side.

rtc::RTCCertificateGenerator does not cache identities (or care about URLs).

[msramek, 2016-06-22 10:46:11]

On 2016/06/22 10:13:15, hbos_chromium wrote:
> On 2016/06/21 16:41:01, msramek wrote:
> > On 2016/06/21 13:54:16, hbos_chromium wrote:
> > > +battre for browser_prefs.cc and the privacy concern raised in #12.
> > > 
> > > As discussed over email, the identities generated were associated with
URLs
> > and
> > > stored in the user profile. By deleting the SQL database files in the user
> > > profile data if present on startup this should no longer be a concern.
> > 
> > Is this referring to the entry in MigrateObsoleteProfilePrefs()? That looks
> fine
> 
> Yes.
> 
> > to me, this is a standard approach for disused sensitive prefs. Although
> > technically in your case this is not a pref, so it perhaps shouldn't live in
> > browser_prefs.cc but somewhere in your own codebase.
> 
> It is code that is meant for profile-related cleanup which is run every time
the
> browser starts so it does the job, but yeah, it's not technically not a pref.
> It's a cache/db in the profile data.
> Since I don't know where to put it, how about I create a TODO and file a bug?
> This being such a large CL and all.

My idea was to call it from a constructor of some major class in your (WebRTC)
codebase that also always gets instantiated on Profile startup, if any such
class exists. We have this kind of cleanup methods at various places in
Chromium, MigrateObsoleteProfilePrefs() is just one of them, so it's strange to
pick exactly this one.

That said, from privacy perspective, I just want it done :) I don't own
browser_prefs.cc, so if the code owners don't complain, TODO is fine with me.

> 
> > If the new rtc::RTCCertificateGenerator does not cache identities, or if it
> does
> > and it's already reachable from under REMOVE_CACHE in
> browsing_data_remover.cc,
> > then LGTM from privacy side.
> 
> rtc::RTCCertificateGenerator does not cache identities (or care about URLs).

[hbos_chromium, 2016-06-30 12:16:08]

I seem to have forgotten about this CL for the past week.

- battre, as an owner of browser_prefs.cc, do you have time to take a look now
or should I find another owner? (Privacy LGTM, but question if OK to place here
with a TODO or if it should be moved.)

- tommi, can you take a look? (perkj, other owner, is in london this week)

On 2016/06/22 10:46:11, msramek wrote:
> My idea was to call it from a constructor of some major class in your (WebRTC)
> codebase that also always gets instantiated on Profile startup, if any such
> class exists. We have this kind of cleanup methods at various places in
> Chromium, MigrateObsoleteProfilePrefs() is just one of them, so it's strange
to
> pick exactly this one.
> 
> That said, from privacy perspective, I just want it done :) I don't own
> browser_prefs.cc, so if the code owners don't complain, TODO is fine with me.

[battre, 2016-06-30 12:38:32]

https://codereview.chromium.org/2033353002/diff/80001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2033353002/diff/80001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:717: false);
I don't think that you can call base::DeleteFile on the UI thread. I think that
you want to post a delayed task to the File thread.

[hbos_chromium, 2016-06-30 15:28:17]

PTAL battre.

https://codereview.chromium.org/2033353002/diff/80001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2033353002/diff/80001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:717: false);
On 2016/06/30 12:38:32, battre wrote:
> I don't think that you can call base::DeleteFile on the UI thread. I think
that
> you want to post a delayed task to the File thread.

Done.

[battre, 2016-07-01 10:43:45]

LGTM with one comment

https://codereview.chromium.org/2033353002/diff/100001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2033353002/diff/100001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:305: content::BrowserThread::PostTask(
I would suggest to use PostDelayedTask with a delay of 120 seconds so that you
don't block the File thread during startup where a lot of stuff needs to be
read.

[hbos_chromium, 2016-07-01 12:15:35]

PTAL tommi

https://codereview.chromium.org/2033353002/diff/100001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2033353002/diff/100001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:305: content::BrowserThread::PostTask(
On 2016/07/01 10:43:45, battre wrote:
> I would suggest to use PostDelayedTask with a delay of 120 seconds so that you
> don't block the File thread during startup where a lot of stuff needs to be
> read.

That makes sense! Done.

[tommi (sloooow) - chröme, 2016-07-01 12:16:45]

lgtm

[hbos_chromium, 2016-07-01 12:20:55]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-07-01 12:20:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, jiayl@chromium.org, dgozman@chromium.org,
mkwst@chromium.org, jochen@chromium.org, msramek@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/2033353002/#ps200001
(title: "Delayed deletion by 120s")

[commit-bot: I haz the power, 2016-07-01 12:21:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-01 13:37:27]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files for WebRTCIdentityStore if present.
  These files are no longer created or referenced, but a client upgrading might
  otherwise still have these files on disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

[commit-bot: I haz the power, 2016-07-01 13:43:34]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

[commit-bot: I haz the power, 2016-07-01 13:43:36]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-01 13:43:39]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 13:45:06]

Description was changed from

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707
==========

to

==========
Remove PeerConnectionIdentityStore and related messaging/storage code.

Motivation and Background:

This is the Chromium implementation of WebRTC's DtlsIdentityStoreInterface,
which allows generating/requesting identities/certificates to be used for the
DTLS handshake by a peer connection.

Originally, RSA-1024 was the only ciphersuite used and this store was optimized
for this by preemptively generating and caching. After certificate generation
was parameterized[1], RSA-2048 support mandated by spec and ECDSA P-256 being
the default certificate type[2], WebRTC crypto code[3] was updated to be
parameterized.

Due to the parameterization we wanted to move over to use the fully
parameterized WebRTC crypto code and stop maintaining two crypto paths. But
during the transition, when RSA-1024 was still the default, we wanted to take
advantage of the performance benefits of the PeerConnectionIdentityStore. The
compromise was an "if request to generate RSA-1024 use this code, else use
WebRTC crypto code". After having switched the default to ECDSA, performance was
no longer an issue and PeerConnectionIdentityStore was no longer used. (Note
that applications insisting on using RSA-1024 (not recommended, weaker) by
explicitly generating it with RTCPeerConnection.generateCertificate have the
ability to reuse certificates with persistent storage in IndexedDB[4] for
performance instead. For RSA-2048 this is what you'd have to do anyway, or
generate every time.)

After this change we will be able to remove DtlsIdentityStoreInterface in favor
of rtc::RTCCertificateGeneratorInterface, rtc::RTCCertificateGenerator being the
implementation used by default.

Changes:

- Remove PeerConnectionIdentityStore and classes specifically related to its
  functionality. Remove related unittests and update gypi files.

- Update code to stop using WebRTCIdentityStore, WebRTCIdentityServiceHost and
  WebRTCIdentityService.

- On startup, delete the SQL database files (after a 120s delay) for
  WebRTCIdentityStore if present. These files are no longer created or
  referenced, but a client upgrading might otherwise still have these files on
  disk since earlier versions of Chrome.

[1]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#widl-RTCPeerCon...

[2] https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#sec.cert-mgmt

[3] By "WebRTC crypto code" I mean SSLIdentity::Generate and new API relying on
it, rtc::RTCCertificateGenerator / rtc::RTCCertificate.

[4]
https://w3c.github.io/webrtc-pc/archives/20160125/webrtc.html#dictionary-rtcc...

BUG=webrtc:5708, webrtc:5707

Committed: https://crrev.com/faf3baf7db0e031ffaacfa5cf18e0626ab1ed284
Cr-Commit-Position: refs/heads/master@{#403448}
==========

[commit-bot: I haz the power, 2016-07-01 13:45:08]

Patchset 11 (id:??) landed as
https://crrev.com/faf3baf7db0e031ffaacfa5cf18e0626ab1ed284
Cr-Commit-Position: refs/heads/master@{#403448}