1091898 - Update WebRTC with promises

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Attachment: add RTCPeerConnection hybrid with promises.

I needed a place to put patches for upcoming w3c work.

Comment 1

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

Attachment: Simple PC test

Comment 2

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

Attachment: add RTCPeerConnection hybrid with promises (2) WIP

Comment 3

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

Attachment: make Idp code in setRemoteDescription work with promises WIP

Comment 4

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

Attachment: Part 1: return DOMError rather than string in peerConnection callbacks

Comment 5

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

Part 1 try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3fe30219e52d

Comment 6

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

Attachment: Part 2: add RTCPeerConnection hybrid with promises (3)

Comment 7

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

Attachment: Part 3: make Idp code in setRemoteDescription work with promises (2)

I keep getting this error in setRemoteDescription with promises:

From https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d819becd929e
> 6 INFO TEST-UNEXPECTED-FAIL
> | /tests/dom/media/tests/mochitest/identity/test_fingerprints.html
> | error setting the session description: Component returned failure code:
> 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.getWebIDLCallerPrincipal] - expected PASS

Any tips about why this would happen is appreciated, as I'm stumped.

I ended up updating the Idp code to use promises, thinking this would help somehow, but no luck. I think it turned out well though, so here it is.

Comment 8

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

bholley, martin says you know about getWebIDLCallerPrincipal(). See comment 7. Any help appreciated.

Comment 9

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

Here's the spec PR.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Any tips about why this would happen is appreciated, as I'm stumped.

Did you try breakpointing in getWebIDLCallerPrincipal in this test and seeing what the JS stack looks like?

I'm _guessing_ the issue is that you're calling it async somehow at this point and then callerPrin of course ends up false.  So breakpoint on that error return and see what the callstack is.

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8522548 [details] [diff] [review]
Part 2: add RTCPeerConnection hybrid with promises (3)

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

I'd like to see the state variable stuff landed separately.

::: dom/media/PeerConnection.js
@@ +383,5 @@
> +            }
> +          };
> +        });
> +      });
> +    });

gah

@@ +421,5 @@
>      if (callback) {
>        this._win.setTimeout(() => {
>          try {
> +          if (typeof callback !== "function") {
> +            this.logErrorAndCallOnError("hi", "fi", "9");

I don't know what to make of this.

@@ +602,1 @@
>            Object.keys(o).length != (o.optional? 2 : 1)) {

This needs to be clearer.  It's very complex.

@@ +649,5 @@
> +        wait: true
> +      });
> +    });
> +    return (onSuccess || onError)? p.then(onSuccess, onError) : p.then(offer => {
> +      return offer;

You don't need the .then on the second thing:

p.then(o => { return o; })
==
p.then(o => o)
== p

@@ +1020,5 @@
>    },
>  
> +  get haveLocalOffer()  { return this._haveLocalOffer; },
> +  get haveRemoteOffer() { return this._haveRemoteOffer; },
> +  get stable()          { return this._stable; },

See comments on webidl.

::: dom/webidl/RTCPeerConnection.webidl
@@ +97,5 @@
> +  // http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0063.html
> +
> +  readonly attribute Promise<mozRTCSessionDescription> haveLocalOffer;
> +  readonly attribute Promise<mozRTCSessionDescription> haveRemoteOffer;
> +  readonly attribute Promise<mozRTCSessionDescription> stable;

I don't think that we want to have these exposed until we have consensus in the W3C to add them.

@@ +137,5 @@
>    attribute EventHandler onaddtrack;  // replaces onaddstream; see AddTrackEvent
>    attribute EventHandler onremovestream;
>    attribute EventHandler oniceconnectionstatechange;
>  
> +  Promise<RTCStatsReport> getStats (optional MediaStreamTrack? selector);

optional and nullable?  That's a change.

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8522553 [details] [diff] [review]
Part 3: make Idp code in setRemoteDescription work with promises (2)

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

Maybe I needed to include more test cases around this, but it's not quite right.

I have a patch that changes other parts of the IdP stuff over to promises, which might conflict with this.

::: dom/media/PeerConnection.js
@@ +781,5 @@
> +      try {
> +        this._remoteIdp.verifyIdentityFromSDP(sdp, resolve);
> +      } catch (e) {
> +        // processing the SDP for identity didn't work
> +        reject(e);

You don't need to catch. Let the exception turn into a rejection automatically.

@@ +793,5 @@
> +      }
> +    });
> +
> +    let setRemoteDone = new Promise((resolve, reject) => {
> +      this._onSetRemoteDescriptionSuccess = resolve;

do we really want to maintain this mess?

@@ +801,5 @@
>  
>      // we can run the IdP validation in parallel with setRemoteDescription this
> +    // ensures that it doesn't hold things up when it's not on the critical path
> +
> +    Promise.all([setRemoteDone, idpDone])

This isn't a wholly faithful translation.  The only time this needs a call to Promise.all() is when we have a known dependency on identity.  Otherwise, the identity operations are allowed to run to completion without blocking the setRemoteDescription() call.

@@ -804,5 @@
> -    // to complete asynchronously.
> -    let idpDone;
> -    if (!this._impl.peerIdentity) {
> -      idpDone = this._processIdpResult.bind(this);
> -      idpComplete = true; // lie about this for allDone()

See this here?  I don't see this in the fix.  The _impl.peerIdentity attribute needs to be checked before you choose to wrap the two promises with Promise.all().

Comment 13

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

(In reply to Boris Zbarsky [:bz] from comment #10)
> Did you try breakpointing in getWebIDLCallerPrincipal in this test and
> seeing what the JS stack looks like?
> 
> I'm _guessing_ the issue is that you're calling it async somehow at this
> point and then callerPrin of course ends up false.  So breakpoint on that
> error return and see what the callstack is.

I've now diffed the callstacks before/after and they look mostly the same except for the presence of promise-constructors:

>     frame #20: 0x0000000107968581 XUL`js::Invoke(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbe3510, construct=NO_CONSTRUCT) + 1585 at Interpreter.cpp:501
> +   frame #21: 0x0000000107954ae4 XUL`js::Invoke(cx=0x000000010039e360, thisv=0x0000000108076eb8, fval=0x00007fff5fbe3828, argc=2, argv=0x00007fff5fbe3908, rval=JS::MutableHandleValue at 0x00007fff5fbe3610) + 900 at Interpreter.cpp:538
> +   frame #22: 0x0000000107722694 XUL`JS::Call(cx=0x000000010039e360, thisv=JS::HandleValue at 0x00007fff5fbe3768, fval=JS::HandleValue at 0x00007fff5fbe3760, args=0x00007fff5fbe37e8, rval=JS::MutableHandleValue at 0x00007fff5fbe3758) + 228 at jsapi.cpp:5029
> +   frame #23: 0x000000010409fbd1 XUL`mozilla::dom::PromiseInit::Call(this=0x0000000118f734c0, cx=0x000000010039e360, aThisVal=Handle<JS::Value> at 0x00007fff5fbe38c0, resolve=Handle<JSObject *> at 0x00007fff5fbe38b8, reject=Handle<JSObject *> at 0x00007fff5fbe38b0, aRv=0x00007fff5fbe3d78) + 993 at PromiseBinding.cpp:46
> +   frame #24: 0x0000000104fc5f00 XUL`mozilla::dom::PromiseInit::Call(this=0x0000000118f734c0, resolve=Handle<JSObject *> at 0x00007fff5fbe39c0, reject=Handle<JSObject *> at 0x00007fff5fbe39b8, aRv=0x00007fff5fbe3d78, aExceptionHandling=eRethrowExceptions) + 304 at PromiseBinding.h:72
> +   frame #25: 0x0000000104fbb8fe XUL`mozilla::dom::Promise::CallInitFunction(this=0x000000011ba30350, aGlobal=0x00007fff5fbe3df0, aInit=0x0000000118f734c0, aRv=0x00007fff5fbe3d78) + 462 at Promise.cpp:556
> +   frame #26: 0x0000000104fbd806 XUL`mozilla::dom::Promise::Constructor(aGlobal=0x00007fff5fbe3df0, aInit=0x0000000118f734c0, aRv=0x00007fff5fbe3d78) + 310 at Promise.cpp:526
> +   frame #27: 0x0000000104106d50 XUL`mozilla::dom::PromiseBinding::_constructor(cx=0x000000010039e360, argc=1, vp=0x00000001169e1588) + 1024 at PromiseBinding.cpp:574
> +   frame #28: 0x00000001079e62b5 XUL`js::CallJSNative(cx=0x000000010039e360, native=0x0000000104106950, args=0x00007fff5fbe4110)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:231
> +   frame #29: 0x00000001079e6701 XUL`js::CallJSNativeConstructor(cx=0x000000010039e360, native=0x0000000104106950, args=0x00007fff5fbe4110)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 209 at jscntxtinlines.h:264
> +   frame #30: 0x0000000107994bbf XUL`js::InvokeConstructor(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbe4110) + 1135 at Interpreter.cpp:579
> +   frame #31: 0x000000010798940c XUL`Interpret(cx=0x000000010039e360, state=0x00007fff5fbe6d40) + 51372 at Interpreter.cpp:2519
> +   frame #32: 0x000000010797cafb XUL`js::RunScript(cx=0x000000010039e360, state=0x00007fff5fbe6d40) + 667 at Interpreter.cpp:432
> +   frame #33: 0x0000000107968581 XUL`js::Invoke(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbe7230, construct=NO_CONSTRUCT) + 1585 at Interpreter.cpp:501
>     frame #34: 0x00000001077b5fd3 XUL`js_fun_apply(cx=0x000000010039e360, argc=2, vp=0x00000001169e14d0) + 1747 at jsfun.cpp:1314
>     frame #35: 0x00000001079e62b5 XUL`js::CallJSNative(cx=0x000000010039e360, native=0x00000001077b5900, args=0x00007fff5fbe7e80)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:231
>     frame #36: 0x000000010796843d XUL`js::Invoke(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbe7e80, construct=NO_CONSTRUCT) + 1261 at Interpreter.cpp:482
>     frame #37: 0x0000000107989476 XUL`Interpret(cx=0x000000010039e360, state=0x00007fff5fbeaab0) + 51478 at Interpreter.cpp:2522
>     frame #38: 0x000000010797cafb XUL`js::RunScript(cx=0x000000010039e360, state=0x00007fff5fbeaab0) + 667 at Interpreter.cpp:432
>     frame #39: 0x0000000107968581 XUL`js::Invoke(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbeafa0, construct=NO_CONSTRUCT) + 1585 at Interpreter.cpp:501
>     frame #40: 0x0000000107954ae4 XUL`js::Invoke(cx=0x000000010039e360, thisv=0x0000000108076eb8, fval=0x00007fff5fbeb2b8, argc=2, argv=0x00007fff5fbeb398, rval=JS::MutableHandleValue at 0x00007fff5fbeb0a0) + 900 at Interpreter.cpp:538
>     frame #41: 0x0000000107722694 XUL`JS::Call(cx=0x000000010039e360, thisv=JS::HandleValue at 0x00007fff5fbeb1f8, fval=JS::HandleValue at 0x00007fff5fbeb1f0, args=0x00007fff5fbeb278, rval=JS::MutableHandleValue at 0x00007fff5fbeb1e8) + 228 at jsapi.cpp:5029
> +   frame #42: 0x000000010409fbd1 XUL`mozilla::dom::PromiseInit::Call(this=0x0000000118f73b50, cx=0x000000010039e360, aThisVal=Handle<JS::Value> at 0x00007fff5fbeb350, resolve=Handle<JSObject *> at 0x00007fff5fbeb348, reject=Handle<JSObject *> at 0x00007fff5fbeb340, aRv=0x00007fff5fbeb808) + 993 at PromiseBinding.cpp:46
> +   frame #43: 0x0000000104fc5f00 XUL`mozilla::dom::PromiseInit::Call(this=0x0000000118f73b50, resolve=Handle<JSObject *> at 0x00007fff5fbeb450, reject=Handle<JSObject *> at 0x00007fff5fbeb448, aRv=0x00007fff5fbeb808, aExceptionHandling=eRethrowExceptions) + 304 at PromiseBinding.h:72
> +   frame #44: 0x0000000104fbb8fe XUL`mozilla::dom::Promise::CallInitFunction(this=0x0000000119e846b0, aGlobal=0x00007fff5fbeb880, aInit=0x0000000118f73b50, aRv=0x00007fff5fbeb808) + 462 at Promise.cpp:556
> +   frame #45: 0x0000000104fbd806 XUL`mozilla::dom::Promise::Constructor(aGlobal=0x00007fff5fbeb880, aInit=0x0000000118f73b50, aRv=0x00007fff5fbeb808) + 310 at Promise.cpp:526
> +   frame #46: 0x0000000104106d50 XUL`mozilla::dom::PromiseBinding::_constructor(cx=0x000000010039e360, argc=1, vp=0x00000001169e1370) + 1024 at PromiseBinding.cpp:574
> +   frame #47: 0x0000000102ba11a9 XUL`xpc::DOMXrayTraits::construct(cx=0x000000010039e360, wrapper=JS::HandleObject at 0x00007fff5fbeba50, args=0x00007fff5fbebc10, baseInstance=0x000000010a794eb0) + 521 at XrayWrapper.cpp:1632
> +   frame #48: 0x0000000102bafe7f XUL`xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::construct(this=0x000000010a400e90, cx=0x000000010039e360, wrapper=Handle<JSObject *> at 0x00007fff5fbebaa8, args=0x00007fff5fbebc10) const + 111 at XrayWrapper.cpp:2174
> +   frame #49: 0x0000000107922301 XUL`js::Proxy::construct(cx=0x000000010039e360, proxy=JS::HandleObject at 0x00007fff5fbebb90, args=0x00007fff5fbebc10) + 385 at Proxy.cpp:450
> +   frame #50: 0x0000000107924705 XUL`js::proxy_Construct(cx=0x000000010039e360, argc=1, vp=0x00000001169e1370) + 245 at Proxy.cpp:822
> +   frame #51: 0x00000001079e62b5 XUL`js::CallJSNative(cx=0x000000010039e360, native=0x0000000107924610, args=0x00007fff5fbebea0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 165 at jscntxtinlines.h:231
> +   frame #52: 0x00000001079e6701 XUL`js::CallJSNativeConstructor(cx=0x000000010039e360, native=0x0000000107924610, args=0x00007fff5fbebea0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 209 at jscntxtinlines.h:264
> +   frame #53: 0x0000000107994bbf XUL`js::InvokeConstructor(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbebea0) + 1135 at Interpreter.cpp:579
> +   frame #54: 0x000000010798940c XUL`Interpret(cx=0x000000010039e360, state=0x00007fff5fbeead0) + 51372 at Interpreter.cpp:2519
> +   frame #55: 0x000000010797cafb XUL`js::RunScript(cx=0x000000010039e360, state=0x00007fff5fbeead0) + 667 at Interpreter.cpp:432
> +   frame #56: 0x0000000107968581 XUL`js::Invoke(cx=0x000000010039e360, args=CallArgs at 0x00007fff5fbeefc0, construct=NO_CONSTRUCT) + 1585 at Interpreter.cpp:501
> +   frame #57: 0x0000000107954ae4 XUL`js::Invoke(cx=0x000000010039e360, thisv=0x00007fff5fbef358, fval=0x00007fff5fbef3a8, argc=3, argv=0x00007fff5fbef4f8, rval=JS::MutableHandleValue at 0x00007fff5fbef0c0) + 900 at Interpreter.cpp:538
> +   frame #58: 0x0000000107722694 XUL`JS::Call(cx=0x000000010039e360, thisv=JS::HandleValue at 0x00007fff5fbef218, fval=JS::HandleValue at 0x00007fff5fbef210, args=0x00007fff5fbef318, rval=JS::MutableHandleValue at 0x00007fff5fbef208) + 228 at jsapi.cpp:5029
>     frame #59: 0x00000001040c17a7 XUL`mozilla::dom::mozRTCPeerConnectionJSImpl::SetRemoteDescription(this=0x000000011eb8e340, description=0x00000001190d2240, successCallback=0x0000000118f737f0, failureCallback=0x0000000118f73820, aRv=0x00007fff5fbef808, aCompartment=0x0000000121385800) + 1863 at RTCPeerConnectionBinding.cpp:5600

The innermost promise (top) is added by patch 3, the outermost (bottom) is the webidl one in patch 2.

I've tried it a number of ways and confirmed that:
- if either one or both are present then mWebIDLCallerPrincipal = 0x0000000000000000. Fails.
- Remove both and mWebIDLCallerPrincipal = 0x000000011e0adb50. Works.

Looks like WebIDLCallerPrincipals don't play nice with Promises. I can work around one but not the other. Bug?

Comment 14

[Boris Zbarsky [:bzbarsky]]

getWebIDLCallerPrincipal is clearly documented as:

                                                    If the current Entry Script on
648      * the Script Settings Stack represents the invocation of JS-implemented
649      * WebIDL, this API returns the principal of the caller at the time
650      * of invocation.

What's not as clear is that if you're inside a C++-to-JS callback then that callback is your entry script.

So once you're inside the callback function the Promise constructor calls you're no longer in a call to JS-implemented Web IDL as far as XPConnect is concerned.  Just like you wouldn't be if you called some random other code in C++ that then called back into chrome JS.

Throwing here is a feature, since if it were not throwing it would be giving you the wrong answer.

The right solution is to grab up front whatever state depends on the caller principal before you start calling random other system-principal code and to have the callback functions you pass to your Promise constructor close over that state instead of computing it inside the function.

Comment 15

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

Attachment: Part 4: add RTCPeerConnection hybrid with promises (4)

Incorporated feedback. Thanks!

Comment 16

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

Just Martin's feedback so far as I just saw comment 14...

Comment 17

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

(In reply to Boris Zbarsky [:bz] from comment #14)
> The right solution is to grab up front whatever state depends on the caller
> principal before you start calling random other system-principal code and to
> have the callback functions you pass to your Promise constructor close over
> that state instead of computing it inside the function.

That takes care of the promise under my control but what about the webidl-generated promise? How can the promise-returning setRemoteDescription (a webidl method) ever get the caller principle?

Comment 18

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

Comment on attachment 8522553 [details] [diff] [review]
Part 3: make Idp code in setRemoteDescription work with promises (2)

(In reply to Martin Thomson [:mt] from comment #12)
> I have a patch that changes other parts of the IdP stuff over to promises,
> which might conflict with this.

I'm happy to yield to that one since this patch turned out not to be needed.

> > +    Promise.all([setRemoteDone, idpDone])
> 
> This isn't a wholly faithful translation.  The only time this needs a call
> to Promise.all() is when we have a known dependency on identity.  Otherwise,
> the identity operations are allowed to run to completion without blocking
> the setRemoteDescription() call.

Ah, I missed that!

> The _impl.peerIdentity attribute needs to be checked before you choose to
> wrap the two promises with Promise.all().

Just curious, what good does the Idp validation do that nobody waits for?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> but what about the webidl-generated promise? 

I don't follow.  What webidl-generated promise?  This is where the JS stack you're seeing would help... because inside setRemoteDescription, I would expect to be able to call getWebIDLCallerPrincipal.  But what's the actual callstack to that call?

In gdb, "call DumpJSStack()" should do the trick.

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8522691 [details] [diff] [review]
Part 4: add RTCPeerConnection hybrid with promises (4)

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

A couple more suggestions.  Sorry for not catching them.  Conditional r+

::: dom/media/PeerConnection.js
@@ +570,5 @@
>      // Note that webidl bindings make o.mandatory implicit but not o.optional.
>      function convertLegacyOptions(o) {
> +      // Detect (mandatory OR optional) AND no other top-level members.
> +      let lcy = ((o.mandatory && Object.keys(o.mandatory).length) || o.optional) &&
> +                Object.keys(o).length == (o.mandatory && 1) + (o.optional && 1);

I really don't like this reliance on the falsy conversion to 0 here.

@@ +615,5 @@
>            JSON.stringify(options) + " instead (note the case difference)!",
>            null, 0);
>      }
> +    var p = new this._win.Promise((resolve, reject) => {
> +      this._queueOrRun({

it looks like _queueOrRun could return a promise here.  That would make this a lot easier.  Particularly if you fed it onSuccess/onError.

@@ +624,2 @@
>      });
> +    return (onSuccess || onError)? p.then(onSuccess, onError) : p;

I didn't notice this before, but the return on the callback-based variant won't be null if onSuccess returns a value.  That will probably cause problems.  I think that you need to do this:

p.then(x => { onSuccess(x); }, onError)

to ensure that the return code is swallowed properly.

I don't know if our WebIDL enforces void-ness, but if it does, we'd be a little exposed.

Comment 21

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

(In reply to Boris Zbarsky [:bz] from comment #19)
> I don't follow.  What webidl-generated promise?

Nevermind, I was confusing myself. Patch coming up. Thanks!

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8522546 [details] [diff] [review]
Part 1: return DOMError rather than string in peerConnection callbacks

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8522691 [details] [diff] [review]
Part 4: add RTCPeerConnection hybrid with promises (4)

Can you point me to the spec draft we're implementing here, please?

Comment 24

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

Attachment: WebRTC draft with PR

It's in the URL: https://github.com/w3c/webrtc-pc/pull/13 - Not merged yet but has tentative OK from chair in comments. Here's a preview.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8522691 [details] [diff] [review]
Part 4: add RTCPeerConnection hybrid with promises (4)

Oh, I'm blind.  Thanks!

>+++ b/dom/media/PeerConnection.js

>+        args: [offer => resolve(offer), reason => reject(reason), options],

Why not just:

  args: [ resolve, reject, options ],

and similar elsewhere?

>+    return (onSuccess || onError)? p.then(onSuccess, onError) : p;

Does the spec define that ordering for the passed-in callbacks relative to other things added on the returned promise via then() from the page?  It doesn't seem to.  It probably should.

r=me

Comment 26

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

(In reply to Please do not ask for reviews for a bit [:bz] from comment #25)
> >+    return (onSuccess || onError)? p.then(onSuccess, onError) : p;
> 
> Does the spec define that ordering for the passed-in callbacks relative to
> other things added on the returned promise via then() from the page?  It
> doesn't seem to.  It probably should.

Are the steps in https://bug1091898.bugzilla.mozilla.org/attachment.cgi?id=8524272#methods-1 not sufficient?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Maybe I'm missing something, but how do those steps determine the ordering of the things I mentioned?

In particular, "Upon fulfillment of p" doesn't have a preexisting definition, right?

Comment 28

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

It does actually: http://www.w3.org/2001/tag/doc/promises-guide#shorthand-reacting

Comment 29

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

Domenic, I still don't see what ordering guarantees this provides wrt other then() callers.  Could we adjust the phrasing here so that it's clear _when_ the then() call is supposed to happen?

Comment 30

[Domenic Denicola]

Happy to adjust the phrasing. The intent is "Upon fulfillment of p, do X" is equivalent to `p.then(doX)`. I can see how "the successive nested steps should be executed inside a function..." is not exactly clear on that.

First attempt: "Upon fulfillment of p with value v is shorthand for calling p.then(onFulfilled), with the successive nested steps comprising the onFulfilled function, using the initial value of Promise.prototype.then. The steps then have access to onFulfilled’s argument as v."

Comment 31

[Boris Zbarsky [:bzbarsky]]

That sounds good to me, I think.

Comment 32

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

Attachment: Part 1: return DOMError rather than string in peerConnection callbacks (2) r=bz, jesup

Updated commit msg.

Comment 33

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

Attachment: Part 2: remove webrtc-specific testing of webidl exceptions.

Remove invalid-arg tests. WebIDL behavior is well-tested elsewhere.

Comment 34

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

Attachment: Part 2: pass in getWebIDLCallerPrincipal().origin to IdP to allow use of Promises

Comment 35

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

Attachment: Part 3: add RTCPeerConnection hybrid with promises (5) r=bz, mt

Incorporated feedback from mt and bz. Carrying forward r=mt, bz.

- Fixed try fail in test_peerConnection_replaceTrack.html.

(In reply to Martin Thomson [:mt] from comment #20)
> A couple more suggestions.  Sorry for not catching them.  Conditional r+

> > +    var p = new this._win.Promise((resolve, reject) => {
> > +      this._queueOrRun({
> 
> it looks like _queueOrRun could return a promise here.  That would make this
> a lot easier.  Particularly if you fed it onSuccess/onError.

_queueOrRun is called by lots of code, some of which does not use promises, so I'd prefer to not specialize it or split it in two.

Don't get me wrong, I think the whole queue can be ripped out and replaced with a single promise, but this bug is involved enough that I'd rather defer that to a separate bug if you don't mind.

Does your conditional r+ require this? I took a chance that it didn't. Please r- me if it does.

Comment 36

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

Attachment: Part 5: remove old non-spec exceptionhandling around callbacks.

This non-spec handling is no longer needed.
Removes callCB since arg is always Promise.resolve.

Comment 37

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

Attachment: Part 5: update test of exceptions from jsimplemented

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

Comment 38

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

Attachment: Simple PC test (2)

Updated test to work without the pc.haveLocalOffer etc. state-transition promises that got nixed for now.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8526547 [details] [diff] [review]
Part 5: update test of exceptions from jsimplemented

(not sure why I should review test changes)

Comment 40

[Nils Ohlmeier [:drno]]

Comment on attachment 8526524 [details] [diff] [review]
Part 2: remove webrtc-specific testing of webidl exceptions.

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

I would like to get some feedback on my concerns/questions before approving.

::: dom/media/tests/mochitest/test_peerConnection_bug835370.html
@@ +10,5 @@
>  <pre id="test">
>  <script type="application/javascript">
>    createHTML({
>      bug: "835370",
> +    title: "PeerConnection readable exceptions"

Bug 835370 is not about readable exceptions, but constraints, which no longer get tested here?!

@@ +19,1 @@
>      var exception = null;

You removed pconnects and it tests, but here you removed one test for pconnect (without the s at end). Was that intentional?

@@ +22,5 @@
>      } catch (e) {
>          ok(e.message.indexOf("updateIce") >= 0, "PeerConnection.js has readable exceptions");
>          exception = e;
>      }
>      ok(exception, "updateIce not yet implemented and throws");

As this now effectively only tests if updateIce throws a readable exception maybe we should merge this with some other exception test case like test_peerConnection_close?

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +47,5 @@
> +      // both Promise and non-Promise return types, legacy APIs with callbacks,
> +      // including setLocalDescription and setRemoteDescription, are unable to
> +      // throw exceptions. Since such exceptions are "programming errors" by the
> +      // spec's own definition, this should not hinder working code from
> +      // working, which is the sole purpose of the legacy API.

I think it would be helpful to list the functions which are affected by this.

@@ +66,5 @@
>  
>        SimpleTest.doesThrow(function() {
>          pc.createDataChannel({})},
>          "createDataChannel() on closed PC raised expected exception");
>  

I don't see getStats() getting changed in your PR for the spec, and I still see that requirement of throwing in the spec http://www.w3.org/TR/webrtc/, so why did you remove this?

Comment 41

[Nils Ohlmeier [:drno]]

Comment on attachment 8526538 [details] [diff] [review]
Part 5: remove old non-spec exceptionhandling around callbacks.

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

LGTM

Comment 42

[Nils Ohlmeier [:drno]]

If I understand this right you are adding promises as an alternative for some of the PC functions here, right?
But I don't see any new tests covering:
 - the dual headed behavior of the functions
 - the new way of using promises instead of callback

Comment 43

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

(In reply to Nils Ohlmeier [:drno] from comment #40)
> >      bug: "835370",
> > +    title: "PeerConnection readable exceptions"
> 
> Bug 835370 is not about readable exceptions, but constraints, which no
> longer get tested here?!

Yeah that bug number is outdated. PeerConnection hasn't had constraints since Bug 1033833. It now uses plain dictionaries whose behaviors are covered by webidl bindings, and whose per-instance characteristics do not require individual testing.

The readable exceptions test was moved here much later.

> You removed pconnects and it tests, but here you removed one test for
> pconnect (without the s at end). Was that intentional?

Yes it was. I think originally pconnects was used for "success paths" and pconnect for failure paths. I forget why. 

> As this now effectively only tests if updateIce throws a readable exception
> maybe we should merge this with some other exception test case like
> test_peerConnection_close?

Good point. I now see it's actually entirely redundant. See test_exceptions_from_jsimplemented.html in Part 6! No wonder I had deja vu. I'll remove it.

> > +      // both Promise and non-Promise return types, legacy APIs with callbacks,
> > +      // including setLocalDescription and setRemoteDescription, are unable to
> > +      // throw exceptions. Since such exceptions are "programming errors" by the
> > +      // spec's own definition, this should not hinder working code from
> > +      // working, which is the sole purpose of the legacy API.
> 
> I think it would be helpful to list the functions which are affected by this.

Will do.

> I don't see getStats() getting changed in your PR for the spec, and I still
> see that requirement of throwing in the spec http://www.w3.org/TR/webrtc/,
> so why did you remove this?

getStats is getting promises like the rest. It was taken out of the PR only because it is moving to another document. And then on top of that there's https://www.w3.org/Bugs/Public/show_bug.cgi?id=26620

Comment 44

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

(In reply to Nils Ohlmeier [:drno] from comment #42)
> If I understand this right you are adding promises as an alternative for
> some of the PC functions here, right?

With this PR, promises is the non-legacy API.

> But I don't see any new tests covering:
>  - the dual headed behavior of the functions

That's a side-effect of providing the legacy API. Any "dual-head" behavior would be undefined and should not be relied on, and hence should not require any testing I don't think.

>  - the new way of using promises instead of callback

I'll add tests for that next. My current plan is to leave the the test.queue harness alone, and instead take all tests that currently *don't* rely on it - there are a handful - and convert those to use promises. I think that will be enlightening.

Comment 45

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

Attachment: Part 1: remove webrtc-specific testing of webidl exceptions (2) r=jesup

Incorporated feedback.

Comment 46

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #35)
> Does your conditional r+ require this? I took a chance that it didn't.
> Please r- me if it does.

Let's go with the single Promise...  Do you have a bug number for that?

Comment 47

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

Attachment: Part 6: promise-chain test + converted non-harness-using tests to promises

Comment 48

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

Attachment: Part 1: return DOMError rather than string in peerConnection callbacks (3) r=bz, jesup

s/InvalidState/InvalidStateError/

Carrying forward r=bz (on irc), r=jesup.

Comment 49

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

Attachment: Part 7: add Error names to PeerConnection errors rather than empty string

Overdue.

Comment 50

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

Attachment: Part 9: fire error-callbacks instead of throwing on legacy programming errors to work around webidl

Comment 51

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

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

Comment 52

[Martin Thomson [:mt:]]

Comment on attachment 8529358 [details] [diff] [review]
Part 9: fire error-callbacks instead of throwing on legacy programming errors to work around webidl

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

I don't quite understand the need for this.  Won't it be odd if I have something like this:

pc.createOffer(o => { throw 'barf'; },
  e => { e === 'barf' && console.log('wtf'); });

p.then(offer => { onSuccess(offer); }, onError)
  .catch(e => this.logWarning('Uncaught exception in callback: ', e))) : p;

Which suggests that you might like to make a helper function for that now.

Comment 53

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

Attachment: Part 9: restore exception-handling around legacy callbacks (2)

You're right. Incorporating feedback. Thanks!

Comment 54

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

Attachment: Part 5: remove old non-spec exceptionhandling around callbacks (2) r=jesup, drno

Revive test_peerConnection_throwInCallbacks.html thanks to Part 9.

Carrying forward r=jesup, drno.

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

Comment 55

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

Attachment: Part 4: remove old non-spec exceptionhandling around callbacks (3) r=jesup, drno

Also revive logErrorAndCallOnError() needed by Part 9.

Carrying forward r=jesup, drno.

Comment 56

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

Attachment: Part 9: restore exception-handling around legacy callbacks (3)

Fix test failures.

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

Comment 57

[Martin Thomson [:mt:]]

Comment on attachment 8529471 [details] [diff] [review]
Part 9: restore exception-handling around legacy callbacks (3)

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

Thanks for fixing that.

::: dom/media/PeerConnection.js
@@ +560,5 @@
> +        errorFunc(e.message, e.fileName, e.lineNumber);
> +      }
> +    }
> +    return onSuccess? p.then(result => callCB(onSuccess, result),
> +                             reason => (onError? callCB(onError, reason) : null)) : p;

I think that this could be a lot simpler.

return onSuccess
         ? p.then(result => { onSuccess(result); }, onError) 
            .catch(e => this.logErrorAndCallOnError(e.message, e.fileName, e.lineNumber)
         : p;

It occurs that logErrorAndCallOnError() could be modified to take a single argument though (which would avoid line length issues here as well as centralizing processing).

Comment 58

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

(In reply to Martin Thomson [:mt] from comment #57)
> > +    return onSuccess? p.then(result => callCB(onSuccess, result),
> > +                             reason => (onError? callCB(onError, reason) : null)) : p;
> 
> I think that this could be a lot simpler.
> 
> return onSuccess
>          ? p.then(result => { onSuccess(result); }, onError) 
>             .catch(e => this.logErrorAndCallOnError(e.message, e.fileName,
> e.lineNumber)
>          : p;

I agree promise-catch would be less verbose here than try/catch, but the callbacks are synchronous so try/catch is actually simpler since it avoids an extra dispatch.

> logErrorAndCallOnError() could be modified to take a single
> argument though (which would avoid line length issues here as well as
> centralizing processing).

It's existing code that's mirrored on logError. Not much gain unless I follow your other advice I think.

Comment 59

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #58)
> I agree promise-catch would be less verbose here than try/catch, but the
> callbacks are synchronous so try/catch is actually simpler since it avoids
> an extra dispatch.

You keep saying that there is one error handling discipline to rule them all...

> It's existing code that's mirrored on logError. Not much gain unless I
> follow your other advice I think.

I see it used exactly once on dxr; I can't imagine an alternative call pattern, so even for one call it's worthwhile doing, I think.

Comment 60

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

(In reply to Martin Thomson [:mt] from comment #59)
> You keep saying that there is one error handling discipline to rule them
> all...

...and in the darkness .bind(this).

> > It's existing code that's mirrored on logError. Not much gain unless I
> > follow your other advice I think.
> 
> I see it used exactly once on dxr; I can't imagine an alternative call
> pattern, so even for one call it's worthwhile doing, I think.

OK.

Comment 61

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

Attachment: Part 8: restore exception-handling around legacy callbacks (4) r=mt

Incorporate feedback. Carrying forward r=mt.

Comment 62

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

(In reply to Martin Thomson [:mt] from comment #46)
> Let's go with the single Promise...  Do you have a bug number for that?

Filed Bug 1106675.

Comment 63

[Nils Ohlmeier [:drno]]

Comment on attachment 8527225 [details] [diff] [review]
Part 1: remove webrtc-specific testing of webidl exceptions (2) r=jesup

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

LGTM

Comment 64

[Nils Ohlmeier [:drno]]

Comment on attachment 8527969 [details] [diff] [review]
Part 6: promise-chain test + converted non-harness-using tests to promises

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

With minor edits LGTM.

::: dom/media/tests/mochitest/test_peerConnection_promise.html
@@ +41,5 @@
> +  runNetworkTest(function() {
> +    is(v2.currentTime, 0, "v2.currentTime is zero at outset");
> +
> +    navigator.mediaDevices.getUserMedia({ fake: true, video: true, audio: true })
> +    .then(stream => pc1.addStream(v1.mozSrcObject = stream))

As you are only adding a stream to pc1, but never to pc2 you will get a recvonly call, or?
Is that on purpose?
If it is on purpose I think it would be good to represent that in the name of the file. From the file name I expected to find a full two way call.

@@ +48,5 @@
> +    .then(() => pc2.setRemoteDescription(pc1.localDescription))
> +    .then(() => pc2.createAnswer())
> +    .then(answer => pc2.setLocalDescription(answer))
> +    .then(() => pc1.setRemoteDescription(pc2.localDescription))
> +    .then(() => ok(true, "Connected."))

You actually don't know if you are connected at this point. You successfully set the descriptions on both ends and ICE is probably under way.
I would at least change the message in the ok() to say that.

Comment 65

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

(In reply to Nils Ohlmeier [:drno] from comment #64)
> As you are only adding a stream to pc1, but never to pc2 you will get a
> recvonly call, or?

Yes.

> Is that on purpose?

Yes it was. I thought we had enough tests of sendrecv. I plan to add a sendrecv test later that uses Promise.all to do setup a little bit more in parallel.

> If it is on purpose I think it would be good to represent that in the name
> of the file. From the file name I expected to find a full two way call.

OK I've renamed it test_peerConnection_promiseSendOnly.html

> > +    .then(() => ok(true, "Connected."))
> 
> You actually don't know if you are connected at this point. You successfully
> set the descriptions on both ends and ICE is probably under way.
> I would at least change the message in the ok() to say that.

Thanks, I've moved it to below the flow test.

Comment 66

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

Comment on attachment 8529356 [details] [diff] [review]
Part 1: return DOMError rather than string in peerConnection callbacks (3) r=bz, jesup

Moved Part 1 patch to Bug 1053407.

Comment 67

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

Attachment: Part 1: add Error names to PeerConnection errors rather than empty string (2) r=jesup

Reorder patches instead. Part 7 is now Part 1 (which I did for other reasons). This should restore the other part numbers.

- Updated commit msg. Carrying forward r=jesup.

Comment 68

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

Attachment: Part 2: remove webrtc-specific testing of webidl exceptions (2) r=jesup, drno

Reordered and updated commit msg. Carrying forward r=jesup, drno.

Comment 69

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

Attachment: Part 3: pass in getWebIDLCallerPrincipal().origin to IdP to allow use of Promises (2) r=mt

Reordered and updated commit msg.

Comment 70

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

Attachment: Part 4: add RTCPeerConnection hybrid with promises (6) r=bz, mt

Reordered. Carrying forward r=bz, jesup.

Comment 71

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

Attachment: Part 5: remove old non-spec exceptionhandling around callbacks (4) r=jesup, drno

Reordered. Carrying forward r=jesup, drno.

Comment 72

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

Attachment: Part 6: update test of exceptions from jsimplemented (2) r=jesup

Reordered and updated commit msg. Carrying forward r=jesup.

Comment 73

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

Attachment: Part 7: promise-chain test + converted non-harness-using tests to promises (2) r=drno

Reordered and updated commit msg. Carrying forward r=drno.

Comment 74

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

Attachment: Part 8: restore exception-handling around legacy callbacks (4) r=mt

No change. Carrying forward r=mt.

Comment 75

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

Attachment: Part 7: promise-chain test + converted non-harness-using tests to promises (3) r=drno

Forgot bug # in commit msg. Carrying forward r=drno.

Comment 76

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

Attachment: Part 8: restore exception-handling around legacy callbacks (5) r=mt

No change. Carrying forward r=mt.

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

Comment 77

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

Attachment: WebRTC draft with PR (2)

Comment 78

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

Attachment: Simple PC test (3)

Comment 79

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

Attachment: WebRTC draft with PR (3)

Correct file.

Comment 80

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

Green try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=10745bfa558d

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/c4f14d82c20b
https://hg.mozilla.org/integration/fx-team/rev/e210b532f9dc
https://hg.mozilla.org/integration/fx-team/rev/fb73f5e56e47
https://hg.mozilla.org/integration/fx-team/rev/e5bf096003d5
https://hg.mozilla.org/integration/fx-team/rev/5bc1b5e79914
https://hg.mozilla.org/integration/fx-team/rev/f9cf5afe311a
https://hg.mozilla.org/integration/fx-team/rev/34fd03c467e8
https://hg.mozilla.org/integration/fx-team/rev/3e280c309d0d

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4f14d82c20b
https://hg.mozilla.org/mozilla-central/rev/e210b532f9dc
https://hg.mozilla.org/mozilla-central/rev/fb73f5e56e47
https://hg.mozilla.org/mozilla-central/rev/e5bf096003d5
https://hg.mozilla.org/mozilla-central/rev/5bc1b5e79914
https://hg.mozilla.org/mozilla-central/rev/f9cf5afe311a
https://hg.mozilla.org/mozilla-central/rev/34fd03c467e8
https://hg.mozilla.org/mozilla-central/rev/3e280c309d0d

Comment 83

[Eric Shepherd [:sheppy]]

All documentation of methods which use callbacks should now be updated to also document the promise versions. Any that haven't been (and I don't think there are any) will be updated as those interfaces are brought up to date over the coming weeks.

Comment 84

[Eric Shepherd [:sheppy]]

Oh yes, this is also now mentioned on Firefox 32 for developers.