Closed Bug 1187206 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
Blocking Flags:

People

(Reporter: jruderman, Assigned: mt)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(3 files)

Attached file 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
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)
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)
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
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)
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
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+
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
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
Keywords: checkin-needed
Belay that, I've confused this enough; save the sheriffs.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da37ed115e8a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.