1322274 - Overhaul PeerConnection.js with modern JavaScript

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

In preparation of bug 1290948 and other work, I decided to modernize the RTCPeerConnection js code, with async/await and destructuring.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise.

Review commit: https://reviewboard.mozilla.org/r/97562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97562/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state.

Review commit: https://reviewboard.mozilla.org/r/97564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97564/

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Use destructuring in PeerConnection.js

Review commit: https://reviewboard.mozilla.org/r/97566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97566/

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Use async/await in PeerConnection.js

Review commit: https://reviewboard.mozilla.org/r/97568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97568/

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

Review commit: https://reviewboard.mozilla.org/r/97570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97570/

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8817122 [details]
Bug 1322274: Use destructuring in PeerConnection.js

https://reviewboard.mozilla.org/r/97566/#review97882

::: dom/media/PeerConnection.js:574
(Diff revision 1)
>     *   msg - Error message to detail which array-entry failed, if any.
>     */
> -  _mustValidateRTCConfiguration: function(rtcConfig, msg) {
> +  _mustValidateRTCConfiguration: function({ iceServers }, msg) {
>  
>      // Normalize iceServers input
> -    rtcConfig.iceServers.forEach(server => {
> +    iceServers.forEach(server => {

Do we need a check for whether iceServers is null/undefined?

::: dom/media/PeerConnection.js:601
(Diff revision 1)
> -      if (!server.urls) {
> +      if (!urls) {
>          throw new this._win.DOMException(msg + " - missing urls", "InvalidAccessError");
>        }
> -      server.urls.forEach(urlStr => {
> -        let url = nicerNewURI(urlStr);
> -        if (url.scheme in { turn:1, turns:1 }) {
> +      urls.map(url => nicerNewURI(url)).forEach(({ scheme, spec }) => {
> +        if (scheme in { turn:1, turns:1 }) {
> +          if (username == undefined) {

!username instead?  It seems like null is also problematic.

::: dom/media/PeerConnection.js:605
(Diff revision 1)
> -        if (url.scheme in { turn:1, turns:1 }) {
> -          if (server.username == undefined) {
> +          if (username == undefined) {
> +            throw new this._win.DOMException(msg + " - missing username: " + spec,
> -            throw new this._win.DOMException(msg + " - missing username: " + urlStr,
>                                               "InvalidAccessError");
>            }
> -          if (server.credential == undefined) {
> +          if (credential == undefined) {

as above

::: dom/media/PeerConnection.js:785
(Diff revision 1)
>          AppConstants.MOZ_B2G ||
>          Services.prefs.getBoolPref("media.navigator.permission.disabled")) {
>        return this._havePermission = Promise.resolve();
>      }
> -    return this._havePermission = new Promise((resolve, reject) => {
> -      this._settlePermission = { allow: resolve, deny: reject };
> +    return this._havePermission = new Promise((allow, deny) => {
> +      this._settlePermission = { allow, deny };

BTW, I find this little "optimization" repulsive.

::: dom/media/PeerConnection.js:809
(Diff revision 1)
> +  setLocalDescription: function({ type, sdp }, onSuccess, onError) {
>      return this._legacyCatchAndCloseGuard(onSuccess, onError, () => {
> -      this._localType = desc.type;
> +      this._localType = type;
>  
> -      let type;
> -      switch (desc.type) {
> +      let action = this._actions[type];
> +      if (action === undefined) {

!action maybe

::: dom/media/PeerConnection.js:911
(Diff revision 1)
>              this._onSetRemoteDescriptionSuccess = resolve;
>              this._onSetRemoteDescriptionFailure = reject;
> -            this._impl.setRemoteDescription(type, desc.sdp);
> +            this._impl.setRemoteDescription(action, sdp);
>            })).then(() => { this._updateCanTrickle(); });
>  
> -        if (desc.type === "rollback") {
> +        if (type == "rollback") {

Did you want to use action instead?

::: dom/media/PeerConnection.js:1280
(Diff revision 1)
> -    if (candidate == "") {
> -      this.foundIceCandidate(null);
> -    } else {
> -      this.foundIceCandidate(new this._dompc._win.RTCIceCandidate(
> +    this.foundIceCandidate((candidate || null) &&
> +                           new this._dompc._win.RTCIceCandidate({ candidate,
> +                                                                  sdpMid,
> +                                                                  sdpMLineIndex }));

I don't like this change.  It's much less clear.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

https://reviewboard.mozilla.org/r/97570/#review97896

::: dom/media/PeerConnection.js:528
(Diff revision 1)
>      // don't propagate errors in the operations chain (this is a fork of p).
>      this._operationsChain = p.catch(() => {});
>      return await p;
>    },
>  
> -  // This wrapper helps implement legacy callbacks in a manner that produces
> +  // These wrappers helps implement legacy callbacks in a manner that produces

These wrappers help

::: dom/media/PeerConnection.js:552
(Diff revision 1)
> +  _closeWrapper: async function(func) {
> +    let closed = this._closed;
>      try {
> -      return this._win.Promise.resolve(operation())
> -        .then(this._wrapLegacyCallback(onSuccess),
> -              this._wrapLegacyCallback(onError));
> +      let result = await func();
> +      if (!closed && this._closed) {
> +        await new Promise(() => {});

Do we need to await an empty promise?  The reason we had these before was that we had a ternary operator.

Is there a need for an extra dispatch?  If so, why?

::: dom/media/PeerConnection.js:572
(Diff revision 1)
> +      try {
> +        onErr && onErr(e);
> +      } catch (e) {
> +        this.logErrorAndCallOnError(e);
> +      }

You could refactor the try { onXXX && onXXX(v); } catch (e) { this._logErrorAndCallOnError(e); } thing.

::: dom/media/PeerConnection.js:1059
(Diff revision 1)
>  
>    _getDTMFToneBuffer: function(sender) {
>      return this._impl.getDTMFToneBuffer(sender.__DOM_IMPL__);
>    },
>  
> -  _replaceTrack: function(sender, withTrack) {
> +  _replaceTrack: async function(sender, withTrack) {

Does this need to call `_checkClosed` too?

::: dom/media/PeerConnection.js:1193
(Diff revision 1)
> +  _getStats: async function(selector) {
> +    return await this._chain(() => new Promise((resolve, reject) => {

Might want to comment that this specifically DOES NOT include a call to `_checkClosed`.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817122 [details]
Bug 1322274: Use destructuring in PeerConnection.js

https://reviewboard.mozilla.org/r/97566/#review97882

> Do we need a check for whether iceServers is null/undefined?

Though far from obvious, it's invariant because of code before it. [1] We have tests for it thankfully.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/media/PeerConnection.js#393,397,404,409

> !username instead?  It seems like null is also problematic.

Can't do that because "" is a valid username.

Also, username is a non-nullable string in WebIDL, so { username: null } becomes { username: "null" }, which is fine ("fine" meaning what the spec says).

> as above

Same thing here.

> BTW, I find this little "optimization" repulsive.

OK removed.

> !action maybe

This is one of the darker corners of JavaScript...

Ci.IPeerConnection.kActionOffer is 0, therefore we must not ignore it as a valid value distinct from absence [1]. Who knew, an actual real reason in js to prefer inline literals over named pre-defined constants elsewhere.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/media/bridge/IPeerConnection.idl#38

> Did you want to use action instead?

Yes thanks!

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

https://reviewboard.mozilla.org/r/97570/#review97896

> Do we need to await an empty promise?  The reason we had these before was that we had a ternary operator.
> 
> Is there a need for an extra dispatch?  If so, why?

Yes because unlike Promise.resolve(), these are actual "empty promises" in that they'll never ever resolve. We need await in order to hang.

(Of course it doesn't really "hang" since we're in syntactic sugarland here, but we don't want the chain (if we stare through the illusion) to resume. It may look a bit scary but it shouldn't be if I understand things correctly. The fact that nothing holds on to the resolve function of the empty promise should ensure it gets garbage collected eventually, and any async function closure with it I hope.)

> You could refactor the try { onXXX && onXXX(v); } catch (e) { this._logErrorAndCallOnError(e); } thing.

You mean bring back _wrapLegacyCallback, or something else?

> Does this need to call `_checkClosed` too?

Yes! Good catch.

Makes me wonder about setParameters too. Filed https://github.com/w3c/webrtc-pc/issues/965

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

https://reviewboard.mozilla.org/r/97570/#review97896

> You mean bring back _wrapLegacyCallback, or something else?

OK I think I have it.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.

Review commit: https://reviewboard.mozilla.org/r/97640/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97640/

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817122 [details]
Bug 1322274: Use destructuring in PeerConnection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97566/diff/1-2/

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817123 [details]
Bug 1322274: Use async/await in PeerConnection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97568/diff/1-2/

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97570/diff/1-2/

Comment 15

[Martin Thomson [:mt:]]

Comment on attachment 8817123 [details]
Bug 1322274: Use async/await in PeerConnection.js

https://reviewboard.mozilla.org/r/97568/#review97918

::: dom/media/PeerConnection.js:450
(Diff revision 1)
>      _globalPCList.addPC(this);
>  
>      this._impl.initialize(this._observer, this._win, rtcConfig,
>                            Services.tm.currentThread);
> -    this._initCertificate(rtcConfig.certificates);
> +
> +    let certificate, { certificates = [] } = rtcConfig;

two lines please

::: dom/media/PeerConnection.js:482
(Diff revision 1)
>  
>    getConfiguration: function() {
>      return this._config;
>    },
>  
> -  _initCertificate: function(certificates = []) {
> +  _initCertificate: async function(certificate) {

I'm not sure that I like this change.  The point of the function was to isolate some of the certificate-related work and now the function does basically nothing.

::: dom/media/PeerConnection.js:723
(Diff revision 1)
>        options = optionsOrOnSuccess;
>      }
> -    return this._legacyCatchAndCloseGuard(onSuccess, onError, () => {
> +    return this._legacyCatchAndCloseGuard(onSuccess, onError, async () => {
>        let origin = Cu.getWebIDLCallerPrincipal().origin;
> -      return this._chain(() => {
> -        let p = Promise.all([this._getPermission(), this._certificateReady])
> +      return await this._chain(async () => {
> +        let haveAssertion = this._localIdp.enabled && this._getIdentityAssertion(origin);

I know you like writing code like this, but && is not a substitute for an if statement.

::: dom/media/PeerConnection.js:762
(Diff revision 1)
> -            }
> +        }
> -            if (this.remoteDescription.type != "offer") {
> +        if (this.remoteDescription.type != "offer") {
> -              throw new this._win.DOMException("No outstanding offer",
> +          throw new this._win.DOMException("No outstanding offer",
> -                                               "InvalidStateError");
> +                                           "InvalidStateError");
> -            }
> +        }
> +        let haveAssertion = this._localIdp.enabled && this._getIdentityAssertion(origin);

as above

::: dom/media/PeerConnection.js:784
(Diff revision 1)
> -        AppConstants.MOZ_B2G ||
> -        Services.prefs.getBoolPref("media.navigator.permission.disabled")) {
> +      this._havePermission = privileged ? Promise.resolve()
> +                                        : new Promise((allow, deny) => {

yuck

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8817292 [details]
Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.

https://reviewboard.mozilla.org/r/97640/#review97974

::: dom/media/tests/mochitest/head.js:537
(Diff revision 1)
> +function getBlackTrack({width = 640, height = 480} = {}) {
> +  let canvas = Object.assign(document.createElement("canvas"), {width, height});
> +  canvas.getContext('2d').fillRect(0, 0, width, height);
> +  let stream = canvas.captureStream();
> +  return Object.assign(stream.getVideoTracks()[0], {enabled: false});
> +}

You don't seem to be using this here.

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8817121 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state.

https://reviewboard.mozilla.org/r/97564/#review97976

Comment 18

[Martin Thomson [:mt:]]

Comment on attachment 8817120 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise.

https://reviewboard.mozilla.org/r/97562/#review97978

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817123 [details]
Bug 1322274: Use async/await in PeerConnection.js

https://reviewboard.mozilla.org/r/97568/#review97918

> I'm not sure that I like this change.  The point of the function was to isolate some of the certificate-related work and now the function does basically nothing.

Ok I'll move the code back in.

> I know you like writing code like this, but && is not a substitute for an if statement.

I do, but two if's coming up.

> yuck

if-else coming up.

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817292 [details]
Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.

https://reviewboard.mozilla.org/r/97640/#review97974

> You don't seem to be using this here.

I do, in the replaceTrack call. Code is from https://blog.mozilla.org/webrtc/warm-up-with-replacetrack/

I put them in head.js because I'm hoping to convert more tests over to use them soon. Specifically, those tests that today use fake cams and mics more as convenient and independent audio and video sources. 

This becomes important with bug 1088621 where I'm moving fake cams and mics to behave more like shared hardware cams and mics, which doesn't play well with some tests.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817123 [details]
Bug 1322274: Use async/await in PeerConnection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97568/diff/2-3/

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97570/diff/2-3/

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817292 [details]
Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97640/diff/1-2/

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817120 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97562/diff/1-2/

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817121 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97564/diff/1-2/

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817122 [details]
Bug 1322274: Use destructuring in PeerConnection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97566/diff/2-3/

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817123 [details]
Bug 1322274: Use async/await in PeerConnection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97568/diff/3-4/

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817124 [details]
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97570/diff/3-4/

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8817292 [details]
Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97640/diff/2-3/

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Rebased.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42e13e249e20
Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise. r=mt
https://hg.mozilla.org/integration/autoland/rev/e09ad423360b
Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state. r=mt
https://hg.mozilla.org/integration/autoland/rev/a5336dc0dedc
Use destructuring in PeerConnection.js r=mt
https://hg.mozilla.org/integration/autoland/rev/02d05b00f0ad
Use async/await in PeerConnection.js r=mt
https://hg.mozilla.org/integration/autoland/rev/d5447e1a4829
Use this._async() wrapper in PeerConnection.js for cleaner code r=mt
https://hg.mozilla.org/integration/autoland/rev/887e43c3474f
Test sender.replaceTrack and other methods on close in parallel. r=mt

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/42e13e249e20
https://hg.mozilla.org/mozilla-central/rev/e09ad423360b
https://hg.mozilla.org/mozilla-central/rev/a5336dc0dedc
https://hg.mozilla.org/mozilla-central/rev/02d05b00f0ad
https://hg.mozilla.org/mozilla-central/rev/d5447e1a4829
https://hg.mozilla.org/mozilla-central/rev/887e43c3474f