1187206 - JavaScript error: resource://gre/modules/media/PeerConnectionIdp.jsm, line 103: TypeError: sdp is null

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

JavaScript error: resource://gre/modules/media/PeerConnectionIdp.jsm, line 103: TypeError: sdp is null

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Sounds like martin

Comment 2

[Maire Reavy [:mreavy]]

Martin -- How important is this to fix "soon"?  I've made this a P2 (which means we'd target fixing it this quarter or next at latest).  If you think it needs to get fixed sooner than this or can wait until later, please comment.  

I realize this may be a trivial fix; so we may just want to do it.  For now I'm just trying to get a priority read.  Thanks.

Comment 3

[Martin Thomson [:mt:]]

Ack.  It's trivial, but I have almost zero time.

It's not a big deal, because it is an error in the program, the only real consequence is that we fail in a non-graceful fashion.

Comment 4

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

Bug 1187206 - Adding null checks to non-rollback SDP, r?jib

Comment 5

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

Comment on attachment 8643329 [details]
MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

https://reviewboard.mozilla.org/r/15061/#review13513

There's still a problem for rollback, e.g.:

    var desc = new mozRTCSessionDescription();
    desc.type = "rollback";
    desc.sdp = null;
    new mozRTCPeerConnection().setRemoteDescription(desc);

::: dom/media/PeerConnection.js:760
(Diff revision 1)
> +        throw new this._win.DOMException(
> +            "Empty or null SDP provided to setLocalDescription",
> +          "InvalidParameterError");

Uneven indent

::: dom/media/PeerConnection.js:842
(Diff revision 1)
> +      if (desc.type !== "rollback" && !desc.sdp) {
> +        throw new this._win.DOMException(
> +            "Empty or null SDP provided to setRemoteDescription",
> +          "InvalidParameterError");
> +      }

same here

::: dom/media/PeerConnection.js:759
(Diff revision 1)
> +      if (desc.type !== "rollback" && !desc.sdp) {

Nits: != preferred. I would put !desc.sdp first, the rarer condition.

::: dom/media/PeerConnection.js:761
(Diff revision 1)
> +            "Empty or null SDP provided to setLocalDescription",

Why is desc.sdp nullable again?

Comment 6

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/15061/#review13513

Right.  Thanks.

> Why is desc.sdp nullable again?

type == "rollback" might be the immediate answer, but the real answer is a long series of bad API design choices

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8643329 [details]
MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

Bug 1187206 - Adding checks for null SDP, r?jib

Comment 8

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1191180 - Wait for certificate before generating identity assertion, r=jib

Bug 1191180 - Wait for certificate before generating identity assertion, r?jib

Comment 9

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

Comment on attachment 8643329 [details]
MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

https://reviewboard.mozilla.org/r/15061/#review13565

::: dom/media/PeerConnection.js:858
(Diff revisions 1 - 2)
> +        // Do setRemoteDescription and identity validation in parallel
> +        if (desc.type !== "rollback") {
> +          var validId = this._validateIdentity(desc.sdp, origin);
> +          return this._win.Promise.all([setRem, validId])
> +            .then(() => {}); // must return undefined
> +        }
> +        return setRem;
> +      });

I find this a little hard to follow. I think a bail pattern:

    if (desc.type == "rollback") {
      return setSem;
    }
    // Do setRemoteDescription and identity validation in parallel

would help me.

::: dom/media/PeerConnection.js:860
(Diff revisions 1 - 2)
> +          var validId = this._validateIdentity(desc.sdp, origin);
> +          return this._win.Promise.all([setRem, validId])

Nit: I would stick to let or var, or inline validId.

Comment 10

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

Comment on attachment 8643777 [details]
MozReview Request: Bug 1191180 - Wait for certificate before generating identity assertion, r=jib

https://reviewboard.mozilla.org/r/15137/#review13571

::: dom/media/PeerConnection.js:621
(Diff revision 1)
> -  _addIdentityAssertion: function(p, origin) {
> +  _addIdentityAssertion: function(sdpPromise, origin) {
>      if (this._localIdp.enabled) {
> -      return this._localIdp.getIdentityAssertion(this._impl.fingerprint, origin)
> -        .then(() => p)
> -        .then(sdp => this._localIdp.addIdentityAttribute(sdp));
> +      return Promise.all([
> +        this._certificateReady
> +          .then(() => this._localIdp.getIdentityAssertion(this._impl.fingerprint,
> +                                                          origin)),
> +        sdpPromise
> +      ]).then(result => this._localIdp.addIdentityAttribute(result[1]));
>      }
> -    return p;
> +    return sdpPromise;

Nit: in hindsight, a bail-pattern would probably help here too (and shave off an indent).

::: dom/media/PeerConnection.js:628
(Diff revision 1)
> +      ]).then(result => this._localIdp.addIdentityAttribute(result[1]));

You could use destructuring here:

    ]).then(([, sdp]) => this._localIdp.addIdentityAttribute(sdp));

Note that ([]) is needed, as just [] wont work. I complained on es-discuss about this a while ago, and they may fix it in es7.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8643329 [details]
MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

Bug 1187206 - Adding checks for null SDP, r=jib

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8643777 [details]
MozReview Request: Bug 1191180 - Wait for certificate before generating identity assertion, r=jib

Bug 1191180 - Wait for certificate before generating identity assertion, r=jib

Comment 13

[Martin Thomson [:mt:]]

Belay that, I've confused this enough; save the sheriffs.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da37ed115e8a

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/da37ed115e8a