content::RTCCertificateGenerator and WebRTC-EnableWebRtcEcdsa update.

content::RTCCertificateGenerator and WebRTC-EnableWebRtcEcdsa update.

The flag WebRTC-EnableWebRtcEcdsa no longer makes assumptions about which type
the default certificate is. Compares with KT_DEFAULT instead; if flag is on ECDSA
is used, if off RSA is used. Previously assumed RSA was the default.
- This is preparation for modifying the value of KT_DEFAULT without messing up
  the WebRTC-EnableWebRtcEcdsa experiment.

Synchronous version of content::RTCCerticicateGenerator's certificate generation
method added: generateCertificateAndWait. This is used in place of
SSLIdentity::Generate - or its replacement
rtc::RTCCertificateGenerator::GenerateCertificate - because using the Chromium
crypto path (PeerConnectionIdentityStore) still has performance benefits for
RSA-1024. We will stop taking advantage of this crypto path as soon as we remove
WebRTC-EnableWebRtcEcdsa and there is no risk of RSA-1024 being the default.

Using generateCertificateAndWait in rtc_peer_connection_handler.cc and removed
old code using SSLIdentity::Generate in its place.

BUG=601850

[hbos_chromium, 2016-05-12 11:41:04]

Description was changed from

==========
RTCCertificateGenerator and WebRTC-EnableWebRtcEcdsa update.

The flag WebRTC-EnableWebRtcEcdsa no longer makes assumptions about which type
the default certificate is. Compares with KT_DEFAULT instead; if flag is on
ECDSA
is used, if off RSA is used. Previously assumed RSA was the default.
- This is preparation for modifying the value of KT_DEFAULT without messing up
  the WebRTC-EnableWebRtcEcdsa experiment.

Synchronous version of content::RTCCerticicateGenerator's certificate generation
method added: generateCertificateAndWait. This is used in place of
SSLIdentity::Generate - or its replacement
rtc::RTCCertificateGenerator::GenerateCertificate - because using the Chromium
crypto path (PeerConnectionIdentityStore) still has performance benefits for
RSA-1024. We will stop taking advantage of this crypto path as soon as we remove
WebRTC-EnableWebRtcEcdsa and there is no risk of RSA-1024 being the default.

Using generateCertificateAndWait in rtc_peer_connection_handler.cc and removed
old code using SSLIdentity::Generate in its place.

BUG=601850
==========

to

==========
content::RTCCertificateGenerator and WebRTC-EnableWebRtcEcdsa update.

The flag WebRTC-EnableWebRtcEcdsa no longer makes assumptions about which type
the default certificate is. Compares with KT_DEFAULT instead; if flag is on
ECDSA
is used, if off RSA is used. Previously assumed RSA was the default.
- This is preparation for modifying the value of KT_DEFAULT without messing up
  the WebRTC-EnableWebRtcEcdsa experiment.

Synchronous version of content::RTCCerticicateGenerator's certificate generation
method added: generateCertificateAndWait. This is used in place of
SSLIdentity::Generate - or its replacement
rtc::RTCCertificateGenerator::GenerateCertificate - because using the Chromium
crypto path (PeerConnectionIdentityStore) still has performance benefits for
RSA-1024. We will stop taking advantage of this crypto path as soon as we remove
WebRTC-EnableWebRtcEcdsa and there is no risk of RSA-1024 being the default.

Using generateCertificateAndWait in rtc_peer_connection_handler.cc and removed
old code using SSLIdentity::Generate in its place.

BUG=601850
==========

[hbos_chromium, 2016-05-12 11:46:42]

Patchset #2 (id:20001) has been deleted

[hbos_chromium, 2016-05-12 11:51:53]

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

[hbos_chromium, 2016-05-12 11:51:53]

Please take a look, tommi.

[hbos_chromium, 2016-05-12 12:29:02]

On 2016/05/12 11:51:53, hbos_chromium wrote:
> Please take a look, tommi.

Hmh, config files for making sure the flag is tested never landed (I created
https://codereview.chromium.org/1974763002/). In the meantime I should run
trybots with the new code path before landing (e.g. by temporarily flipping the
flag).

[tommi (sloooow) - chröme, 2016-05-12 12:42:56]

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:54: base::WaitableEvent&
request_event() {
non-const on purpose?  If you only need it to wait, I'd prefer having a Wait()
method here and call request_event_.Wait() from there.  As is, this getter
exposes the whole object.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:58:
rtc::scoped_refptr<rtc::RTCCertificate> Result() const {
avoid using rtc:: types in Chrome whenever you can.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:59: result_lock_.Acquire();
use a scoped locker object.

Why do you need the lock though?

How many times will Result() be called and why does result_ need to be valid
after the first call (including in the dtor)?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:62: return ret;
std::move, to avoid extra refcount?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:78: request_event_.Reset();
needed?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:87: GURL url,
const &

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:126:
request_event_.Signal();
will this signal always be waited upon on the main thread?  If not (and never),
then what about always posting the DoCallbackOnMainThread task, only touch
result_ there and only check the observer_ there?
You might not even need request_event_ actually.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:127: if (observer_) {
is it safe to check observer_ on this thread without a lock?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:139: if (certificate)
now you need {} else {}

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:152: rtc::KeyParams
key_params_;
can these be const?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:236: return
identity_observer->Result();
thinking about it some more, do we need the observer interface, or would holding
a callback object that is Run() on the callback thread (not the main thread),
work?  Using that, an event could be signalled or a task posted to the main
thread.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.h:43: static
rtc::scoped_refptr<rtc::RTCCertificate> generateCertificateAndWait(
since this is in Chrome and not overriding anything from webrtc, can we avoid
using rtc::scoped_refptr and use scoped_refptr from Chrome instead?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:738: if (key_type !=
rtc::KT_DEFAULT) {
Can you add a static assert that ensure that KT_DEFAULT is always either
KT_ECDSA or KT_RSA?

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:739:
rtc::scoped_refptr<rtc::RTCCertificate> certificate =
avoid rtc:: types

[hbos_chromium, 2016-05-12 14:28:00]

PTAL tommi.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:54: base::WaitableEvent&
request_event() {
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> non-const on purpose?  If you only need it to wait, I'd prefer having a Wait()
> method here and call request_event_.Wait() from there.  As is, this getter
> exposes the whole object.

Done. Made request_event_ mutable.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:58:
rtc::scoped_refptr<rtc::RTCCertificate> Result() const {
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> avoid using rtc:: types in Chrome whenever you can.

Done.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:59: result_lock_.Acquire();
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> use a scoped locker object.
> 
> Why do you need the lock though?
> 
> How many times will Result() be called and why does result_ need to be valid
> after the first call (including in the dtor)?

Removed the lock and added comment to make sure caller has done WaitForRequest
before calling.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:62: return ret;
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> std::move, to avoid extra refcount?

Now returning const& instead.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:78: request_event_.Reset();
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> needed?

Removed.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:87: GURL url,
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> const &

This copy is on purpose because it is being called cross-threads, a reference
would no longer be valid. (Unless base::Bind automatically copies? Don't think
so?)

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:126:
request_event_.Signal();
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> will this signal always be waited upon on the main thread?  If not (and
never),
> then what about always posting the DoCallbackOnMainThread task, only touch
> result_ there and only check the observer_ there?
> You might not even need request_event_ actually.

Can't do Wait on the same thread as does the Signal; it does not process posted
messages while Wait:ing so it would deadlock. Setting result and signaling here
instead of in the posted task is on purpose.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:127: if (observer_) {
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> is it safe to check observer_ on this thread without a lock?

There is an assumption about there only being one RequestIdentity pending. I
changed it so that RequestIdentity can only be called once so that we can treat
these as constants from that point on and not lock. (Slightly simpler change
than making sure only one is pending.)

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:139: if (certificate)
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> now you need {} else {}

Done.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:152: rtc::KeyParams
key_params_;
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> can these be const?

I tried to make them const by moving what RequestIdentity does into the
constructor, but I ended up with compile errors trying to instantiate the class
due to templates and having to do std::move with the observer which it did not
do. Did not change this.

DCHECKing that RequestIdentity is only called once though, in callback functions
we can treat these as const since they will not change after a request has been
made.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.cc:236: return
identity_observer->Result();
On 2016/05/12 12:42:55, tommi-chrömium wrote:
> thinking about it some more, do we need the observer interface, or would
holding
> a callback object that is Run() on the callback thread (not the main thread),
> work?  Using that, an event could be signalled or a task posted to the main
> thread.

I think I need to re-read that to understand. I'll reply to this later, I gotta
run!

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_certificate_generator.h:43: static
rtc::scoped_refptr<rtc::RTCCertificate> generateCertificateAndWait(
On 2016/05/12 12:42:56, tommi-chrömium wrote:
> since this is in Chrome and not overriding anything from webrtc, can we avoid
> using rtc::scoped_refptr and use scoped_refptr from Chrome instead?

Done.

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:738: if (key_type !=
rtc::KT_DEFAULT) {
On 2016/05/12 12:42:56, tommi-chrömium wrote:
> Can you add a static assert that ensure that KT_DEFAULT is always either
> KT_ECDSA or KT_RSA?

As discussed, that is not necessary. We should make sure we get a certificate of
whatever type key_type is, regardless of KT_DEFAULT. The reason for checking for
KT_DEFAULT is that if we rely on the default code path we end up with the same
certificate that we wanted but the peer connection is doing it for us, utilizing
the the worker thread (async!), which might be beneficial. (But even if
KT_DEFAULT was "KT_CAESAR_CIPHER" this should work.)

https://codereview.chromium.org/1972853003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:739:
rtc::scoped_refptr<rtc::RTCCertificate> certificate =
On 2016/05/12 12:42:56, tommi-chrömium wrote:
> avoid rtc:: types

Done.

[tommi (sloooow) - chröme, 2016-05-12 15:35:57]

The CQ bit was checked by tommi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 15:36:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972853003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972853003/60001

[commit-bot: I haz the power, 2016-05-12 16:20:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-12 16:20:45]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2016-05-13 08:35:47]

Fieltrials make sure code is tested with flag both on and off, but before
landing the trybots only test the "off" path, so the bots are green even though
generateCertificateAndWait deadlocks. (Can be tested locally or by temporarily
modifying the code before running trybots.)
The PeerConnectionIdentityStore used by RTCCertificateGenerator posts tasks on
the main thread. But it is on the main thread we are blocking with Wait(). I
looked into the message loop and I don't think it is possible to safely "run
tasks while waiting". Do you know?

I think these are our options considering time:
1) Pass a different thread than main_thread_ to the
PeerConnectionIdentityStore's constructor so it posts Request/Callback on that
thread instead. (I tried using the webrtc worker thread just to see what would
happen a different thread that I had at my disposal but that crashed...)
2) Do not care about RSA performance and use the already blocking
SSLIdentity::Generate/rtc::RTCCertificateGenerator::GenerateCertificate. But
then if we would have to use the kill switch and go back to RSA we would get a
performance regression.
3) Let KT_DEFAULT remain KT_RSA for the duration of the experiment. That way we
can rely on the default code path and generate asynchronously (we simply do not
land this CL).

1) and 2) has problems. To avoid risk and extra work, I vote for option 3). We
still make KT_DEFAULT = KT_ECDSA when BUILD_WITH_CHROMIUM is false and we launch
in chriomium by making the flag true by default.

Thoughts, tommi?

[hbos_chromium, 2016-05-13 09:17:40]

We'll go ahead with opt. 3) as discussed offline. Closing issue.