RTCCertificate.getFingerprints added (exposed to the web)

RTCCertificate.getFingerprints added (exposed to the web).

Spec: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtccertificate-getfingerprints

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig

BUG=698313, 609108
Review-Url: https://codereview.chromium.org/2828563002
Cr-Commit-Position: refs/heads/master@{#477962}
Committed: https://chromium.googlesource.com/chromium/src/+/149d40a4e451675bbed7a093e56d40fb11f73196

[hbos_chromium, 2017-04-18 18:35:18]

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

[commit-bot: I haz the power, 2017-04-18 18:35:53]

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

[hbos_chromium, 2017-04-18 18:38:00]

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

[hbos_chromium, 2017-04-18 18:38:00]

Please take a look, foolip.

I'll do an Intent to Ship before landing.

[commit-bot: I haz the power, 2017-04-18 19:44:09]

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

[commit-bot: I haz the power, 2017-04-18 19:44:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hbos_chromium, 2017-04-19 08:37:31]

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

[hbos_chromium, 2017-04-19 08:37:32]

PTAL guidou.

[Guido Urdaneta, 2017-04-19 12:20:42]

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate.cc (right):

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.cc:44: return
blink::WebVector<blink::WebRTCDtlsFingerprint>(fingerprints);
Is there a way to know the number of fingerprints in advance so that you can
void the std::vector and this copy?

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp
(right):

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:54: if
(!fingerprints_.size()) {
use IsEmpty() instead of !size

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:62:
fingerprints_.push_back(v8::Global<v8::Value>(
I've seen that other cases that return a FrozenArray of a dictionary type use a
FreezeV8Object function. Should you use it too?

[hbos_chromium, 2017-04-20 09:05:20]

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

[commit-bot: I haz the power, 2017-04-20 09:05:51]

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

[hbos_chromium, 2017-04-20 09:07:10]

PTAL guidou

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate.cc (right):

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.cc:44: return
blink::WebVector<blink::WebRTCDtlsFingerprint>(fingerprints);
On 2017/04/19 12:20:42, Guido Urdaneta wrote:
> Is there a way to know the number of fingerprints in advance so that you can
> void the std::vector and this copy?

I'd have to count it with another for-loop which is messier code, and WebVector
can't change size after construction so I'd have to default-initialize |size|
WebRTCDtlsFingerprints meaning I'd have to add a parameterless constructor.

I opt not to change this code, the copying should be fine because 1) typically
there is only one fingerprint, if multiple it's a tiny number and 2) copying
fingerprints is cheap because copying WebStrings (which is all it contains) is
cheap.

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp
(right):

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:54: if
(!fingerprints_.size()) {
On 2017/04/19 12:20:42, Guido Urdaneta wrote:
> use IsEmpty() instead of !size

Done.

https://codereview.chromium.org/2828563002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:62:
fingerprints_.push_back(v8::Global<v8::Value>(
On 2017/04/19 12:20:42, Guido Urdaneta wrote:
> I've seen that other cases that return a FrozenArray of a dictionary type use
a
> FreezeV8Object function. Should you use it too?

Yes, good catch. Without it the array is frozen but not the objects inside it.

[Guido Urdaneta, 2017-04-20 10:20:03]

guidou@chromium.org changed reviewers:
+ haraken@chromium.org

[Guido Urdaneta, 2017-04-20 10:20:03]

To me it looks good, but would like someone more knowledgeable to review the 
RTCCertificate::fingerprints implementation. 

+haraken@chromium.org: Can you take a look at RTCCertificate::fingerprints(),
especially with regards to the frozen array construction?

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate.cc (right):

https://codereview.chromium.org/2828563002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.cc:44: return
blink::WebVector<blink::WebRTCDtlsFingerprint>(fingerprints);
On 2017/04/20 09:07:09, hbos_chromium wrote:
> On 2017/04/19 12:20:42, Guido Urdaneta wrote:
> > Is there a way to know the number of fingerprints in advance so that you can
> > void the std::vector and this copy?
> 
> I'd have to count it with another for-loop which is messier code, and
WebVector
> can't change size after construction so I'd have to default-initialize |size|
> WebRTCDtlsFingerprints meaning I'd have to add a parameterless constructor.
> 
> I opt not to change this code, the copying should be fine because 1) typically
> there is only one fingerprint, if multiple it's a tiny number and 2) copying
> fingerprints is cheap because copying WebStrings (which is all it contains) is
> cheap.

Acknowledged.

[commit-bot: I haz the power, 2017-04-20 11:57:34]

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

[commit-bot: I haz the power, 2017-04-20 11:57:34]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
Vector<v8::Global<v8::Value>> fingerprints_;

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:54]

Some comments. As pedantic tests as possible in wpt would be great :)

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp
(right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:61:
fingerprint.setAlgorithm(web_fingerprint.Algorithm());
Can anything be asserted about the value? Looks per spec like at least they
cannot be empty strings?

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:69:
fingerprints[i] = FreezeV8Object(
The members of a FrozenArray shouldn't be frozen, it the array itself that
should be frozen. Can you write a test that checks that the objects in the array
can in fact be modified?

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:70: //
|DISALLOW_NEW_EXCEPT_PLACEMENT_NEW|). |toV8| is used to convert the
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.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
Vector<v8::Global<v8::Value>> fingerprints_;
On 2017/04/20 13:21:54, haraken wrote:
> 
> 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.
> 

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.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCDtlsFingerprint.idl
(right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCDtlsFingerprint.idl:5: //
https://www.w3.org/TR/webrtc/#dom-rtcdtlsfingerprint
Please avoid /TR/ links as they are usually not the most up-to-date version of
any spec. https://w3c.github.io/webrtc-pc/#rtcdtlsfingerprint* is what I'd link
to here, and I've sent https://github.com/w3c/webrtc-pc/pull/1139/files to fix
the weird section heading.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCCertificate.h:17: //
https://www.w3.org/TR/webrtc/#dom-rtcdtlsfingerprint
Ditto

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

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:70: //
|DISALLOW_NEW_EXCEPT_PLACEMENT_NEW|). |toV8| is used to convert the
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> 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.

Won't HeapVector<RTCDtlsFingerprint> work?

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
Vector<v8::Global<v8::Value>> fingerprints_;
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> On 2017/04/20 13:21:54, haraken wrote:
> > 
> > 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.
> > 
> 
> Does this actually achieve the intended effect though?

Yes, but the problem is that v8::Global<v8::Value> will cause memory leaks.

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

Yeah, [SaveSameObject] seems like a right solution.

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

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:70: //
|DISALLOW_NEW_EXCEPT_PLACEMENT_NEW|). |toV8| is used to convert the
On 2017/04/21 19:59:16, haraken wrote:
> On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> > 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

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.

> > there's a better way that could be supported? Link to it here if it's not
> > immediately closed as invalid.
> 
> Won't HeapVector<RTCDtlsFingerprint> work?

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

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
Vector<v8::Global<v8::Value>> fingerprints_;
On 2017/04/21 19:59:16, haraken wrote:
> On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> > On 2017/04/20 13:21:54, haraken wrote:
> > > 
> > > 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.
> > > 
> > 
> > Does this actually achieve the intended effect though?
> 
> Yes, but the problem is that v8::Global<v8::Value> will cause memory leaks.
> 
> > 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.
> 
> Yeah, [SaveSameObject] seems like a right solution.

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]

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
Vector<v8::Global<v8::Value>> fingerprints_;
On 2017/04/24 10:35:31, hbos_chromium wrote:
> On 2017/04/21 19:59:16, haraken wrote:
> > On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> > > On 2017/04/20 13:21:54, haraken wrote:
> > > > 
> > > > 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.
> > > > 

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.

> > > 
> > > Does this actually achieve the intended effect though?
> > 
> > Yes, but the problem is that v8::Global<v8::Value> will cause memory leaks.
> > 
> > > 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.
> > > 

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.

> > > 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.
> > 
> > Yeah, [SaveSameObject] seems like a right solution.
> 
> 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?

[foolip, 2017-04-24 10:46:40]

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

Since the same array should be returned each time, and it's frozen, it will
return the same objects-representing-dictionaries every time. Those objects
themselves will be mutable though.

[hbos_chromium, 2017-04-24 10:48:46]

On 2017/04/24 10:46:40, foolip_UTC7 wrote:
> > > > > 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.
> > > > > 
> > 
> > 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.
> 
> Since the same array should be returned each time, and it's frozen, it will
> return the same objects-representing-dictionaries every time. Those objects
> themselves will be mutable though.

Oh... If they're mutable then returning same object would be dangerous because
then the caller could mess up the fingerprints.
In this CL I assumed even the underlying objects should be frozen, but in that
case I should remove the FreezeV8Object call.

[hbos_chromium, 2017-04-24 10:49:44]

On 2017/04/24 10:48:46, hbos_chromium wrote:
> On 2017/04/24 10:46:40, foolip_UTC7 wrote:
> > > > > > 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.
> > > > > > 
> > > 
> > > 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.
> > 
> > Since the same array should be returned each time, and it's frozen, it will
> > return the same objects-representing-dictionaries every time. Those objects
> > themselves will be mutable though.
> 
> Oh... If they're mutable then returning same object would be dangerous because
> then the caller could mess up the fingerprints.
> In this CL I assumed even the underlying objects should be frozen, but in that
> case I should remove the FreezeV8Object call.

No they should be immutable: "A frozen array type is a parameterized type whose
values are references to objects that hold a fixed length array of unmodifiable
values."
https://heycam.github.io/webidl/#idl-frozen-array

[foolip, 2017-04-24 10:56:08]

On 2017/04/24 10:49:44, hbos_chromium wrote:
> On 2017/04/24 10:48:46, hbos_chromium wrote:
> > On 2017/04/24 10:46:40, foolip_UTC7 wrote:
> > > > > > > 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.
> > > > > > > 
> > > > 
> > > > 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.
> > > 
> > > Since the same array should be returned each time, and it's frozen, it
will
> > > return the same objects-representing-dictionaries every time. Those
objects
> > > themselves will be mutable though.
> > 
> > Oh... If they're mutable then returning same object would be dangerous
because
> > then the caller could mess up the fingerprints.
> > In this CL I assumed even the underlying objects should be frozen, but in
that
> > case I should remove the FreezeV8Object call.
> 
> No they should be immutable: "A frozen array type is a parameterized type
whose
> values are references to objects that hold a fixed length array of
unmodifiable
> values."
> https://heycam.github.io/webidl/#idl-frozen-array

That's a bit ambiguous, but I think it refers to the fact that
cert.fingerprints[0]=42 doesn't work. Note that
https://heycam.github.io/webidl/#es-frozen-array only does the same steps as for
converting to sequence<T> and then freezes the array. So if the objects
converted from dictionaries should also be frozen, the WebRTC spec would have to
do that in some extra prose.

You might want to file a Web IDL bug to ask that this is clarified though.

[haraken, 2017-04-24 10:59:05]

On 2017/04/24 10:35:31, hbos_chromium wrote:
>
https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h
(right):
> 
>
https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:70: //
> |DISALLOW_NEW_EXCEPT_PLACEMENT_NEW|). |toV8| is used to convert the
> On 2017/04/21 19:59:16, haraken wrote:
> > On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> > > 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
> 
> 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.
> 
> > > there's a better way that could be supported? Link to it here if it's not
> > > immediately closed as invalid.
> > 
> > Won't HeapVector<RTCDtlsFingerprint> work?
> 
> 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

You should be able to use HeapVector<RTCDtlsFingerprint>, although you cannot
use HeapVector<Member<RTCDtlsFingerprint>>. HeapVector<RTCDtlsFingerprint>
doesn't require 'new'.

> 
>
https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/peerconnection/RTCCertificate.h:77:
> Vector<v8::Global<v8::Value>> fingerprints_;
> On 2017/04/21 19:59:16, haraken wrote:
> > On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> > > On 2017/04/20 13:21:54, haraken wrote:
> > > > 
> > > > 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.
> > > > 
> > > 
> > > Does this actually achieve the intended effect though?
> > 
> > Yes, but the problem is that v8::Global<v8::Value> will cause memory leaks.
> > 
> > > 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.
> > 
> > Yeah, [SaveSameObject] seems like a right solution.
> 
> 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.

Note that v8::Global is treated as a GC root for V8.

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

^^^ This creates a cycle. It leaks all objects reachable from fingerprints_,
including the window object.

[hbos_chromium, 2017-04-24 15:06:37]

Patchset #4 (id:60001) has been deleted

[hbos_chromium, 2017-04-24 15:06:49]

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

[commit-bot: I haz the power, 2017-04-24 15:07:33]

Dry run: CQ is trying da patch.

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

[hbos_chromium, 2017-04-24 15:25:27]

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

[commit-bot: I haz the power, 2017-04-24 15:25:51]

Dry run: CQ is trying da patch.

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

[hbos_chromium, 2017-04-24 15:36:53]

PTAL, using HeapVector and added unittests. I made sure fingerprints are
lowercase too as the spec says and verified fingerprints with regex.

With regards to [SameObject]... It seems FrozenArray means the array is frozen,
not the elements. We can't return the same elements each time if the caller can
modify them, and dictionaries should always be returned by value so that
modification of them is safe. As such we have to return a new array every time.
This makes it seem rather pointless to return FrozenArray over sequence...
Discussion to be had on spec bug. Easy/safe change to go from FrozenArray to
sequence if that changes.

On 2017/04/24 10:59:05, haraken wrote:
> You should be able to use HeapVector<RTCDtlsFingerprint>, although you cannot
> use HeapVector<Member<RTCDtlsFingerprint>>. HeapVector<RTCDtlsFingerprint>
> doesn't require 'new'.

You're right, I guess my concern is generally not applicable then. This worked
with the cache too, but I decided to remove it for simplicity - premature
optimization.

> 
> > On 2017/04/21 19:59:16, haraken wrote:
> Note that v8::Global is treated as a GC root for V8.
> 
> > > > >   fingerprints_ (GC root) => V8 object = (any arbitrary reference) =>
> > RTCCertificate's
> > > > > wrapper => RTCCertificate => fingerprints_
> 
> ^^^ This creates a cycle. It leaks all objects reachable from fingerprints_,
> including the window object.

Roger that!

[hbos_chromium, 2017-04-24 15:37:54]

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp
(right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:61:
fingerprint.setAlgorithm(web_fingerprint.Algorithm());
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> Can anything be asserted about the value? Looks per spec like at least they
> cannot be empty strings?

Done.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.cpp:69:
fingerprints[i] = FreezeV8Object(
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> The members of a FrozenArray shouldn't be frozen, it the array itself that
> should be frozen. Can you write a test that checks that the objects in the
array
> can in fact be modified?

I added tests for this, also regex-testing the fingerprints. I discovered I used
upper-case instead of lower-case and fixed that.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/peerconnection/RTCDtlsFingerprint.idl
(right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/peerconnection/RTCDtlsFingerprint.idl:5: //
https://www.w3.org/TR/webrtc/#dom-rtcdtlsfingerprint
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> Please avoid /TR/ links as they are usually not the most up-to-date version of
> any spec. https://w3c.github.io/webrtc-pc/#rtcdtlsfingerprint* is what I'd
link
> to here, and I've sent https://github.com/w3c/webrtc-pc/pull/1139/files to fix
> the weird section heading.

Done.

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/2828563002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCCertificate.h:17: //
https://www.w3.org/TR/webrtc/#dom-rtcdtlsfingerprint
On 2017/04/21 16:51:53, foolip_UTC7 wrote:
> Ditto

Done.

[commit-bot: I haz the power, 2017-04-24 16:43:22]

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

[commit-bot: I haz the power, 2017-04-24 16:43:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hbos_chromium, 2017-04-24 17:47:53]

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

[commit-bot: I haz the power, 2017-04-24 17:48:44]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-04-24 19:13:22]

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

[commit-bot: I haz the power, 2017-04-24 19:13:24]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-04-25 12:49:31]

https://codereview.chromium.org/2828563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCCertificate.idl
(right):

https://codereview.chromium.org/2828563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCCertificate.idl:35: readonly
attribute FrozenArray<RTCDtlsFingerprint> fingerprints;
Assuming https://github.com/w3c/webrtc-pc/pull/1145 is approved, I'll need to
change this to sequence<RTCDtlsFingerprint> getFingerprints(). You can hold of
review until that and I've done an Intent to Implement & Ship.

[haraken, 2017-04-25 17:42:48]

From the implementation perspective, LGTM.

Let's wait for foolip's approval.

[foolip, 2017-04-25 17:52:05]

On 2017/04/25 17:42:48, haraken wrote:
> From the implementation perspective, LGTM.
> 
> Let's wait for foolip's approval.

This looks fine to me as well, but there are spec changes to be done and an
Intent to Ship, so putting on hold as suggested.

[hbos_chromium, 2017-04-28 11:14:47]

Description was changed from

==========
RTCCertificate.fingerprints added (exposed to the web).

Spec: https://w3c.github.io/webrtc-pc/#rtccertificate-interface

BUG=698313, 609108
==========

to

==========
RTCCertificate.fingerprints added (exposed to the web).

Spec: https://w3c.github.io/webrtc-pc/#rtccertificate-interface
PR changing "fingerprints" to "getFingerprints()":
https://github.com/w3c/webrtc-pc/pull/1145

BUG=698313, 609108
==========

[hbos_chromium, 2017-04-28 11:17:34]

Description was changed from

==========
RTCCertificate.fingerprints added (exposed to the web).

Spec: https://w3c.github.io/webrtc-pc/#rtccertificate-interface
PR changing "fingerprints" to "getFingerprints()":
https://github.com/w3c/webrtc-pc/pull/1145

BUG=698313, 609108
==========

to

==========
RTCCertificate.getFingerprints added (exposed to the web).

Spec: https://w3c.github.io/webrtc-pc/#rtccertificate-interface
PR changing "fingerprints" to "getFingerprints()":
https://github.com/w3c/webrtc-pc/pull/1145

BUG=698313, 609108
==========

[hbos_chromium, 2017-04-28 11:41:35]

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

[commit-bot: I haz the power, 2017-04-28 11:41:49]

Dry run: CQ is trying da patch.

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

[hbos_chromium, 2017-04-28 11:55:40]

PTAL guidou and foolip.

The PR has three LGTMs to change "fingerprints" to "getFingerprints()", I made
the appropriate changes.

As soon as the PR has been merged I'll do the Intent to Ship, but this CL seems
unlikely to change. I'll hold off landing until the I2S has been approved.

[commit-bot: I haz the power, 2017-04-28 13:24:21]

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

[commit-bot: I haz the power, 2017-04-28 13:24:22]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-05-03 15:04:29]

third_party/WebKit/ implementation LGTM, but it would be very good to write the
tests as part of LayoutTest/external/wpt/, since that will be much appreciated
(but not required) in an Intent to Ship. Happy to review such tests if added.

[Guido Urdaneta, 2017-05-03 15:54:15]

https://codereview.chromium.org/2828563002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-generateCertificate-expected.txt
(right):

https://codereview.chromium.org/2828563002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-generateCertificate-expected.txt:12:
FAIL (function(){ fingerprints[0] = null; return fingerprints[0] != null; })()
should be true. Was false.
Are these FAIL intended?

[hbos_chromium, 2017-05-05 13:05:20]

https://codereview.chromium.org/2828563002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-generateCertificate-expected.txt
(right):

https://codereview.chromium.org/2828563002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-generateCertificate-expected.txt:12:
FAIL (function(){ fingerprints[0] = null; return fingerprints[0] != null; })()
should be true. Was false.
On 2017/05/03 15:54:15, Guido Urdaneta wrote:
> Are these FAIL intended?

Oops, nope, good catch. Done.

[Guido Urdaneta, 2017-05-05 13:09:55]

lgtm

[hbos_chromium, 2017-05-05 13:38:20]

Description was changed from

==========
RTCCertificate.getFingerprints added (exposed to the web).

Spec: https://w3c.github.io/webrtc-pc/#rtccertificate-interface
PR changing "fingerprints" to "getFingerprints()":
https://github.com/w3c/webrtc-pc/pull/1145

BUG=698313, 609108
==========

to

==========
RTCCertificate.getFingerprints added (exposed to the web).

Spec:
https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtccertificate-getfin...

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig

BUG=698313, 609108
==========

[hbos_chromium, 2017-05-05 13:38:55]

Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig
web-platform-tests: https://github.com/w3c/web-platform-tests/issues/5796

[foolip, 2017-05-05 19:54:27]

On 2017/05/05 13:38:55, hbos_chromium wrote:
> Intent to Implement:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig
> web-platform-tests: https://github.com/w3c/web-platform-tests/issues/5796

Can the wpt issue be fixed in this CL? I don't think it's blocked on any
automation, right?

A cool trick you can do is to write "Fixes
https://github.com/w3c/web-platform-tests/issues/5796" in the description and
once the CL is exported, that issue will be closed automatically :)

[hbos_chromium, 2017-06-08 11:17:48]

On 2017/05/05 19:54:27, foolip OOO until June 13 wrote:
> On 2017/05/05 13:38:55, hbos_chromium wrote:
> > Intent to Implement:
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig
> > web-platform-tests: https://github.com/w3c/web-platform-tests/issues/5796
> 
> Can the wpt issue be fixed in this CL? I don't think it's blocked on any
> automation, right?
> 
> A cool trick you can do is to write "Fixes
> https://github.com/w3c/web-platform-tests/issues/5796%22 in the description
and
> once the CL is exported, that issue will be closed automatically :)

The web-platform-tests have now been added by soareschen, rebasing and merging
this CL.

[hbos_chromium, 2017-06-08 11:23:43]

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

[commit-bot: I haz the power, 2017-06-08 11:23:55]

Dry run: CQ is trying da patch.

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

[hbos_chromium, 2017-06-08 11:25:39]

The CQ bit was unchecked by hbos@chromium.org

[hbos_chromium, 2017-06-08 11:25:47]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2017-06-08 11:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, foolip@chromium.org, guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2828563002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-08 11:25:54]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-08 14:40:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496921147070730,
"parent_rev": "0378b3560726088bb24b5ae81bef3f94753855f0", "commit_rev":
"149d40a4e451675bbed7a093e56d40fb11f73196"}

[commit-bot: I haz the power, 2017-06-08 14:40:50]

Description was changed from

==========
RTCCertificate.getFingerprints added (exposed to the web).

Spec:
https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtccertificate-getfin...

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig

BUG=698313, 609108
==========

to

==========
RTCCertificate.getFingerprints added (exposed to the web).

Spec:
https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtccertificate-getfin...

Intent to Implement & Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/h8qqS5sI-ig

BUG=698313, 609108

Review-Url: https://codereview.chromium.org/2828563002
Cr-Commit-Position: refs/heads/master@{#477962}
Committed:
https://chromium.googlesource.com/chromium/src/+/149d40a4e451675bbed7a093e56d...
==========

[commit-bot: I haz the power, 2017-06-08 14:40:53]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/149d40a4e451675bbed7a093e56d...