Closed Bug 1091898 Opened 5 years ago Closed 5 years ago

Update WebRTC with promises

Categories

(Core :: WebRTC, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jib, Assigned: jib)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 29 obsolete files)

7.88 KB, patch
jib
: review+
Details | Diff | Splinter Review
6.75 KB, patch
jib
: review+
Details | Diff | Splinter Review
9.12 KB, patch
jib
: review+
Details | Diff | Splinter Review
17.01 KB, patch
jib
: review+
Details | Diff | Splinter Review
9.11 KB, patch
jib
: review+
Details | Diff | Splinter Review
2.72 KB, patch
jib
: review+
Details | Diff | Splinter Review
11.48 KB, patch
jib
: review+
Details | Diff | Splinter Review
13.95 KB, patch
jib
: review+
Details | Diff | Splinter Review
1.63 KB, text/html
Details
228.97 KB, text/html
Details
I needed a place to put patches for upcoming w3c work.
Attached file Simple PC test (obsolete) —
Depends on: 1091290
Summary: Updated WebRTC to spec → Update WebRTC to spec
Attachment #8514612 - Attachment is obsolete: true
Attachment #8522347 - Attachment is obsolete: true
Attachment #8522548 - Flags: review?(martin.thomson)
Attachment #8522548 - Flags: review?(bzbarsky)
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.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(bzbarsky)
Attachment #8522553 - Flags: review?(martin.thomson)
Attachment #8522351 - Attachment is obsolete: true
bholley, martin says you know about getWebIDLCallerPrincipal(). See comment 7. Any help appreciated.
Flags: needinfo?(martin.thomson) → needinfo?(bobbyholley)
> 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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8522548 - Flags: review?(martin.thomson) → review-
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().
Attachment #8522553 - Flags: review?(martin.thomson)
(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?
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.
Flags: needinfo?(bobbyholley)
Incorporated feedback. Thanks!
Attachment #8522548 - Attachment is obsolete: true
Attachment #8522548 - Flags: review?(bzbarsky)
Attachment #8522691 - Flags: review?(martin.thomson)
Attachment #8522691 - Flags: review?(bzbarsky)
Just Martin's feedback so far as I just saw comment 14...
(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 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?
Attachment #8522553 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8522691 - Flags: review?(martin.thomson) → review+
(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!
Attachment #8522546 - Flags: review?(rjesup) → review+
Depends on: 1075133
Comment on attachment 8522546 [details] [diff] [review]
Part 1: return DOMError rather than string in peerConnection callbacks

r=me
Attachment #8522546 - Flags: review?(bzbarsky) → review+
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?
Flags: needinfo?(jib)
Attached file WebRTC draft with PR (obsolete) —
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.
Flags: needinfo?(jib)
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
Attachment #8522691 - Flags: review?(bzbarsky) → review+
(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?
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?
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?
Flags: needinfo?(d)
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."
Flags: needinfo?(d)
That sounds good to me, I think.
Updated commit msg.
Attachment #8522546 - Attachment is obsolete: true
Attachment #8526510 - Flags: review+
Attachment #8522691 - Attachment description: Part 2: add RTCPeerConnection hybrid with promises (4) → Part 4: add RTCPeerConnection hybrid with promises (4)
Remove invalid-arg tests. WebIDL behavior is well-tested elsewhere.
Attachment #8526524 - Flags: review?(rjesup)
Attachment #8526524 - Flags: review?(drno)
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.
Attachment #8522691 - Attachment is obsolete: true
Flags: needinfo?(martin.thomson)
Attachment #8526532 - Flags: review+
This non-spec handling is no longer needed.
Removes callCB since arg is always Promise.resolve.
Attachment #8526538 - Flags: review?(rjesup)
Attachment #8526538 - Flags: review?(drno)
Attached file Simple PC test (2) (obsolete) —
Updated test to work without the pc.haveLocalOffer etc. state-transition promises that got nixed for now.
Attachment #8514613 - Attachment is obsolete: true
Attachment #8526524 - Flags: review?(rjesup) → review+
Attachment #8526538 - Flags: review?(rjesup) → review+
Comment on attachment 8526547 [details] [diff] [review]
Part 5: update test of exceptions from jsimplemented

(not sure why I should review test changes)
Attachment #8526547 - Flags: review?(bugs) → review?(rjesup)
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 on attachment 8526538 [details] [diff] [review]
Part 5: remove old non-spec exceptionhandling around callbacks.

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

LGTM
Attachment #8526538 - Flags: review?(drno) → review+
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
Flags: needinfo?(jib)
Attachment #8526547 - Flags: review?(rjesup) → review+
(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
Flags: needinfo?(jib)
(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.
Attachment #8526526 - Flags: review?(martin.thomson) → review+
Incorporated feedback.
Attachment #8526524 - Attachment is obsolete: true
Attachment #8526524 - Flags: review?(drno)
Attachment #8527225 - Flags: review?(drno)
(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?
Flags: needinfo?(martin.thomson)
s/InvalidState/InvalidStateError/

Carrying forward r=bz (on irc), r=jesup.
Attachment #8529356 - Flags: review+
Overdue.
Attachment #8526510 - Attachment is obsolete: true
Attachment #8529357 - Flags: review?(rjesup)
Attachment #8529358 - Flags: review?(martin.thomson)
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.
Attachment #8529358 - Flags: review?(martin.thomson)
You're right. Incorporating feedback. Thanks!
Attachment #8529358 - Attachment is obsolete: true
Attachment #8529456 - Flags: review?(martin.thomson)
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
Attachment #8526538 - Attachment is obsolete: true
Attachment #8529458 - Flags: review+
Also revive logErrorAndCallOnError() needed by Part 9.

Carrying forward r=jesup, drno.
Attachment #8529458 - Attachment is obsolete: true
Attachment #8529469 - Flags: review+
Attachment #8529456 - Attachment description: Part 9: restore exception-handling around legacy callbacks → Part 9: restore exception-handling around legacy callbacks (2)
Fix test failures.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69618d91482a
Attachment #8529456 - Attachment is obsolete: true
Attachment #8529456 - Flags: review?(martin.thomson)
Attachment #8529471 - Flags: review?(martin.thomson)
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).
Attachment #8529471 - Flags: review?(martin.thomson) → review+
(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.
(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.
(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.
Incorporate feedback. Carrying forward r=mt.
Attachment #8530981 - Flags: review+
Attachment #8529471 - Attachment is obsolete: true
(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.
Attachment #8529357 - Flags: review?(rjesup) → review+
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
Attachment #8527225 - Flags: review?(drno) → review+
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.
Attachment #8527969 - Flags: review?(drno) → review+
(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 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.
Attachment #8529356 - Attachment is obsolete: true
Attachment #8527225 - Attachment description: Part 2: remove webrtc-specific testing of webidl exceptions (2) r=jesup → Part 1: remove webrtc-specific testing of webidl exceptions (2) r=jesup
Attachment #8526526 - Attachment description: Part 3: pass in getWebIDLCallerPrincipal().origin to IdP to allow use of Promises → Part 2: pass in getWebIDLCallerPrincipal().origin to IdP to allow use of Promises
Attachment #8526532 - Attachment description: Part 4: add RTCPeerConnection hybrid with promises (5) r=bz, mt → Part 3: add RTCPeerConnection hybrid with promises (5) r=bz, mt
Attachment #8529469 - Attachment description: Part 5: remove old non-spec exceptionhandling around callbacks (3) r=jesup, drno → Part 4: remove old non-spec exceptionhandling around callbacks (3) r=jesup, drno
Attachment #8526547 - Attachment description: Part 6: update test of exceptions from jsimplemented → Part 5: update test of exceptions from jsimplemented
Attachment #8527969 - Attachment description: Part 7: promise-chain test + converted non-harness-using tests to promises → Part 6: promise-chain test + converted non-harness-using tests to promises
Attachment #8529357 - Attachment description: Part 8: add Error names to PeerConnection errors rather than empty string → Part 7: add Error names to PeerConnection errors rather than empty string
Attachment #8530981 - Attachment description: Part 9: restore exception-handling around legacy callbacks (4) r=mt → Part 8: restore exception-handling around legacy callbacks (4) r=mt
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.
Attachment #8533189 - Flags: review+
Reordered and updated commit msg. Carrying forward r=jesup, drno.
Attachment #8526526 - Attachment is obsolete: true
Attachment #8527225 - Attachment is obsolete: true
Attachment #8533190 - Flags: review+
Reordered. Carrying forward r=bz, jesup.
Attachment #8526532 - Attachment is obsolete: true
Attachment #8533192 - Flags: review+
Reordered. Carrying forward r=jesup, drno.
Attachment #8529469 - Attachment is obsolete: true
Attachment #8533193 - Flags: review+
Reordered and updated commit msg. Carrying forward r=jesup.
Attachment #8526547 - Attachment is obsolete: true
Attachment #8533195 - Flags: review+
Reordered and updated commit msg. Carrying forward r=drno.
Attachment #8527969 - Attachment is obsolete: true
Attachment #8533196 - Flags: review+
No change. Carrying forward r=mt.
Attachment #8529357 - Attachment is obsolete: true
Attachment #8530981 - Attachment is obsolete: true
Attachment #8533199 - Flags: review+
Forgot bug # in commit msg. Carrying forward r=drno.
Attachment #8533196 - Attachment is obsolete: true
Attachment #8533203 - Flags: review+
Summary: Update WebRTC to spec → Update WebRTC with promises
Attached file Simple PC test (3)
Attachment #8526780 - Attachment is obsolete: true
Correct file.
Attachment #8533242 - Attachment is obsolete: true
Blocks: 1050930
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.
Oh yes, this is also now mentioned on Firefox 32 for developers.
Depends on: 1227482
You need to log in before you can comment on or make changes to this bug.