RTCCertificate.getFingerprints added (exposed to the web)

third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h

Index: third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h
diff --git a/third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h b/third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h
index 837ff114825262c9a5435341d13c46490ccb5816..2a00b62161bd86af5810f05afb0cdba4bc6a00ca 100644
--- a/third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h
+++ b/third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h
@@ -34,12 +34,19 @@
 #include "bindings/core/v8/ScriptWrappable.h"
 #include "core/dom/DOMTimeStamp.h"
 #include "modules/ModulesExport.h"
+#include "modules/peerconnection/RTCDtlsFingerprint.h"
 #include "platform/heap/GarbageCollected.h"
+#include "platform/wtf/Vector.h"
+#include "platform/wtf/text/WTFString.h"
 #include "public/platform/WebRTCCertificate.h"
+#include "v8/include/v8.h"
+
 #include <memory>
 
 namespace blink {
 
+class ScriptState;
+
 class MODULES_EXPORT RTCCertificate final
     : public GarbageCollectedFinalized<RTCCertificate>,
       public ScriptWrappable {
@@ -57,9 +64,17 @@ class MODULES_EXPORT RTCCertificate final
 
   // Returns the expiration time in ms relative to epoch, 1970-01-01T00:00:00Z.
   DOMTimeStamp expires() const;
+  // Returns "FrozenArray<RTCDtlsFingerprint>" as per IDL. Because
+  // |RTCDtlsFingerprint| is a dictionary type, vectors of
+  // |RTCDtlsFingerprint| are not allowed (due to
+  // |DISALLOW_NEW_EXCEPT_PLACEMENT_NEW|). |toV8| is used to convert the

[foolip, 2017/04/21 16:51:53]

This sound rather annoying and something that will affect anyone trying to add a
FrozenArray<DictionaryThing>. Can you file a bug for Blink>Bindings and ask if
there's a better way that could be supported? Link to it here if it's not
immediately closed as invalid.

[haraken, 2017/04/21 19:59:16]

Won't HeapVector<RTCDtlsFingerprint> work?

[hbos_chromium, 2017/04/24 10:35:31]

It's annoying for anyone wanting to return any data structure containing
dictionaries or wanting to return by reference and not copy by return value.
No, HeapVector requires "new RTCDtlsFingerprint", not allowed, dictionaries must
live on the call stack or in the class body, they can only be copied and
returned by value. This is a strange design decision considering dictionaries
are javascript objects and javascript objects do live on the heap and can be
referenced as objects. So why not as their actual type?

This has caused me problems before when wanting a Maplike of dictionary types,
and I wound up having to use v8::Value to reference the resulting javascript
objects as opposed to the dictionary class: https://crbug.com/630210#c10

I filed: https://crbug.com/714575

+  // |RTCDtlsFingerprint| objects to |v8::Value|s which can populate vectors.
+  Vector<v8::Local<v8::Value>> fingerprints(ScriptState*);
 
  private:
   std::unique_ptr<WebRTCCertificate> certificate_;
+  // Cache for the result of |fingerprints|.
+  Vector<v8::Global<v8::Value>> fingerprints_;

[haraken, 2017/04/20 13:21:54]

This strong persistent handle will leak many things. Note that this creates the
following object cycle:

  fingerprints_ => V8 object = (any arbitrary reference) => RTCCertificate's
wrapper => RTCCertificate => fingerprints_

Do you really need to cache the result? There is a way to cache it without
causing leaks (i.e., wrapper tracing) but if this is not performance-sensitive,
I'd prefer simply removing the cache.

[foolip, 2017/04/21 16:51:53]

Does this actually achieve the intended effect though? I've filed
https://github.com/w3c/webrtc-pc/issues/1138 to clarify the spec, and assume
that we actually want to return the same value each time.

If it's possible to write a test for this in LayoutTests/external/wpt/webrtc
that would be great. I think `assert_equals(certificate.fingerprints,
certificate.fingerprints)` will fail, and what is needed is [SaveSameObject].
Then the code can just return something once and will never be called from
bindings again.

[haraken, 2017/04/21 19:59:16]

Yes, but the problem is that v8::Global<v8::Value> will cause memory leaks.
Yeah, [SaveSameObject] seems like a right solution.

[hbos_chromium, 2017/04/24 10:35:31]

How could this create a cycle? It's a list of javascript objects, none of which
reference the RTCCertificate. They can't be modified externally to reference the
RTCCertificate again to create that cycle because they're frozen objects.

The cache should only live for the duration of the RTCCertificate object?

[hbos_chromium, 2017/04/24 10:41:09]

What is this wrapper tracing thing you speak of?
I believe fingerprints aren't free but there are so few of them (typically only
a single one) that I can go ahead and remove the cache if this other stuff is a
problem.
I don't think it matters whether or not you return the same objects, it's
essentially strings, but returning the same object would make sense if caching
is something we want to do.

 };
 
 }  // namespace blink