Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Description • 7 years ago

We're removing stat.isRemote [1] in 58 (bug 1380555). This may break some sites who use isRemote rather than remoteId to locate remote stats.

Safe way (won't break):

      let stats = await pc.getStats();
      for (let stat of stats.values()) {
        if (stat.isRemote) continue;
        switch (stat.type) {
          case "outbound-rtp": {
            console.log(dumpOutbound(stat));
            let rtcp = stats.get(stat.remoteId); // <-- fine!
            if (rtcp) console.log("Remote " + dumpInbound(rtcp));
            break;
          }

Unsafe way (will break):

      let stats = await pc.getStats();
      for (let stat of stats.values()) {
        switch (stat.type) {
          case "outbound-rtp": {
            if (stat.isRemote) {                          // always undefined!
              console.log("Remote " + dumpInbound(stat)); // will never run!
            } else {
              console.log(dumpOutbound(stat));            // overwrites w/wrong stats!
            }
            break;
          }

We've found no good way to detect breakage after the fact, but it turns out we can warn people who will be affected in 57.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177156

I am leaving the review r? so I get notifications on changes. I have one comment on the warning message. Otherwise r+ with one nit.

::: dom/media/PeerConnection.js:513
(Diff revision 1)
>      this._warnDeprecatedStatsCallbacksNullable = { warn: () =>
>        this.logWarning("Callback-based pc.getStats is deprecated, and will be removed in the near future! Use promise-version! " +
>                        "See http://w3c.github.io/webrtc-pc/#getstats-example for usage.") };
>  
> +    this._warnDeprecatedStatsRemoteAccessNullable = { warn: () =>
> +      this.logWarning("Detected soon-to-break getStats() use! stat.isRemote goes away in Firefox 58! - Instead use stats.get(otherStat.remoteId). " +

The relation between stat.isRemote and stats.get(otherStat.remoteId) is not immediately clear. There is an unstated second change that makes this usage possible, which is that typenames are changing for the remote stats. Perhaps it would be better to only point to the blog post, where full context will be available?

::: dom/media/PeerConnection.js:513
(Diff revision 1)
>      this._warnDeprecatedStatsCallbacksNullable = { warn: () =>
>        this.logWarning("Callback-based pc.getStats is deprecated, and will be removed in the near future! Use promise-version! " +
>                        "See http://w3c.github.io/webrtc-pc/#getstats-example for usage.") };
>  
> +    this._warnDeprecatedStatsRemoteAccessNullable = { warn: () =>
> +      this.logWarning("Detected soon-to-break getStats() use! stat.isRemote goes away in Firefox 58! - Instead use stats.get(otherStat.remoteId). " +

You will probably get dinged by the linter for concating 2 literal strings, use multiline strings instead. e.g:
foo("a \
    bar");

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177312

::: dom/webidl/RTCStatsReport.webidl:200
(Diff revision 1)
>    [ChromeOnly]
>    readonly attribute DOMString mozPcid;
> +
> +  // Special internal method to help intercept Map.get() to detect proper
> +  // stats.get(otherStat.remoteId) use to turn off deprecation warnings.
> +  any warnGet(DOMString id);

What is this? Could we not add non-spec'ed stuff, pretty please.

::: dom/webidl/RTCStatsReport.webidl:205
(Diff revision 1)
> +  any warnGet(DOMString id);
> +};
> +
> +[Pref="media.peerconnection.enabled",
> + JSImplementation="@mozilla.org/dom/rtcstatsdeprecationwarningentry;1"]
> +interface RTCStatsDeprecationWarningEntry {

We don't want to add RTCStatsDeprecationWarningEntry to the global scope of all the pages.
Should this be ChromeOnly?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177312

> What is this? Could we not add non-spec'ed stuff, pretty please.

In order to avoid false deprecation warnings, this patch needs to intercept .get() on RTCStatsReport, which is a maplike interface, but our WebIDL bindings make it impossible to intercept .get() on a maplike object.

So, I had to hack around it. I define a new content-visible webidl method, and then swap it and .get() after creating it within chrome code, and before returning the object to content (in this patch):

    webidlobj.nativeGet = webidlobj.get;
    webidlobj.wrappedJSObject.get = webidlobj.wrappedJSObject.warnGet;

Not being a security expert, this seemed like the safest way to do this. The last thing I want to do is introduce any security issues in 57. Specifically, I wanted to avoid doing this: 

    webidlobj.wrappedJSObject.get = chromeobj.warnGet;

That said, I'm open to better ways of doing this.

Note that all of this code will be removed in 58. It's for one release only.

> We don't want to add RTCStatsDeprecationWarningEntry to the global scope of all the pages.
> Should this be ChromeOnly?

This most closely follows the existing procedure/trick we use to emit deprecation warnings for getStats(with, callbacks) in the surrounding code: we expose getters on a content-visible webidl object, and these getters in turn call a platform method.

In this case, the content-visible webidl object is the `entry` we put into the Map.

I first tried other approaches that avoided adding a new interface:

 1. I tried substituting the entry for a Proxy, but this gave me the following warning in web console:

    "Security wrapper denied access to property "get" on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported."

 2. I then tried using Cu.cloneInto (the other suggestion in the warning) on the internal entry, and putting getters on the result of that, but doing so gave me the following error:

    "Error: Not allowed to define accessor property on [Object] or [Array] XrayWrapper"

So step 3 is this patch where I temporarily create a content-visible webidl object, in order to get the corrrect bindings generated for safe access.

I'm hoping we can make an exception since this will all be removed again in 58.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 6 • 7 years ago

I've updated the other nits, and since getStats may get called a lot once it's called, I added an additional optimization to not intercept Map.get() if we've already given (up on) a warning.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177500

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177932

::: dom/media/PeerConnection.js:1615
(Diff revision 3)
> -    chromeobj.makeStatsPublic(pc._warnDeprecatedStatsCallbacksNullable &&
> -                              pc._warnDeprecatedStatsAccessNullable,
> +    chromeobj.makeStatsPublic(pc._warnDeprecatedStatsCallbacksNullable,
> +                              pc._warnDeprecatedStatsRemoteAccessNullable,
>                                pc._onGetStatsIsLegacy);
> +    webidlobj.nativeGet = webidlobj.get;
> +    if (pc._warnDeprecatedStatsRemoteAccessNullable.warn) {
> +      webidlobj.wrappedJSObject.get = webidlobj.wrappedJSObject.warnGet;

So which .get are you replacing? get is an own property of the prototype of the interface object.

::: dom/webidl/RTCStatsReport.webidl:204
(Diff revision 3)
> +  // stats.get(otherStat.remoteId) use to turn off deprecation warnings.
> +  any warnGet(DOMString id);
> +};
> +
> +[Pref="media.peerconnection.enabled",
> + JSImplementation="@mozilla.org/dom/rtcstatsdeprecationwarningentry;1"]

We don't want to add non-standard properties to the global scope. This won't even pass all the tests.

Why is this not [NoInterfaceObject] ?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177932

> So which .get are you replacing? get is an own property of the prototype of the interface object.

So what this code does is merely trump the prototype's get property with an own property on the object itself. This seems to work when I test it in typical cases, and I think I'm happy with doing this per-object, especially since in many cases (not all) we'll typically only do this for the first couple of returned stats objects, until the logic either warns once or drops warning based on identified stats gathering pattern on the part of the JS.

We're not trying to capture all cases, e.g. not RTCStatsReport.prototype.get.call (I did a search and found no hits in adapter.js).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review177932

> We don't want to add non-standard properties to the global scope. This won't even pass all the tests.
> 
> Why is this not [NoInterfaceObject] ?

That worked, thanks!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/171926/#review178312

I want to see the test before reviewing this.
And need to think if all that wrappedJSObject usage is safe.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 7 years ago

FWIW, I think bholley or someone should review the change too. It is so bizarre and scary.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 18 • 7 years ago

Hi bholley, I tried to mirror this on a similar hack we did in bug 1213056, so hopefully it is equally safe. Please ping me or ni? me if you have questions.

	Bobby Holley (:bholley)
	Comment 19 • 7 years ago

Comment on attachment 8900530 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

I'm pretty swamped at the moment. I think Peter can give an equivalent or better review.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8901900 [details]
Bug 1393306 - Test deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173324/#review178730

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8901900 [details]
Bug 1393306 - Test deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173324/#review178734

r+ I guess. (this all is still very weird. Is there really no simpler thing we could do?)

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 22 • 7 years ago

Sorry, I messed up my testing on [NoInterfaceObject]. Looks like that won't work, as the webidl compiler no longer generates the piece that I need. :-(

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 23 • 7 years ago

[ChromeOnly] was what I needed.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 27 • 7 years ago

So the last patch may be tricky. I had to move the warnGet() instance out of the instance, as it was messing up tests that were validating our legacy stats (which are own properties on the same instance). Instead, the get/warnGet switch is now done in the prototype on first access.

But, unless I also modify the prototype on the wrappedJSObject, it doesn't seem to take (that is: Map.get() gets called directly, and .warnGet() never gets called). Is this a maplike limitation? Is my workaround get safe? bz mentioned some alternative options on irc, if this doesn't work out.

	Peter Van der Beken [:peterv]
	Comment 28 • 7 years ago

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #27)
> But, unless I also modify the prototype on the wrappedJSObject, it doesn't
> seem to take (that is: Map.get() gets called directly, and .warnGet() never
> gets called). Is this a maplike limitation? Is my workaround get safe?

Isn't the problem that you're swapping the .get() of the Xray wrapper for RTCStatsReport's prototype? Shouldn't this all be happening on pc._win.RTCStatsReport.prototype.wrappedJSObject in the first place?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 7 years ago

Bug 1395421 adds a hook that should be usable here without having to munge anything page-visible, I think.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 36 • 7 years ago

Rebased and rolled up changes using maplike hook from bug 1395421 and removed warnGet webidl kludge.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 37 • 7 years ago

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

Since peterv reviewed Bug 1395421, I'll let him to review this one too.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 38 • 7 years ago
mozreview-review

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173466/#review181350

	Bulk Bug Changes for mreavy's org
	Comment 39 • 7 years ago

Mass change P1->P2 to align with new Mozilla triage process

	Peter Van der Beken [:peterv]
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173466/#review181220

I'll just r+ this, given that you didn't let me know if the alternative approach I suggested to you works.

::: dom/media/PeerConnection.js:349
(Diff revision 4)
> +            }, entry)
> +          };
> +        }
> +        Object.defineProperties(entry.wrappedJSObject, warnProps);
> +        entry._isRemote = stat.isRemote;
> +        internal = entry;

As I said, I'd prefer to do something like

        let entry = Cu.createObjectIn(this._win);
        let stat = internal;
        for (let key in stat) {
          Object.defineProperty(entry, key, {
            enumerable: true, configurable: false,
            get: Cu.exportFunction(function() {
              // Warn on remote stat access other than the recommended approach of
              //
              // for (let stat of stats.values()) {
              //   switch (stat.type) {
              //     case "outbound-rtp": {
              //       if (stat.isRemote) continue;
              //       let rtcp = stats.get(stat.remoteId);
              //
              if (warnRemoteNullable.warn && stat.isRemote &&
                  key != "type" &&
                  key != "isRemote") {
                // id is first prop, a sign of JSON.stringify(), cancel warnings.
                if (key != "id") {
                  warnRemoteNullable.warn();
                }
                warnRemoteNullable.warn = null;
              }
              return stat[key];
            }, entry)});
        }
        Cu.unwaiveXrays(entry)._isRemote = stat.isRemote;
        internal = entry;

::: dom/media/PeerConnection.js:1610
(Diff revision 4)
>    onGetStatsSuccess(dict) {
>      let pc = this._dompc;
> -    let chromeobj = new RTCStatsReport(pc._win, dict);
> +    let chromeobj = new RTCStatsReport(pc, dict);
>      let webidlobj = pc._win.RTCStatsReport._create(pc._win, chromeobj);
> -    chromeobj.makeStatsPublic(pc._warnDeprecatedStatsCallbacksNullable &&
> -                              pc._warnDeprecatedStatsAccessNullable,
> +    chromeobj.makeStatsPublic(pc._warnDeprecatedStatsCallbacksNullable,
> +                              pc._warnDeprecatedStatsRemoteAccessNullable,

Shouldn't you remove _warnDeprecatedStatsAccessNullable completely, I don't see any other place where it's used?

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 41 • 6 years ago

Adding a deprecation warning to use of the isRemote stat on inbound-rtp and outbound-rtp.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 44 • 6 years ago

Rebased.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 47 • 6 years ago
mozreview-review-reply

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173466/#review181220

> As I said, I'd prefer to do something like
> 
>         let entry = Cu.createObjectIn(this._win);
>         let stat = internal;
>         for (let key in stat) {
>           Object.defineProperty(entry, key, {
>             enumerable: true, configurable: false,
>             get: Cu.exportFunction(function() {
>               // Warn on remote stat access other than the recommended approach of
>               //
>               // for (let stat of stats.values()) {
>               //   switch (stat.type) {
>               //     case "outbound-rtp": {
>               //       if (stat.isRemote) continue;
>               //       let rtcp = stats.get(stat.remoteId);
>               //
>               if (warnRemoteNullable.warn && stat.isRemote &&
>                   key != "type" &&
>                   key != "isRemote") {
>                 // id is first prop, a sign of JSON.stringify(), cancel warnings.
>                 if (key != "id") {
>                   warnRemoteNullable.warn();
>                 }
>                 warnRemoteNullable.warn = null;
>               }
>               return stat[key];
>             }, entry)});
>         }
>         Cu.unwaiveXrays(entry)._isRemote = stat.isRemote;
>         internal = entry;

Thanks, I've incorporated this verbatim.

Doing so meant giving up on the RTCStatsDeprecationWarningEntry class, the basis for the test (which only tested this observable side-effect).

Plan is to land without test. Code is for two releases only. Have confirmed behavior locally.

> Shouldn't you remove _warnDeprecatedStatsAccessNullable completely, I don't see any other place where it's used?

I removed the wrong one by mistake here. _warnDeprecatedStatsAccessNullable is the one to use here. Now fixed. Thanks for catching this!

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 48 • 6 years ago

Gave up on test.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 50 • 6 years ago
mozreview-review

Comment on attachment 8997633 [details]
Bug 1393306 - Pick Firefox 65 as new isRemote end-date.

https://reviewboard.mozilla.org/r/261330/#review268408

LGTM

	Code Review Bot [:reviewbot]
	Comment 51 • 6 years ago
mozreview-review

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173466/#review268416


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/PeerConnection.js:390
(Diff revision 5)
> +class RTCStatsDeprecationWarningEntry {
> +}
> +setupPrototype(RTCStatsDeprecationWarningEntry, {
> +  classID: PC_STATS_WARN_CID,
> +  contractID: PC_STATS_WARN_CONTRACT,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports]),

Error: Please use ChromeUtils.generateQI instead of XPCOMUtils.generateQI [eslint: mozilla/use-chromeutils-generateqi]

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 52 • 6 years ago
mozreview-review-reply

Comment on attachment 8902056 [details]
Bug 1393306 - Add deprecation warning in 57 for removal of stat.isRemote in 58.

https://reviewboard.mozilla.org/r/173466/#review268416

> Error: Please use ChromeUtils.generateQI instead of XPCOMUtils.generateQI [eslint: mozilla/use-chromeutils-generateqi]

This code is no longer in the patch. Filed https://github.com/mozilla/release-services/issues/1346

	Pulsebot
	Comment 53 • 6 years ago

Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/425a0334587b
Add deprecation warning in 57 for removal of stat.isRemote in 58. r=ng,peterv
https://hg.mozilla.org/integration/autoland/rev/68b0a51e3908
Pick Firefox 65 as new isRemote end-date. r=ng

	Daniel Varga [:dvarga]
	Comment 54 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/425a0334587b
https://hg.mozilla.org/mozilla-central/rev/68b0a51e3908

	Kohei Yoshino [:kohei]
	Comment 55 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/rtcrtpstreamstats-isremote-has-been-deprecated/

	Eric Shepherd [:sheppy]
	Comment 56 • 6 years ago

The addition of this warning and a mention of the upcoming removal of isRemote has been added to Firefox 63 for developers. RTCRtpStreamStats has not yet been documented, and documenting it is out of scope for the purposes of this small item, but I hope to get to stats documentation before much longer.