1111666 - RTCPeerConnection should throw and fail with properly named errors (use DOMException)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Currently, the Error.name property is clobbered to "Error" by Bug 1107592. The solution in that bug was to expose a DOMException constructor that PeerConnection should use for exceptions and callbacks. This bug covers changing PeerConnection to use DOMException.

Comment 1

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

Attachment: Have peerConnection throw DOMException.

Comment 2

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

Try showing Bug 1111679: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0a8eacaa9a2d

JavaScript error: http://mochi.test:8888/tests/dom/media/tests/mochitest/test_peerConnection_close.html, line 82: InvalidStateError: Peer connection is closed

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8536700 [details] [diff] [review]
Have peerConnection throw DOMException.

Review of attachment 8536700 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +394,5 @@
> +    this._checkClosed();
> +    // No need to checkClosed inside the promise, since the c++ code - which is
> +    // driving the chain - stops calling us once we're closed. Spec behavior as
> +    // of this writing is to NOT reject outstanding promises on close.
> +    let p = this._taskChain.then(() => func());

Do you want to see if we can resolve on the reject issue before closing this out?

@@ +1068,5 @@
>        "IncompatibleMediaStreamTrackError",
>        "InternalError"
>      ];
>      let name = reasonName[Math.min(code, reasonName.length - 1)];
> +    return new this._dompc._win.DOMException(message, name);

This bothers me a little.  I assume that once we remove the whole observer thing, these will originate in C++ land properly?

Otherwise, I think that I'd be inclined to ask for a new WebIDL enum.

Comment 4

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

(In reply to Martin Thomson [:mt] from comment #3)
> > +    this._checkClosed();
> > +    // No need to checkClosed inside the promise, since the c++ code - which is
> > +    // driving the chain - stops calling us once we're closed. Spec behavior as
> > +    // of this writing is to NOT reject outstanding promises on close.
> > +    let p = this._taskChain.then(() => func());
> 
> Do you want to see if we can resolve on the reject issue before closing this
> out?

What reject issue? Se spec. ;-)

Happy to, though this particular line removes what should be a no-op, since the c++ code shouldn't call on us in closed-state. On the off chance that it were to, the new behavior here of succeeding is actually closer to how Firefox works historically, since before this patch it queued close.

That is of course terrible behavior, so we should probably do as you suggest and check explicitly for closed in the observer and either a) squelch it, or b) reject the chain, depending on the outcome of our discussion. Though the spec seems fairly clear to me that we should squelch it...

> @@ +1068,5 @@
> >        "IncompatibleMediaStreamTrackError",
> >        "InternalError"
> >      ];
> >      let name = reasonName[Math.min(code, reasonName.length - 1)];
> > +    return new this._dompc._win.DOMException(message, name);
> 
> This bothers me a little.  I assume that once we remove the whole observer
> thing, these will originate in C++ land properly?
> 
> Otherwise, I think that I'd be inclined to ask for a new WebIDL enum.

This is just a refactor of existing terrible code.

Comment 5

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

(In reply to Martin Thomson [:mt] from comment #3)
> I assume that once we remove the whole observer
> thing, these will originate in C++ land properly?

I've filed Bug 1111742 for that, though I'm unsure it would be a win.

Comment 6

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

Attachment: Part 1: Have peerConnection throw DOMException (2) r=mt

Put the _checkClosed() call back where it was before, as the new location was problematic. Patch is otherwise unchanged. Carrying forward r=mt.

Comment 7

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

Attachment: Part 2: peerConnection legacy error-callbacks weren't catching exception

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1d7a923cee3d

Comment 8

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

Attachment: Part 3: refactor setRemoteDescription to use promises and DOMException

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8537604 [details] [diff] [review]
Part 3: refactor setRemoteDescription to use promises and DOMException

Review of attachment 8537604 [details] [diff] [review]:
-----------------------------------------------------------------

I had to be careful here, because it didn't seem possible to replace that much code with so little.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8536812 [details] [diff] [review]
Part 2: peerConnection legacy error-callbacks weren't catching exception

Review of attachment 8536812 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +663,5 @@
>              "Invalid type " + desc.type + " provided to setRemoteDescription",
>              "InvalidParameterError");
>      }
> +    return new this._win.Promise((resolve, reject) =>
> +      this._setRemoteDescriptionImpl(type, desc.sdp, origin, resolve, reject));

Why can't _setRemoteDescriptionImpl return its own promise?

Comment 11

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

(In reply to Martin Thomson [:mt] from comment #10)
> Why can't _setRemoteDescriptionImpl return its own promise?

Part 3, remember? ;-)

Comment 12

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

(In reply to Martin Thomson [:mt] from comment #9)
> I had to be careful here, because it didn't seem possible to replace that
> much code with so little.

Thanks! Quite proud of that one.

Comment 13

[Martin Thomson [:mt:]]

Landing plan?  I would like this relatively soon.

Comment 14

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

It's blocked on DOMException (Bug 1107592 and Bug 1107953).

Comment 15

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

Attachment: Part 2: peerConnection legacy error-callbacks weren't catching exception (2) r=mt

Updated commit msg. Carrying forward r=mt.

Comment 16

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

Attachment: Part 1: Have peerConnection throw DOMException (3)

Fixes error line-number in test per Bug 1107953 comment 20.

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8550449 [details] [diff] [review]
Part 1: Have peerConnection throw DOMException (3)

Review of attachment 8550449 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it.

::: dom/media/PeerConnection.js
@@ +832,2 @@
>      }
>      if (stream.getTracks().indexOf(track) == -1) {

Nit: don't use '==', I prefer '< 0' or '>= 0' for indexOf

@@ +1059,5 @@
>    __init: function(dompc) {
>      this._dompc = dompc._innerObject;
>    },
>  
> +  newError: function(message, code) {

It strikes me that you could have saved some pain by keeping the argument order...

Comment 18

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

Attachment: Part 1: Have peerConnection throw DOMException (4) r=mt

Comment 19

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

Comment on attachment 8550468 [details] [diff] [review]
Part 1: Have peerConnection throw DOMException (4) r=mt

Incorporate feedback. Carrying forward r=mt.

Comment 20

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

Attachment: Part 2: peerConnection legacy error-callbacks weren't catching exception (3) r=mt

Rebase.

Comment 21

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

Attachment: Part 2: peerConnection legacy error callbacks working with line numbers.

Had to redo part 2 to generate correct line-numbers in errors.

Comment 22

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

Attachment: Part 3: refactor setRemoteDescription to use promises and DOMException (2) r=mt

Rebased. Carrying forward r=mt.

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8551618 [details] [diff] [review]
Part 2: peerConnection legacy error callbacks working with line numbers.

Review of attachment 8551618 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +416,5 @@
> +      return func().then(this._wrapLegacyCallback(onSuccess),
> +                         this._wrapLegacyCallback(onError));
> +    } catch (e) {
> +      this._wrapLegacyCallback(onError)(e);
> +      return Promise.resolve(); // avoid webidl TypeError

this._win.Promise() or we are likely to get odd problems in content-land (I think)

Comment 24

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

Attachment: Part 2: peerConnection legacy error callbacks working with line numbers (2) r=mt

Incorporate feedback. Carrying forward r=mt.

Comment 25

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

Attachment: Part 2: peerConnection legacy error callbacks working with line numbers (2) r=mt

Reorder patches (no change).

Comment 26

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

Green try (red one is gUM, and we're not touching gUM here)
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8665f780307c

Comment 27

[Martin Thomson [:mt:]]

Yeah, some day I'll try to get that gUM thing fixed, but last time I looked it was in the MSG morass and I'm not familiar enough with that to make progress.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fef95e82103
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f34b1c7ebbb
https://hg.mozilla.org/integration/mozilla-inbound/rev/54209f8c0911

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fef95e82103
https://hg.mozilla.org/mozilla-central/rev/7f34b1c7ebbb
https://hg.mozilla.org/mozilla-central/rev/54209f8c0911