Closed Bug 1111666 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect)

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(3 files, 9 obsolete files)

21.14 KB, patch
jib
: review+
Details | Diff | Splinter Review
18.06 KB, patch
jib
: review+
Details | Diff | Splinter Review
5.73 KB, patch
jib
: review+
Details | Diff | Splinter Review
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.
Depends on: 1111679
Attachment #8536700 - Flags: review?(martin.thomson)
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.
Attachment #8536700 - Flags: review?(martin.thomson) → review+
(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.
(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.
Depends on: 1107953
No longer depends on: 1111679
Put the _checkClosed() call back where it was before, as the new location was problematic. Patch is otherwise unchanged. Carrying forward r=mt.
Attachment #8536700 - Attachment is obsolete: true
Attachment #8536810 - Flags: review+
Attachment #8536810 - Attachment description: Have peerConnection throw DOMException (2) r=mt → Part 1: Have peerConnection throw DOMException (2) r=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.
Attachment #8537604 - Flags: review?(martin.thomson) → review+
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?
Attachment #8536812 - Flags: review?(martin.thomson) → review+
(In reply to Martin Thomson [:mt] from comment #10)
> Why can't _setRemoteDescriptionImpl return its own promise?

Part 3, remember? ;-)
(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.
Landing plan?  I would like this relatively soon.
It's blocked on DOMException (Bug 1107592 and Bug 1107953).
Updated commit msg. Carrying forward r=mt.
Attachment #8536812 - Attachment is obsolete: true
Attachment #8550350 - Flags: review+
Fixes error line-number in test per Bug 1107953 comment 20.
Attachment #8536810 - Attachment is obsolete: true
Attachment #8550449 - Flags: review?(martin.thomson)
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...
Attachment #8550449 - Flags: review?(martin.thomson) → review+
Comment on attachment 8550468 [details] [diff] [review]
Part 1: Have peerConnection throw DOMException (4) r=mt

Incorporate feedback. Carrying forward r=mt.
Attachment #8550468 - Flags: review+
Attachment #8550449 - Attachment is obsolete: true
Had to redo part 2 to generate correct line-numbers in errors.
Attachment #8550469 - Attachment is obsolete: true
Attachment #8551618 - Flags: review?(martin.thomson)
Rebased. Carrying forward r=mt.
Attachment #8537604 - Attachment is obsolete: true
Attachment #8551619 - Flags: review+
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)
Attachment #8551618 - Flags: review?(martin.thomson) → review+
Incorporate feedback. Carrying forward r=mt.
Attachment #8551618 - Attachment is obsolete: true
Attachment #8551800 - Flags: review+
Reorder patches (no change).
Attachment #8551619 - Attachment is obsolete: true
Attachment #8551802 - Flags: review+
Green try (red one is gUM, and we're not touching gUM here)
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8665f780307c
Keywords: checkin-needed
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.
You need to log in before you can comment on or make changes to this bug.