Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2489673003:
RTCPeerConnection.getStats: Whitelist of stats in unittest. (Closed)
|
Created:
4 years, 1 month ago by hbos_chromium Modified:
4 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, phoglund+watch_chromium.org, mcasas+watch+vc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRTCPeerConnection.getStats: Whitelist of stats in unittest.
The promise-based getStats automatically translates WebRTC layer stats
to JavaScript dictionary objects. The whitelist is meant to prevent
leaking stats to the web platform that don't have an associated Chromium
CL explicitly allowing it to be exposed (by updating the whitelist).
The whitelist describes RTCStats-derived dictionary objects according to
the spec[1]. A stats object returned by getStats must have a known
"type" and any member it has must exist in the whitelist with the
correct type.
Note that this only prevents stats from being exposed to the web that
would be returned by getStats in the scenario tested for in this
unittest. In a future CL we will make sure that ALL stats in the
whitelist are returned by the test to ensure sufficient test coverage
and update the unittest if that is not the case.
[1] https://w3c.github.io/webrtc-stats/
BUG=627816
Committed: https://crrev.com/4a9a590c838590a32b0a9cf5112ea2fb615a8f58
Cr-Commit-Position: refs/heads/master@{#434471}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Using JSON instead of semicolon separated string list #Patch Set 3 : Updated a comment #
Total comments: 6
Patch Set 4 : Addressed nits #
Messages
Total messages: 32 (14 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hbos@chromium.org changed reviewers: + hta@chromium.org, phoglund@chromium.org
Please take a look, hta and phoglund
Please take a look, hta and phoglund
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a minor comment. https://codereview.chromium.org/2489673003/diff/20001/chrome/test/data/webrtc... File chrome/test/data/webrtc/peerconnection_getstats.js (right): https://codereview.chromium.org/2489673003/diff/20001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:254: returnToTest('ok-' + iterableToSemicolonList(statsTypes.values())); I'd recommend using JSON.stringify here instead of rolling your own text serialization mechanism. I believe there are nice JSON parsing utilities in Chrome's c++ code base you can use from the browser test. For an example, see https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection.j....
PTAL phoglund, hta. https://codereview.chromium.org/2489673003/diff/20001/chrome/test/data/webrtc... File chrome/test/data/webrtc/peerconnection_getstats.js (right): https://codereview.chromium.org/2489673003/diff/20001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:254: returnToTest('ok-' + iterableToSemicolonList(statsTypes.values())); On 2016/11/09 15:50:10, phoglund_chrome wrote: > I'd recommend using JSON.stringify here instead of rolling your own text > serialization mechanism. I believe there are nice JSON parsing utilities in > Chrome's c++ code base you can use from the browser test. > > For an example, see > https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/peerconnection.j.... Done. (The referenced JSON is being returned to C++ only to be passed as a string back to JavaScript which does JSON.parse. But we do have JSON readers in the C++ layer used by other tests, updated the code to use base::JSONReader::Read and friends.)
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Note: We'll still want to have a whitelist filter, removing non-whitelisted stats on the way from webrtc to blink. Having a filter would be enough to ensure we don't leak stats to the web without a unittest I suppose, but testing-wise it's probably a good idea to have a test that explicitly makes sure not the wrong stats are leaked - and in the future ensures all stats dictionaries are included in the integration test - even though we'll end up having two whitelists for the same stats (unittest & filter)...
lgtm https://codereview.chromium.org/2489673003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_browsertest_base.cc (right): https://codereview.chromium.org/2489673003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_browsertest_base.cc:124: std::vector<std::string> JsonArrayToVectorOfStrings( Ok, I think this ended up being a wash (I hoped you'd get less code in total), but I think this is much safer.
Please take a look, hta. https://codereview.chromium.org/2489673003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/webrtc_browsertest_base.cc (right): https://codereview.chromium.org/2489673003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/webrtc_browsertest_base.cc:124: std::vector<std::string> JsonArrayToVectorOfStrings( On 2016/11/10 09:01:39, phoglund_chrome wrote: > Ok, I think this ended up being a wash (I hoped you'd get less code in total), > but I think this is much safer. :)
Hm. I'm starting to think this is genuinely hard. The easiest place to see hardness is in peerconnection_getstats.js, where you define a third representation of stats (the first one being the spec's WebIDL, the second one being the C++ in the implementation). This is neither easy to read or easy to maintain. So it isn't a compelling piece of evidence to show to the API maintainers to say "we're not going to publish any new stats without a proper launch approval". Another approach, which might actually be less code, would be: - Create a test containing the actual IDL from the spec. Example here: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/imported/... - Run a call to generate some stats objects - Use the testidl functions to tell that the stats objects actually match the spec That gets rid of one layer of spec-rewriting. But I'm not sure the IDL processor is actually powerful enough to check against extra members in a dictionary. It seems hard to get something that is workable, maintainable, checks the right things, and is compelling to others.
I don't think it supports testing dictionary members at all? https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... And I see it's used for layout tests, not browser tests, but maybe it can be reused. Something like this would be great, but I'm wondering about the cost vs benefit of using this if it needs to be worked on.
On 2016/11/18 11:08:26, hbos_chromium wrote: > I don't think it supports testing dictionary members at all? > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > And I see it's used for layout tests, not browser tests, but maybe it can be > reused. > > Something like this would be great, but I'm wondering about the cost vs benefit > of using this if it needs to be worked on. Suggest you put this aside for now and concentrate on the stats. I think more thinking is required in order to figure out what the properties required of a solution are here.
On 2016/11/19 04:52:40, hta - Chromium wrote: > On 2016/11/18 11:08:26, hbos_chromium wrote: > > I don't think it supports testing dictionary members at all? > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > > And I see it's used for layout tests, not browser tests, but maybe it can be > > reused. > > > > Something like this would be great, but I'm wondering about the cost vs > benefit > > of using this if it needs to be worked on. > > Suggest you put this aside for now and concentrate on the stats. I think more > thinking is required in order to figure out what the properties required of a > solution are here. hta: Are we sure we don't want to go with this? I think it is pretty similar to the rtcstats_integrationtest.cc. I could create a CL that makes sure we get all dictionaries we expect without checking individual members instead, which would be easier to read and maintain, but that is less robust and risks leaking non-whitelisted members of whitelisted dictionaries.
On 2016/11/24 13:12:24, hbos_chromium wrote: > On 2016/11/19 04:52:40, hta - Chromium wrote: > > On 2016/11/18 11:08:26, hbos_chromium wrote: > > > I don't think it supports testing dictionary members at all? > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > > > And I see it's used for layout tests, not browser tests, but maybe it can be > > > reused. > > > > > > Something like this would be great, but I'm wondering about the cost vs > > benefit > > > of using this if it needs to be worked on. > > > > Suggest you put this aside for now and concentrate on the stats. I think more > > thinking is required in order to figure out what the properties required of a > > solution are here. > > hta: Are we sure we don't want to go with this? I think it is pretty similar to > the rtcstats_integrationtest.cc. > I could create a CL that makes sure we get all dictionaries we expect without > checking individual members instead, which would be easier to read and maintain, > but that is less robust and risks leaking non-whitelisted members of whitelisted > dictionaries. Or I could look into what is required to make idlharness.js support dictionaries.
Alternative: https://codereview.chromium.org/2530873002/
lgtm (with misgivings about the maintenance burden). note - CLs that modify this test are still not Blink CLs. So please update the description to not mention Blink. https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... File chrome/test/data/webrtc/peerconnection_getstats.js (right): https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:264: * Returns to test a complete list of whitelisted "RTCStats.type" values as a Nit: "Returns to test" is not grammatical. You probably can get away with just "returns", or "returns a complete list .... to the test". https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:278: } Isn't this supposed to be "this = Array.from(parent) or some equivalent? I remember copying over object properties is a handsome mess in Javascript; you get all sorts of exceptional cases.
Description was changed from ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Blink CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 ========== to ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Chromium CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 ==========
https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... File chrome/test/data/webrtc/peerconnection_getstats.js (right): https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:264: * Returns to test a complete list of whitelisted "RTCStats.type" values as a On 2016/11/24 16:12:14, hta - Chromium wrote: > Nit: "Returns to test" is not grammatical. You probably can get away with just > "returns", or "returns a complete list .... to the test". > Done. https://codereview.chromium.org/2489673003/diff/60001/chrome/test/data/webrtc... chrome/test/data/webrtc/peerconnection_getstats.js:278: } On 2016/11/24 16:12:14, hta - Chromium wrote: > Isn't this supposed to be "this = Array.from(parent) or some equivalent? > I remember copying over object properties is a handsome mess in Javascript; you > get all sorts of exceptional cases. > Done /w Object.assign(). Array.from would turn "this" into an array? I think completely trying to copy an object, including "prototype" stuff, is problematic, but just copying properties of an object used as a property map, like we do, is totally fine. (I'm not a JavaScript expert.) We can use Object.assign() though, that's cleaner.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@chromium.org, hta@chromium.org Link to the patchset: https://codereview.chromium.org/2489673003/#ps80001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480065830668000,
"parent_rev": "9c672c6067d4e05b13f65ea3877e1f04675ead94", "commit_rev":
"1a5d180230760807fb056a9665d93e80b401c132"}
Message was sent while issue was closed.
Description was changed from ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Chromium CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 ========== to ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Chromium CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Chromium CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 ========== to ========== RTCPeerConnection.getStats: Whitelist of stats in unittest. The promise-based getStats automatically translates WebRTC layer stats to JavaScript dictionary objects. The whitelist is meant to prevent leaking stats to the web platform that don't have an associated Chromium CL explicitly allowing it to be exposed (by updating the whitelist). The whitelist describes RTCStats-derived dictionary objects according to the spec[1]. A stats object returned by getStats must have a known "type" and any member it has must exist in the whitelist with the correct type. Note that this only prevents stats from being exposed to the web that would be returned by getStats in the scenario tested for in this unittest. In a future CL we will make sure that ALL stats in the whitelist are returned by the test to ensure sufficient test coverage and update the unittest if that is not the case. [1] https://w3c.github.io/webrtc-stats/ BUG=627816 Committed: https://crrev.com/4a9a590c838590a32b0a9cf5112ea2fb615a8f58 Cr-Commit-Position: refs/heads/master@{#434471} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4a9a590c838590a32b0a9cf5112ea2fb615a8f58 Cr-Commit-Position: refs/heads/master@{#434471} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
