Add deprecation warning for removal of stat.isRemote in 65.

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
Rank:
19
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: jib, Assigned: jib)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → jib

Comment 2

2 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");

Comment 3

2 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?
Attachment #8900530 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

2 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.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 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.
Comment hidden (mozreview-request)

Comment 8

2 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
Attachment #8900530 - Flags: review?(na-g) → review+

Comment 9

2 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] ?
Attachment #8900530 - Flags: review?(bugs) → review-

Updated

2 years ago
Rank: 19
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 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).
(Assignee)

Comment 12

2 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!

Comment 13

2 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.
Attachment #8900530 - Flags: review?(bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
FWIW, I think bholley or someone should review the change too. It is so bizarre and scary.
(Assignee)

Updated

2 years ago
Attachment #8900530 - Flags: review?(bugs) → review?(bobbyholley)
(Assignee)

Comment 18

2 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.
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.
Attachment #8900530 - Flags: review?(bobbyholley) → review?(peterv)

Comment 20

2 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
Attachment #8901900 - Flags: review?(na-g) → review+

Comment 21

2 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?)
Attachment #8901900 - Flags: review?(bugs) → review+
(Assignee)

Comment 22

2 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. :-(
(Assignee)

Comment 23

2 years ago
[ChromeOnly] was what I needed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

2 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.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(jib)
Comment hidden (mozreview-request)
Bug 1395421 adds a hook that should be usable here without having to munge anything page-visible, I think.
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8900530 - Attachment is obsolete: true
Attachment #8900530 - Flags: review?(peterv)
Attachment #8900530 - Flags: review?(bugs)
(Assignee)

Comment 36

2 years ago
Rebased and rolled up changes using maplike hook from bug 1395421 and removed warnGet webidl kludge.
Flags: needinfo?(jib)
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.
Attachment #8902056 - Flags: review?(bugs)

Comment 38

2 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
Attachment #8902056 - Flags: review?(na-g) → review+
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2

Comment 40

2 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?
Attachment #8902056 - Flags: review?(peterv) → review+
(Assignee)

Updated

2 years ago
Summary: Add deprecation warning in 57 for removal of stat.isRemote in 58. → Add deprecation warning in 58 for removal of stat.isRemote in 59.
Adding a deprecation warning to use of the isRemote stat on inbound-rtp and outbound-rtp.

Updated

9 months ago
Summary: Add deprecation warning in 58 for removal of stat.isRemote in 59. → Add deprecation warning for removal of stat.isRemote in 65.
(Assignee)

Updated

9 months ago
Attachment #8996794 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

9 months ago
Rebased.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

9 months 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!
(Assignee)

Updated

9 months ago
Attachment #8901900 - Attachment is obsolete: true
(Assignee)

Comment 48

9 months ago
Gave up on test.
Comment hidden (mozreview-request)

Comment 50

9 months 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
Attachment #8997633 - Flags: review?(na-g) → review+

Comment 51

9 months 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]
(Assignee)

Comment 52

9 months 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

Comment 53

9 months 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

Comment 54

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/425a0334587b
https://hg.mozilla.org/mozilla-central/rev/68b0a51e3908
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Attachment #8996794 - Attachment is obsolete: false

Updated

5 months ago
Blocks: 1512517
You need to log in before you can comment on or make changes to this bug.