Closed Bug 1322274 Opened 8 years ago Closed 8 years ago

Overhaul PeerConnection.js with modern JavaScript

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(6 files)

Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise.
58 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state.
58 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1322274: Use destructuring in PeerConnection.js
58 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1322274: Use async/await in PeerConnection.js
58 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1322274: Use this._async() wrapper in PeerConnection.js for cleaner code
58 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1322274: Test sender.replaceTrack and other methods on close in parallel.
58 bytes, text/x-review-board-request
mt
: review+
Details 
In preparation of bug 1290948 and other work, I decided to modernize the RTCPeerConnection js code, with async/await and destructuring.
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.
Attachment #8817122 - Flags: review?(martin.thomson) → review+
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`.
Attachment #8817124 - Flags: review?(martin.thomson) → review+
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 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 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.
Attachment #8817292 - Flags: review?(martin.thomson)
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
Attachment #8817123 - Flags: review?(martin.thomson) → review+
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.
Attachment #8817292 - Flags: review?(martin.thomson) → review+
Comment on attachment 8817121 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for checking closed state.

https://reviewboard.mozilla.org/r/97564/#review97976
Attachment #8817121 - Flags: review?(martin.thomson) → review+
Comment on attachment 8817120 [details]
Bug 1322274: Make internal pc._legacyCatchAndCloseGuard responsible for returning content promise.

https://reviewboard.mozilla.org/r/97562/#review97978
Attachment #8817120 - Flags: review?(martin.thomson) → review+
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 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.
Rebased.
Pushed by jbruaroey@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: