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

RESOLVED FIXED in Firefox 42

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: mt)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla42
testcase
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8638361 [details]
testcase

JavaScript error: resource://gre/modules/media/PeerConnectionIdp.jsm, line 103: TypeError: sdp is null
Sounds like martin
Flags: needinfo?(martin.thomson)
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.
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
(Assignee)

Comment 3

3 years ago
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.
Assignee: nobody → martin.thomson
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 4

3 years ago
Created attachment 8643329 [details]
MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib

Bug 1187206 - Adding null checks to non-rollback SDP, r?jib
Attachment #8643329 - Flags: review?(jib)
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?
Attachment #8643329 - Flags: review?(jib)
(Assignee)

Comment 6

3 years ago
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
(Assignee)

Updated

3 years ago
Attachment #8643329 - Attachment description: MozReview Request: Bug 1187206 - Adding null checks to non-rollback SDP, r?jib → MozReview Request: Bug 1187206 - Adding checks for null SDP, r?jib
Attachment #8643329 - Flags: review?(jib)
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 8

3 years ago
Created 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
Attachment #8643777 - Flags: review?(jib)
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.
Attachment #8643329 - Flags: review?(jib) → review+
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.
Attachment #8643777 - Flags: review?(jib) → review+
(Assignee)

Comment 11

3 years ago
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
Attachment #8643329 - Attachment description: MozReview Request: Bug 1187206 - Adding checks for null SDP, r?jib → MozReview Request: Bug 1187206 - Adding checks for null SDP, r=jib
(Assignee)

Comment 12

3 years ago
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
Attachment #8643777 - Attachment description: MozReview Request: Bug 1191180 - Wait for certificate before generating identity assertion, r?jib → MozReview Request: Bug 1191180 - Wait for certificate before generating identity assertion, r=jib
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

3 years ago
Belay that, I've confused this enough; save the sheriffs.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da37ed115e8a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.