Bug 166916 – [WebRTC] Support modern RTCStatsReport

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

This requires supporting mapLike

[youenn fablet]

Created attachment 298537 [details]
WIP

[Build Bot]

Comment on attachment 298537 [details]
WIP

Attachment 298537 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2867171

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 298548 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 298661 [details]
Patch

[youenn fablet]

Created attachment 298662 [details]
Patch

[youenn fablet]

Created attachment 298694 [details]
Patch

[Build Bot]

Comment on attachment 298694 [details]
Patch

Attachment 298694 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2878259

New failing tests:
webrtc/stats.html

[Build Bot]

Created attachment 298700 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 298694 [details]
Patch

Attachment 298694 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2878235

New failing tests:
webrtc/stats.html

[Build Bot]

Created attachment 298701 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 298706 [details]
Patch

[youenn fablet]

Created attachment 298712 [details]
Patch

[Build Bot]

Comment on attachment 298712 [details]
Patch

Attachment 298712 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2879934

New failing tests:
fast/mediastream/MediaStream-video-element-track-stop.html

[Build Bot]

Created attachment 298745 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Sam Weinig]

Comment on attachment 298712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review

> Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48
> +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id));
> +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type)));
> +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp));

I'm not thrilled with having so much JS code in non-bindings code.  Are there any other ways to accomplish this while still keeping a more substantial boundary?

[Sam Weinig]

(In reply to comment #15)
> Comment on attachment 298712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298712&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id));
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type)));
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp));
> 
> I'm not thrilled with having so much JS code in non-bindings code.  Are
> there any other ways to accomplish this while still keeping a more
> substantial boundary?

For instance, why couldn't we just use a HashMap as the backing map?

[Sam Weinig]

Comment on attachment 298712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review

> Source/WebCore/bindings/js/JSDOMBinding.cpp:1039
> +    mapLike.putDirect(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().backingMapPrivateName(), JSMap::create(mapLike.globalObject()->globalExec(), vm, mapLike.globalObject()->mapStructure()), DontEnum);

If we do go the JSMap as backing map route, do we need to do something to prevent the user from tampering with the prototype of the Map object (e.g. have an JSInternalMap, like we have JSInternalPromise)?

[youenn fablet]

> I'm not thrilled with having so much JS code in non-bindings code.  Are
> there any other ways to accomplish this while still keeping a more
> substantial boundary?

We can move that code to bindings/js, something like a writable dictionary.

> For instance, why couldn't we just use a HashMap as the backing map?

We could but we would depart from https://heycam.github.io/webidl/#es-maplike.
The goal of maplike is to be totally equivalent to es Map.
For instance, iterators returned by maplike should be @MapIterator.

[youenn fablet]

(In reply to comment #17)
> Comment on attachment 298712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298712&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:1039
> > +    mapLike.putDirect(vm, static_cast<JSVMClientData*>(vm.clientData)->builtinNames().backingMapPrivateName(), JSMap::create(mapLike.globalObject()->globalExec(), vm, mapLike.globalObject()->mapStructure()), DontEnum);
> 
> If we do go the JSMap as backing map route, do we need to do something to
> prevent the user from tampering with the prototype of the Map object (e.g.
> have an JSInternalMap, like we have JSInternalPromise)?

From my quick reading of webidl, we are to use the public slot of the Map object.
But it would probably be safer to use private slots on the Map prototype.

[youenn fablet]

I'll fix the test and will improve the JS binding layering

[Darin Adler]

Comment on attachment 298712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298712&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388
> +    if (m_signalingState == SignalingState::Closed)
> +        promise.reject(INVALID_STATE_ERR);

I think this is missing a return. Or am I mistaken? Do we have a test case covering this?

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 298712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298712&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388
> > +    if (m_signalingState == SignalingState::Closed)
> > +        promise.reject(INVALID_STATE_ERR);
> 
> I think this is missing a return. Or am I mistaken? Do we have a test case
> covering this?

That is right.
LayoutTests/fast/mediastream/RTCPeerConnection-closed-state.html is covering this case.
I am not sure why it has not caught it.

[youenn fablet]

Created attachment 299067 [details]
Patch

[youenn fablet]

(In reply to comment #22)
> (In reply to comment #21)
> > Comment on attachment 298712 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=298712&action=review
> > 
> > > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:388
> > > +    if (m_signalingState == SignalingState::Closed)
> > > +        promise.reject(INVALID_STATE_ERR);
> > 
> > I think this is missing a return. Or am I mistaken? Do we have a test case
> > covering this?
> 
> That is right.
> LayoutTests/fast/mediastream/RTCPeerConnection-closed-state.html is covering
> this case.
> I am not sure why it has not caught it.

Since the change to make promise active dom objects, a promise may be rejected/resolved twice without getting notice. The second time, it just exits early.

[youenn fablet]

(In reply to comment #15)
> Comment on attachment 298712 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298712&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCStatsReport.cpp:48
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "id"), toJS<IDLDOMString>(state, globalObject, m_id));
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "type"), toJS<IDLDOMString>(state, globalObject, String(type)));
> > +    m_object->putDirect(vm, JSC::Identifier::fromString(&vm, "timestamp"), toJS<IDLUnsignedLongLong>(timestamp));
> 
> I'm not thrilled with having so much JS code in non-bindings code.  Are
> there any other ways to accomplish this while still keeping a more
> substantial boundary?

I added an UntypedDictionary.
It might be useful in the future to have a kind of extensible IDLDictionary where additional key from the ones defined in the IDL could be put.
In the case of WebRTC, this does not seem to be worth it since the dictionary information is not persistent as would be case for Dictionary attributes for instance.

[Sam Weinig]

This still feels like a step backwards. We spent a lot of effort to remove all the uses of Dictionary from WebCore, and this feels like it will have many of the same problems (e.g. incorrect dealing with exceptions / gc lifetimes). 

Can you explain again why this can't be backed by a HashMap or if needs to be ordered, some other map, where type conversions happen at the boundary?

[youenn fablet]

(In reply to comment #26)
> This still feels like a step backwards. We spent a lot of effort to remove
> all the uses of Dictionary from WebCore, and this feels like it will have
> many of the same problems (e.g. incorrect dealing with exceptions / gc
> lifetimes). 

This patch is doing two things:
1. adding support for MapLike
2. adding support for stats API which uses a promise that resolves to a MapLike of extensible dictionaries

In this discussion, we should decouple the two.

For item 1, the maplike is stored as a slot to the JSXX object.
There should be no gc issues.
As of exceptions, they could happen when creating the backing map like and storing it as a slot. They could also happen when adding items. I can only imagine memory exception in both cases.
In our specific case, the only thing we could do would be to reject the promise with the given exception.

For item 2,we could use IDL dictionaries instead to build RTCStats items.
But this would be more code/less efficient/less flexible in that specific case.
We would create structures, fill them, convert them to JS, add them to map like and here we go.

> Can you explain again why this can't be backed by a HashMap or if needs to
> be ordered, some other map, where type conversions happen at the boundary?

WebIDL is defining maplike exactly as implemented in this patch: an object exposing data stored into a backing JS map.
To expose it properly as defined by WebIDL, we need to send back MapIterator objects to methods like values or entries. A MapIterator is bound to a JS Map.

The alternative I considered at some point was to use JS built-ins to feed the RTCStatsReport.
Instead of resolving a fed RTCStatsReport, we would resolve the promise with a vector containing the different RTCStats. The JS built-in would be responsible to fill the backing map slot with the provided vector.
I did not go this way since it is less generic and creating a temporary array is less efficient than directly adding items to the map. It is true though that the exception handling would be probably nicer.

wdyt?

[Sam Weinig]

(In reply to comment #27)
> (In reply to comment #26)
> > This still feels like a step backwards. We spent a lot of effort to remove
> > all the uses of Dictionary from WebCore, and this feels like it will have
> > many of the same problems (e.g. incorrect dealing with exceptions / gc
> > lifetimes). 
> 
> This patch is doing two things:
> 1. adding support for MapLike
> 2. adding support for stats API which uses a promise that resolves to a
> MapLike of extensible dictionaries
> 
> In this discussion, we should decouple the two.
> 
> For item 1, the maplike is stored as a slot to the JSXX object.
> There should be no gc issues.
> As of exceptions, they could happen when creating the backing map like and
> storing it as a slot. They could also happen when adding items. I can only
> imagine memory exception in both cases.

This is only true of a readonly maplike object. Once C++ land has to read values out of it, conversion exceptions can happen if I am not mistaken. 

> In our specific case, the only thing we could do would be to reject the
> promise with the given exception.
> 
> For item 2,we could use IDL dictionaries instead to build RTCStats items.
> But this would be more code/less efficient/less flexible in that specific
> case.
> We would create structures, fill them, convert them to JS, add them to map
> like and here we go.

This would be no different than all the rest of IDL infrastructure. A c++ backing, wrapped by a JavaScript object.

> 
> > Can you explain again why this can't be backed by a HashMap or if needs to
> > be ordered, some other map, where type conversions happen at the boundary?
> 
> WebIDL is defining maplike exactly as implemented in this patch: an object
> exposing data stored into a backing JS map.
> To expose it properly as defined by WebIDL, we need to send back MapIterator
> objects to methods like values or entries. A MapIterator is bound to a JS
> Map.
> 

Is any of that actually user observable, or is the spec just using the language of the ES spec to define the behavior. If for instance, modifications to the Map prototype effect the behavior of maplike objects, then it probably makes sense to implement it using the JS Map. But if not, I would like to stick to our normal patterns.

> The alternative I considered at some point was to use JS built-ins to feed
> the RTCStatsReport.
> Instead of resolving a fed RTCStatsReport, we would resolve the promise with
> a vector containing the different RTCStats. The JS built-in would be
> responsible to fill the backing map slot with the provided vector.
> I did not go this way since it is less generic and creating a temporary
> array is less efficient than directly adding items to the map. It is true
> though that the exception handling would be probably nicer.
> 
> wdyt?

I prefer keeping our pattern of c++ wrapped by js object for these.

[youenn fablet]

> This is only true of a readonly maplike object. Once C++ land has to read
> values out of it, conversion exceptions can happen if I am not mistaken. 

Right, I would expect WebCore binding code to actually enforce that key and value have the types defined by the maplike in the IDL.
The storage in the backing map would happen in the binding code and WebCore regular code would get notified of the converted C++ key/value.

> > In our specific case, the only thing we could do would be to reject the
> > promise with the given exception.
> > 
> > For item 2,we could use IDL dictionaries instead to build RTCStats items.
> > But this would be more code/less efficient/less flexible in that specific
> > case.
> > We would create structures, fill them, convert them to JS, add them to map
> > like and here we go.
> 
> This would be no different than all the rest of IDL infrastructure. A c++
> backing, wrapped by a JavaScript object.

In the RTCStatsReport specific case, the C++ structures (the dictionaries) would only last the time of the conversion.
In the case of an attribute getter returning a dictionary, the corresponding C++ object will probably be kept, updated and manipulated. This is not the case here, we would just create the C++ structure for the purpose of obtaining the corresponding JS object.

RTCStats are extensible, the WebIDL is not defining all fields provided by the WebRTC backend.
We could beef up WebKit IDLs to cover all possible dictionaries.

We would need to implement the marshalling of the WebRTC backend stats (a map) to the C++ structure which would be used to be converted to the dictionary.
Currently, we just have to handle the type of each stat field.

I do not see any advantage here.

> > 
> > > Can you explain again why this can't be backed by a HashMap or if needs to
> > > be ordered, some other map, where type conversions happen at the boundary?
> > 
> > WebIDL is defining maplike exactly as implemented in this patch: an object
> > exposing data stored into a backing JS map.
> > To expose it properly as defined by WebIDL, we need to send back MapIterator
> > objects to methods like values or entries. A MapIterator is bound to a JS
> > Map.
> > 
> 
> Is any of that actually user observable, or is the spec just using the
> language of the ES spec to define the behavior. If for instance,
> modifications to the Map prototype effect the behavior of maplike objects,
> then it probably makes sense to implement it using the JS Map. But if not, I
> would like to stick to our normal patterns.

This is indeed user observable, maplike would be affected by Map/Map Iterator prototype changes.

[Radar WebKit Bug Importer]

<rdar://problem/30293780>

[youenn fablet]

maplike and setlike being very close one from the other, I went trying to implement setlike in bug 159140. A similar design could be then applied to maplike.

[youenn fablet]

Created attachment 301985 [details]
Patch

[WebKit Commit Bot]

Attachment 301985 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 301986 [details]
Patch

[WebKit Commit Bot]

Attachment 301986 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 301990 [details]
Patch

[WebKit Commit Bot]

Attachment 301990 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:65:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 302021 [details]
Patch

[WebKit Commit Bot]

Attachment 302021 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 302030 [details]
Patch

[WebKit Commit Bot]

Attachment 302030 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 302032 [details]
Fixing GTK build

[WebKit Commit Bot]

Attachment 302032 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGuardedObject.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMMapLike.h:66:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 302032 [details]
Fixing GTK build

View in context: https://bugs.webkit.org/attachment.cgi?id=302032&action=review

Let's write a test verifying the overwriting of maplike prototype so we can compare it with the behavior of other browsers and change the test if we change our minds.

> Source/WebCore/Modules/mediastream/RTCStatsReport.h:41
> +

extra space

> Source/WebCore/bindings/scripts/test/TestMapLike.idl:26
> +    maplike<DOMString, DOMString>;

It would probably be a better test to have two different types to verify you got the right type in the right place.

[Sam Weinig]

Can you add tests that modify the Map prototype and show how it affects MapLike objects (for instance, replace the forEach function)?

[youenn fablet]

Here is the related WebIDL thread: https://github.com/heycam/webidl/issues/254
The spec currently describes what the current patch is implementing.
But the end goal is that maplike would be shielded from Map prototype changes.

Hum, I think I would prefer that maplike functions do not return any MapIterator.
That way, we could implement map like without any backing map.

[youenn fablet]

Created attachment 302895 [details]
Patch for landing

[youenn fablet]

(In reply to comment #47)
> Created attachment 302895 [details]
> Patch for landing

Patch does not include prototype change shielding.
I'll do that as a follow-up and will add tests for it.

[WebKit Commit Bot]

Comment on attachment 302895 [details]
Patch for landing

Clearing flags on attachment: 302895

Committed r213108: <http://trac.webkit.org/changeset/213108>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.