content::RTCCertificateGenerator and WebRTC-EnableWebRtcEcdsa update.

content/renderer/media/rtc_certificate_generator.cc

Index: content/renderer/media/rtc_certificate_generator.cc
diff --git a/content/renderer/media/rtc_certificate_generator.cc b/content/renderer/media/rtc_certificate_generator.cc
index 94b94fb1c899520cd8520b5bde5b9b1c9ebc4e6a..3a5ecffa42fdf1ee7f355817d242caba6f1fc789 100644
--- a/content/renderer/media/rtc_certificate_generator.cc
+++ b/content/renderer/media/rtc_certificate_generator.cc
@@ -9,12 +9,12 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/synchronization/lock.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/renderer/media/peer_connection_identity_store.h"
 #include "content/renderer/media/rtc_certificate.h"
 #include "content/renderer/media/webrtc/peer_connection_dependency_factory.h"
 #include "content/renderer/render_thread_impl.h"
-#include "third_party/webrtc/base/rtccertificate.h"
-#include "third_party/webrtc/base/scoped_ref_ptr.h"
 #include "url/gurl.h"
 
 namespace content {
@@ -44,26 +44,38 @@ class RTCCertificateIdentityObserver
       const scoped_refptr<base::SingleThreadTaskRunner>& signaling_thread)
       : main_thread_(main_thread),
         signaling_thread_(signaling_thread),
-        observer_(nullptr) {
+        observer_(nullptr),
+        request_event_(false, false) {
     DCHECK(main_thread_);
     DCHECK(signaling_thread_);
   }
   ~RTCCertificateIdentityObserver() override {}
 
-  // Perform |store|->RequestIdentity with this identity observer and ensure
-  // that this identity observer is not deleted until the request has completed
-  // by holding on to a reference to itself for the duration of the request.
+  base::WaitableEvent& request_event() {

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

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.

[hbos_chromium, 2016/05/12 14:27:59]

Done. Made request_event_ mutable.

+    return request_event_;
+  }
+
+  rtc::scoped_refptr<rtc::RTCCertificate> Result() const {

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

avoid using rtc:: types in Chrome whenever you can.

[hbos_chromium, 2016/05/12 14:27:59]

Done.

+    result_lock_.Acquire();

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

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)?

[hbos_chromium, 2016/05/12 14:27:59]

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

+    rtc::scoped_refptr<rtc::RTCCertificate> ret(result_);
+    result_lock_.Release();
+    return ret;

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

std::move, to avoid extra refcount?

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

Now returning const& instead.

+  }
+
+  // Perform |store|->RequestIdentity with |this| identity observer and ensure
+  // that |this| identity observer is not deleted until the request has
+  // completed by holding on to a reference to itself for the duration of the
+  // request. The |observer| is optional, see |request_event| and |Result|.
   void RequestIdentity(
-      const blink::WebRTCKeyParams& key_params,
+      const rtc::KeyParams& key_params,
       const GURL& url,
       const GURL& first_party_for_cookies,
       const rtc::Optional<uint64_t>& expires_ms,
       std::unique_ptr<blink::WebRTCCertificateCallback> observer) {
     DCHECK(main_thread_->BelongsToCurrentThread());
-    DCHECK(!observer_) << "Already have a RequestIdentity in progress.";
     key_params_ = key_params;
     observer_ = std::move(observer);
-    DCHECK(observer_);
+    request_event_.Reset();

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

needed?

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

Removed.

     // Identity request must be performed on the WebRTC signaling thread.
     signaling_thread_->PostTask(FROM_HERE, base::Bind(
         &RTCCertificateIdentityObserver::RequestIdentityOnWebRtcSignalingThread,
@@ -79,16 +91,14 @@ class RTCCertificateIdentityObserver
     std::unique_ptr<PeerConnectionIdentityStore> store(
         new PeerConnectionIdentityStore(main_thread_, signaling_thread_, url,
                                         first_party_for_cookies));
-    // Request identity with |this| as the observer. OnSuccess/OnFailure will be
-    // called asynchronously.
-    store->RequestIdentity(WebRTCKeyParamsToKeyParams(key_params_),
-                           expires_ms, this);
+    // Request identity with |this| as the observer. |OnSuccess|/|OnFailure|
+    // will be called asynchronously.
+    store->RequestIdentity(key_params_, expires_ms, this);
   }
 
   // webrtc::DtlsIdentityRequestObserver implementation.
   void OnFailure(int error) override {
     DCHECK(signaling_thread_->BelongsToCurrentThread());
-    DCHECK(observer_);
     main_thread_->PostTask(FROM_HERE, base::Bind(
         &RTCCertificateIdentityObserver::DoCallbackOnMainThread,
         this, nullptr));
@@ -108,22 +118,27 @@ class RTCCertificateIdentityObserver
   }
   void OnSuccess(std::unique_ptr<rtc::SSLIdentity> identity) override {
     DCHECK(signaling_thread_->BelongsToCurrentThread());
-    DCHECK(observer_);
     rtc::scoped_refptr<rtc::RTCCertificate> certificate =
         rtc::RTCCertificate::Create(std::move(identity));
-    main_thread_->PostTask(
-        FROM_HERE,
-        base::Bind(&RTCCertificateIdentityObserver::DoCallbackOnMainThread,
-                   this, base::Passed(base::WrapUnique(
-                             new RTCCertificate(certificate)))));
+    result_lock_.Acquire();
+    result_ = certificate;
+    result_lock_.Release();
+    request_event_.Signal();

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

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.

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

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.

+    if (observer_) {

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

is it safe to check observer_ on this thread without a lock?

[hbos_chromium, 2016/05/12 14:27:59]

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.)

+      main_thread_->PostTask(
+          FROM_HERE,
+          base::Bind(&RTCCertificateIdentityObserver::DoCallbackOnMainThread,
+                     this, certificate));
+    }
   }
 
   void DoCallbackOnMainThread(
-      std::unique_ptr<blink::WebRTCCertificate> certificate) {
+      rtc::scoped_refptr<rtc::RTCCertificate> certificate) {
     DCHECK(main_thread_->BelongsToCurrentThread());
     DCHECK(observer_);
     if (certificate)

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

now you need {} else {}

[hbos_chromium, 2016/05/12 14:27:59]

Done.

-      observer_->onSuccess(std::move(certificate));
+      observer_->onSuccess(std::unique_ptr<blink::WebRTCCertificate>(
+          new RTCCertificate(certificate)));
     else
       observer_->onError();
     observer_.reset();
@@ -134,8 +149,11 @@ class RTCCertificateIdentityObserver
   // The signaling thread is a WebRTC thread used to invoke
   // PeerConnectionIdentityStore::RequestIdentity on, as is required.
   const scoped_refptr<base::SingleThreadTaskRunner> signaling_thread_;
-  blink::WebRTCKeyParams key_params_;
+  rtc::KeyParams key_params_;

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

can these be const?

[hbos_chromium, 2016/05/12 14:27:59]

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.

   std::unique_ptr<blink::WebRTCCertificateCallback> observer_;
+  base::WaitableEvent request_event_;
+  rtc::scoped_refptr<rtc::RTCCertificate> result_;
+  mutable base::Lock result_lock_;
 
   DISALLOW_COPY_AND_ASSIGN(RTCCertificateIdentityObserver);
 };
@@ -185,13 +203,42 @@ void RTCCertificateGenerator::generateCertificateWithOptionalExpiration(
       new rtc::RefCountedObject<RTCCertificateIdentityObserver>(
           main_thread, signaling_thread));
   // |identity_observer| lives until request has completed.
-  identity_observer->RequestIdentity(key_params, url, first_party_for_cookies,
-                                     expires_ms, std::move(observer));
+  identity_observer->RequestIdentity(
+      WebRTCKeyParamsToKeyParams(key_params), url, first_party_for_cookies,
+      expires_ms, std::move(observer));
 #else
   observer->onError();
 #endif
 }
 
+rtc::scoped_refptr<rtc::RTCCertificate>
+RTCCertificateGenerator::generateCertificateAndWait(
+      const rtc::KeyParams& key_params,
+      const rtc::Optional<uint64_t>& expires_ms) {
+#if defined(ENABLE_WEBRTC)
+  const scoped_refptr<base::SingleThreadTaskRunner> main_thread =
+      base::ThreadTaskRunnerHandle::Get();
+
+  PeerConnectionDependencyFactory* pc_dependency_factory =
+      RenderThreadImpl::current()->GetPeerConnectionDependencyFactory();
+  pc_dependency_factory->EnsureInitialized();
+  const scoped_refptr<base::SingleThreadTaskRunner> signaling_thread =
+      pc_dependency_factory->GetWebRtcSignalingThread();
+
+  rtc::scoped_refptr<RTCCertificateIdentityObserver> identity_observer(
+      new rtc::RefCountedObject<RTCCertificateIdentityObserver>(
+          main_thread, signaling_thread));
+
+  // Don't care about URL origin parameters, using empty URL.
+  identity_observer->RequestIdentity(key_params, GURL(), GURL(),
+                                     expires_ms, nullptr);
+  identity_observer->request_event().Wait();
+  return identity_observer->Result();

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

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.

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

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

+#else
+  return nullptr;
+#endif
+}
+
 bool RTCCertificateGenerator::isSupportedKeyParams(
     const blink::WebRTCKeyParams& key_params) {
   return WebRTCKeyParamsToKeyParams(key_params).IsValid();