Closed Bug 1322503 Opened 8 years ago Closed 7 years ago

Firefox's RTCStatsType is not spec-compatible (missing hyphens in most enum names)

Categories

(Core :: WebRTC, defect, P1)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jib, Assigned: ng)

References

(
URL
)

Details

Attachments

(3 files)

Bug 1322503 - Hyphenate rtc stats type as per spec.
58 bytes, text/x-review-board-request
jib
: review+
Details
results-20161219-115622.json
226.40 KB, text/plain
Details
results-20161219-121717.json
39.71 KB, text/plain
Details 
Firefox has:
  "inboundrtp",
  "outboundrtp",
  "candidatepair",
  "localcandidate",
  "remotecandidate"

Spec says [1]:
  "inbound-rtp",
  "outbound-rtp",
  "track",
  "transport",
  "candidate-pair",
  "local-candidate",
  "remote-candidate",

We should update to the spec asap as Chrome is near shipping their spec stats (it's behind a flag now).

This is a compat issue unfortunately, as the primary way we encourage people to read stats is to enumerate records until a matching type is found.

FF returns a hybrid object today that's both maplike and has legacy native properties doubled up (which we probably should get rid of soon).

I propose we only fix (break) the maplike version to be spec compatible, since it's my understanding very few people actually use the maplike syntax yet (though I have no data to back that up). Those who've jumped on the maplike syntax have self-selected themselves as people who jump on things early, so they should hopefully be able to react quickly again. Unfair, I know, but life etc.

[1] https://github.com/w3c/webrtc-stats/issues/5

PS: Separately from this bug, fippo is planning a fix for adapter.js that will add the hyphens in for older firefox, which should give us some us breathing room for the sub-population that actively updates adapter (it'll likely be a major version, so sites have some control of when they want to take the hit of updating to listen to new names. Recall: adapter only polyfills the spec on older browsers, not vice versa, so anyone using maplike syntax with adapter would need to update their code to match the spec at the same time).
https://github.com/webrtc/adapter/pull/392 at your service. If you already know a fixversion where this is no longer required but this wont affect stats with the new type.
Rank: 15
Priority: -- → P1
Assignee: nobody → na-g
Comment on attachment 8819251 [details]
Bug 1322503 - Hyphenate rtc stats type as per spec.

https://reviewboard.mozilla.org/r/99096/#review99418

Lgtm with some nits and optional suggestions.

::: dom/media/PeerConnection.js:315
(Diff revision 1)
> +    let specToLegacyFieldMapping = specFieldName => {
> +       let map = {'inbound-rtp' : 'inboundrtp',
> +        'outbound-rtp':'outboundrtp',
> +        'candidate-pair':'candidatepair',
> +        'local-candidate':'localcandidate',
> +        'remote-candidate':'remotecandidate'};
> +      return map[specFieldName] || specFieldName;
> +    };

No need to create this static map on every getStats, when we could make it a member of the pc, but maybe I'm micro-optimizing. Then again it's not unusual to call getStats repeatedly on a high frequency.

Also, note I'm biased against single-use functions, preferring inlining, so I tend to point these out as I do fairly opinionated JS reviews.

::: dom/media/tests/mochitest/pc.js:1582
(Diff revision 1)
>              ok(res.bytesSent >= res.packetsSent, "Rtp bytesSent");
>            } else {
>              ok(res.packetsReceived !== undefined, "Rtp packetsReceived");
>              ok(res.bytesReceived >= res.packetsReceived, "Rtp bytesReceived");
>            }
> -          if (res.remoteId) {
> +          if (res.remoteId) { 

trailing whitespace

::: dom/media/tests/mochitest/pc.js:1625
(Diff revision 1)
> -    is(JSON.stringify(counters), JSON.stringify(counters2),
> -       "Spec and legacy variant of RTCStatsReport enumeration agree");
> +    isnot(JSON.stringify(counters), JSON.stringify(counters2),
> +       "Spec and legacy variant of RTCStatsReport enumeration disagree");

To preserve this test, it might be worthwhile to do a reverse map here on the type. Otherwise we're no longer testing that all the other legacy stats match.

Sorry I forgot we already had this test in pc.js when I suggested a new test.

FWIW I'd be happy with adding a map to this all-or-nothing JSON.stringify compare instead of moving it to a new test.

I remember the reason now that I put it in pc.js which is that it's hard for a single test to provoke full coverage of the stats, as one often needs to wait up to 5 seconds before rtcp fires, and who knows how long on android test machines.

The new test added in this patch tests individual properties, which may be be overkill given that all legacy stats get created from a single algorithm.

::: dom/media/tests/mochitest/pc.js:1633
(Diff revision 1)
> -    ok((counters.inboundrtp || 0) >= nin, "Have at least " + nin + " inboundrtp stat(s) *");
> +    ok((counters["inbound-rtp"] || 0) >= nin, "Have at least " + nin + " inbound-rtp stat(s) *");
>  
> -    is(counters.outboundrtp || 0, nout, "Have " + nout + " outboundrtp stat(s)");
> +    is(counters["outbound-rtp"] || 0, nout, "Have " + nout + " outbound-rtp stat(s)");
>  
> -    var numLocalCandidates  = counters.localcandidate || 0;
> -    var numRemoteCandidates = counters.remotecandidate || 0;
> +    var numLocalCandidates  = counters["local-candidate"] || 0;
> +    var numRemoteCandidates = counters["remote-candidate"] || 0;

This change seems optional, but fine. Pity about the uglier syntax.

::: dom/media/tests/mochitest/test_peerConnection_mapLikeToLegacyStatsMapping.html:24
(Diff revision 1)
> +        'local-candidate':'localcandidate',
> +         'remote-candidate':'remotecandidate'};
> +
> +    var checkMapping = (stats, statId) => {
> +      // Spec compliant to legacy name map
> +      let map = (typeName) => mapping[typeName] || typename;

Redundant () parens

::: dom/media/tests/mochitest/test_peerConnection_mapLikeToLegacyStatsMapping.html:42
(Diff revision 1)
> +      function PC_LOCAL_CHECK_LEGACY_STATS_MAPPING(test) {
> +        var checked = [];
> +        test.pcLocal.getStats().then(stats => {
> +          // Test each received stat block
> +          for (let [key, value] of stats) {
> +            checkMapping(stats, key);

I would inline all nested single-use functions, as I find it shrinks the code and makes it more readable without the indirections. Reduces maintenance bugs too IMHO.

For example: `stats.get(statId)` becomes redundant once inlined, since you already have `value`.

YMMV. Though see my other comment on maybe not keeping this test.
Attachment #8819251 - Flags: review?(jib) → review+
I have attached results-20161219-115622.json which is a list of page/resource URLs in httparchive:har.2016_11_15_chrome_requests_bodies containing both 'RTCPeerConnection' and 'getStats' and one of the non-hyphenated strings listed in comment #0. results-20161219-121717.json is the same but excluding a lot of uninteresting results from intercomcdn.com.

A lot of the results in one of these:
https://www.twilio.com/docs/api/client/twilio-js-14
https://github.com/webrtc/adapter
https://tokbox.com/developer/sdks/js/
http://www.airbnb.*

I didn't analyse it exhaustively, but what I looked at fell into one of two categories:
1. UA sniffing, and assuming 'hyphenless' names in Firefox and 'googCamelCase' in Chrome, with no fallback to a per-spec variant.
2. The same but checking both 'hyphenless' and 'googCamelCase' but with no UA checks.

Note that adapter.js has matches, but that's trying to map 'hyphenless' to 'hyphen-less'.

Switching to 'hyphen-less' in all contexts is thus likely to cause regressions for Firefox, but AFAICT these are using the callback-based getStats, so changing only the promise-based variant probably works.

If, based on this data, you see risks in changing to hyphenated names, please shout ASAP because then dropping the hyphens from the spec could be a quicker and safer path to interop.
https://webtorrent.io/bundle.js is just a single case, but interesting nonetheless:

    items.forEach(function (item) {
      var isCandidatePair = (
        (item.type === 'googCandidatePair' && item.googActiveConnection === 'true') ||
        (item.type === 'candidatepair' && item.selected)
      )
      if (isCandidatePair) setActiveCandidates(item)
    })

There's no UA sniffing here, but it will break if one switches the callback-based getStats to internally be an alias of the promise-based one.
Thanks Philip! As I point out in [1] I would guess few of these use both the promise API and maplike. Do you happen to know? In this bug we're only switching the maplike version.

(In reply to Philip Jägenstedt from comment #12)
> https://webtorrent.io/bundle.js is just a single case, but interesting
> nonetheless:
> 
>     items.forEach(function (item) {
>       var isCandidatePair = (
>         (item.type === 'googCandidatePair' && item.googActiveConnection ===
> 'true') ||
>         (item.type === 'candidatepair' && item.selected)
>       )
>       if (isCandidatePair) setActiveCandidates(item)
>     })
> 
> There's no UA sniffing here, but it will break if one switches the
> callback-based getStats to internally be an alias of the promise-based one.

This one is interesting, since Firefox has supported `forEach` since the beginning, long before it was a part of the maplike pattern, so these guys will actually be affected by this patch.

Nico, based on this, would it make sense to not add hyphens if the legacy callback API is used?

OTOH, if we work too hard not to break people I worry they'll never upgrade, and the non-spec approach wins. :-(

[1] https://github.com/w3c/webrtc-stats/issues/116#issuecomment-268052150
Flags: needinfo?(na-g)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)
> Thanks Philip! As I point out in [1] I would guess few of these use both the
> promise API and maplike. Do you happen to know? In this bug we're only
> switching the maplike version.
> 
> (In reply to Philip Jägenstedt from comment #12)
> > https://webtorrent.io/bundle.js is just a single case, but interesting
> > nonetheless:
> > 
> >     items.forEach(function (item) {
> >       var isCandidatePair = (
> >         (item.type === 'googCandidatePair' && item.googActiveConnection ===
> > 'true') ||
> >         (item.type === 'candidatepair' && item.selected)
> >       )
> >       if (isCandidatePair) setActiveCandidates(item)
> >     })
> > 
> > There's no UA sniffing here, but it will break if one switches the
> > callback-based getStats to internally be an alias of the promise-based one.
> 
> This one is interesting, since Firefox has supported `forEach` since the
> beginning, long before it was a part of the maplike pattern, so these guys
> will actually be affected by this patch.
> 
> Nico, based on this, would it make sense to not add hyphens if the legacy
> callback API is used?

Jan-Ivar, my initial reaction is no.

My leaning would to move towards spec compliant usage, and to create as few exceptions as possible. I think that spec consistency in the new map-like interface is more important than maintaining the legacy interface, and that the overlap is unfortunate.

That said, I do not have a feel for how much developer pain it will cause.

Jib, do we have a bug for the deprecation of the legacy interface?

> 
> OTOH, if we work too hard not to break people I worry they'll never upgrade,
> and the non-spec approach wins. :-(
> 
> [1] https://github.com/w3c/webrtc-stats/issues/116#issuecomment-268052150
Flags: needinfo?(na-g) → needinfo?(jib)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)
> Thanks Philip! As I point out in [1] I would guess few of these use both the
> promise API and maplike. Do you happen to know? In this bug we're only
> switching the maplike version.

I haven't paid much attention to the changes to maplike and what it was like before. To be clear, do you mean that only the promise-based getStats uses a maplike RTCStatsReport, and that this is the only API you're intending to make hyphenated?

In the examples I looked at, I didn't see any using the promise-vending getStats(), although I certainly made no effort to exhaustively check all cases. If I were making the change, I'd certainly give it better odds to change just the promise-based API rather than trying to change both, or having both hypenated and hyphenless duplicates in the same report.

> (In reply to Philip Jägenstedt from comment #12)
> > https://webtorrent.io/bundle.js is just a single case, but interesting
> > nonetheless:
> > 
> >     items.forEach(function (item) {
> >       var isCandidatePair = (
> >         (item.type === 'googCandidatePair' && item.googActiveConnection ===
> > 'true') ||
> >         (item.type === 'candidatepair' && item.selected)
> >       )
> >       if (isCandidatePair) setActiveCandidates(item)
> >     })
> > 
> > There's no UA sniffing here, but it will break if one switches the
> > callback-based getStats to internally be an alias of the promise-based one.
> 
> This one is interesting, since Firefox has supported `forEach` since the
> beginning, long before it was a part of the maplike pattern, so these guys
> will actually be affected by this patch.
> 
> Nico, based on this, would it make sense to not add hyphens if the legacy
> callback API is used?

To make sure that my guesses aren't totally off, you may also want to try visiting some of these sites with an instrumented build to see what code paths are actually being hit. You get lots of dead code in httparchive searches, so the number of sites that actually break could be much lower.

> OTOH, if we work too hard not to break people I worry they'll never upgrade,
> and the non-spec approach wins. :-(
> 
> [1] https://github.com/w3c/webrtc-stats/issues/116#issuecomment-268052150

The status quo is certainly not acceptable for an interop standpoint, so worrying to the point of making no changes would be bad indeed. Given how things look, however, it seems pretty hopeless to gradually migrate the callback-based getStats be more like the new, so it could be that the best path forward is to minimize changes to the old API, while improving the new one and going out to find users of the old getStats and helping them migrate.

(This is an approach I'd normally not favor at all, but aliasing here really doesn't seem workable.)
Oh, and if you want me to search for any specific patterns in httparchive to help you make a better judgement, I'd be happy to help. In January, because now I'll be gone :)
(In reply to Philip Jägenstedt from comment #15)
> I haven't paid much attention to the changes to maplike and what it was like
> before. To be clear, do you mean that only the promise-based getStats uses a
> maplike RTCStatsReport, and that this is the only API you're intending to
> make hyphenated?

Firefox returns a hybrid object that is both maplike and has old-spec object properties defined directly on it as well. The two sets can be seen and compared here: https://jsfiddle.net/sc3dh9wr/

The patch here hyphenates the map entries, but not the old-spec duplicates on the same object. The old callback methods return the same object as the promise method (thanks spec).

I thought only callers using both promises and maplike were using forEach, but your webtorrent.io example proves otherwise. Turns out forEach is both new (a feature of Maps) and exceedingly old stats spec. You know how it is, people cut'n'paste what works.

Skipping the hyphenation entirely when the callback methods are called would cause less breakage.
Flags: needinfo?(jib)
Comment on attachment 8819251 [details]
Bug 1322503 - Hyphenate rtc stats type as per spec.

https://reviewboard.mozilla.org/r/99096/#review102552

r+ for the webidl change, but please think of backwards compatibility here.
Perhaps this should land the same time as bug 1328440
Attachment #8819251 - Flags: review+
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/10841f767eaf
Hyphenate rtc stats type as per spec. r=jib,smaug
https://hg.mozilla.org/mozilla-central/rev/10841f767eaf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi folks, I'm the maintainer of WebTorrent and simple-peer, which is where the https://webtorrent.io/bundle.js example you're using actually comes from. I stumbled upon this issue while updated simple-peer to use the new promise-based getStats() API.

Just wanted to weigh in and say that I'm okay with you breaking our code if it simplifies things for you. For new web features that were introduced recently, I think backwards compatibility matters less than getting things right.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: