975144 - Support electrolysis for WebRTC identity

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Martin Thomson [:mt:]]

The code in Bug 884573 doesn't run under electrolysis.  That's bad for b2g and for new versions of the browser that will be running e10s.

This is only tricky because the sandbox we use needs to be created in the parent process.  Child processes cannot access the hidden DOM window.

Comment 1

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-975144-Adding-e10s-support-for-WebRTC-IdP.patch

This does some fairly convoluted stuff to replicate code like the following:

var x = new Foo();

...where x is in a content process and the instance of Foo is in the parent process.  This needs to be this way since we are using Sandbox.jsm, which can only run in the parent process.

It's not generic code.  I considered taking steps to make it generic, but unless and until someone else needs to do this, this is far easier to reason about.

:billm, can you look at this for any egregious abuses of the message manager code.

:jib knows WebRTC well enough to make sense of the functional aspects of this (which should remain basically identical from before this change).

Comment 2

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

Comment on attachment 8412907 [details]
0001-Bug-975144-Adding-e10s-support-for-WebRTC-IdP.patch

Review of attachment 8412907 [details]:
-----------------------------------------------------------------

lgtm.

::: browser/components/nsBrowserGlue.js
@@ +514,5 @@
>    },
>  
> +  receiveMessage: function(message) {
> +    if (message.name === "WebRTC:IdP") {
> +      Cu.import("resource://gre/modules/media/IdpProxyChrome.jsm");

I usually see Cu.import in global scope. I guess this is a prototype method which makes it ok?

::: dom/media/IdpProxy.jsm
@@ +88,5 @@
> +  receiveMessage: function(message) {
> +    let channel = this._channels[message.data.id];
> +    if (channel && typeof channel[message.data.action] === "function") {
> +      channel[message.data.action](message.data.message);
> +    } else {

I'm pretty sure JS blows up fine without your interference here.

::: dom/media/IdpProxyChrome.jsm
@@ +24,5 @@
> + * There is no visible UX here, as we assume the user has already
> + * logged in elsewhere (on a different screen in the web site hosting
> + * the RTC functions).
> + */
> +function IdpSandbox(uri, messageCallback, targetBrowser) {

targetBrowser appears unused. Remove.

@@ +112,5 @@
> +      this._cleanup(message.target);
> +    } else if (typeof this[message.data.action] === "function") {
> +      this[message.data.action](message.target, message.data.id, message.data.message);
> +    } else {
> +      // blow up

I'm pretty sure JS blows up fine without your interference here.

Comment 3

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> ::: browser/components/nsBrowserGlue.js
> @@ +514,5 @@
> >    },
> >  
> > +  receiveMessage: function(message) {
> > +    if (message.name === "WebRTC:IdP") {
> > +      Cu.import("resource://gre/modules/media/IdpProxyChrome.jsm");
> 
> I usually see Cu.import in global scope. I guess this is a prototype method
> which makes it ok?

Yes, imports are usually out in the open, but that has a consequence of loading an unused module at startup time.  That would be bad for something like this that isn't used that often.  This is a pattern that you see a bit in various places; remote-browser.xml does this, for example.

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8412907 [details]
0001-Bug-975144-Adding-e10s-support-for-WebRTC-IdP.patch

Looks like I'm going to have to do some homework on how to open hidden frames in the child process.  That's frightening, so I've been putting this off.

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8412907 [details]
0001-Bug-975144-Adding-e10s-support-for-WebRTC-IdP.patch

We have a new approach.  This one works, but opens an expressway into the parent process.

Comment 6

[Martin Thomson [:mt:]]

OK, after a long hiatus, I have a new approach.  This one is not 100% solid because I'm waiting on review from the working group, and there seem to be a few xpconnect oddities (probably all my fault).  Either way, I'm going to put it all up on reviewboard so that it's on the record somewhere.

Comment 7

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e352db4cae2

Comment 8

[Martin Thomson [:mt:]]

After a few failures, try again: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e05e8c043ef9 - I'm proud of that try line.

Comment 9

[Martin Thomson [:mt:]]

Reviewer cheat sheet (since RB doesn't make this very clear).  Feel free to pass on your r? if you are busy.

:sicking

https://reviewboard.mozilla.org/r/2325/diff/
 - can you look at the webidl (this isn't official yet, but I'm confident that we'll do this based on feedback so far)
https://reviewboard.mozilla.org/r/2333/diff/
 - can you just look at IdpSandbox.jsm (line 44, sorry I can't give you a direct link) to see that my channel use is OK, following that through to where I set the principal on the sandbox on line 199 would be good.

:gabor

https://reviewboard.mozilla.org/r/2329/diff/
https://reviewboard.mozilla.org/r/2331/diff/
 - This is all very much like bug 1116269

:bholley

https://reviewboard.mozilla.org/r/2333/diff/
 - I just want to ensure that I'm not being stupid.  IdpSandbox.jsm lines 195-219 - can you quickly confirm that I'm OK to use registrar from here on as long as I don't waive (it's a C++-implemented WebIDL).

:jib

Congratulations you get the bulk of the work.  If you think this is a bit much, I'll see if I can offload some of this on :drno or :bwc, but you are most familiar with this stuff.

Comment 10

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://975144/mt

Comment 11

[Martin Thomson [:mt:]]

/r/2325 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/2327 - Bug 975144 - Implementation of RTC identity DOM component
/r/2329 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope
/r/2331 - Bug 975144 - Tests for rtcIdentityProvider property
/r/2333 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/2335 - Bug 975144 - Updating test IdP for new API
/r/2337 - Bug 975144 - Updating RTC identity tests
/r/2339 - Bug 975144 - Adding IdP loading tests
/r/2341 - Bug 975144 - Enabling tests on e10s

Pull down these commits:

hg pull review -r 452abd4192ab421d9eef9b29c2aeb532dbe7c46b

Comment 12

[Bobby Holley (:bholley)]

https://reviewboard.mozilla.org/r/2323/#review1527

::: dom/media/IdpSandbox.jsm
(Diff revision 1)
> +    let registrar = this.sandbox.rtcIdentityProvider;

Yes, this should be good as long as no code runs beforehand. Confirm Cu.isXrayWrapper(registrar).

Comment 13

[Bobby Holley (:bholley)]

https://reviewboard.mozilla.org/r/2323/#review1529

Ship It!

Comment 14

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/2329/#review1539

Ship It!

Comment 15

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/2331/#review1541

Ship It!

Comment 16

[Gabor Krizsanits (INACTIVE)]

I don't know what else needs documentation here but for sure we added a new Sandbox global property: rtcIdentityProvider

Comment 17

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

https://reviewboard.mozilla.org/r/2327/#review1557

::: dom/media/RTCIdentityProviderRegistrar.cpp
(Diff revision 1)
> +    , mIdp(nullptr)

Technically redundant

Comment 18

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

https://reviewboard.mozilla.org/r/2333/#review1671

I'm unhappy about the overall error strategy.

::: dom/media/PeerConnection.js
(Diff revision 1)
> -    }.bind(this));
> +        }, e => {});

Same on createAnswer

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> +function delay(t) {
> +  return new Promise(resolve => {
> +    let timer = Cc['@mozilla.org/timer;1'].getService(Ci.nsITimer);
> +    timer.initWithCallback(new TimerResolver(resolve), t, 0); // One shot
> +  });
> +}

Maybe a comment about why setTimeout is unsuitable?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> +   * A little note on error handling in this class: this class reports errors
> +   * exclusively through the event handlers that are passed to it
> +   * (this._dispatchError, specifically).  That means that all the functions
> +   * return resolved promises; promises are never rejected.  This probably isn't
> +   * the best design, but the refactor can wait.

I'd love to know more about what the pain-points are about getting errors done right.

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> -      } catch (e) {
> +    } catch (e) {
> -        this.reportError("validation",
> -                         "invalid identity assertion: " + e);
> +      this.reportError('validation',
> +                       'invalid identity assertion: ' + e);
> -      } // for JSON.parse

Yeah, real unhappy about this now that I see it clearly. What's the obstable to throwing and rejecting things up?

Also, shouldn't it return undefined here?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> -
> -      this.reportError("validation", "assertion missing" +
> -                       " idp/idp.domain/assertion");
> +    if (!this._isValidAssertion(assertion)) {
> +      this.reportError('validation', 'assertion missing' +
> +                       ' idp/idp.domain/assertion');
>      }
> -    // undefined!
> +    return assertion;

This will return an invalid assertion, whereas before it returned undefined. That seems bad.

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> -    let onAssertion = function() {
> +    let validationPromise = this._idp.start()

Maybe s/validationPromise/validated/ or inline it?

ditto s/assertionPromise/asserted/

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> +        .catch(e => this.reportError(type, 'error reported by IdP: ' + e.message))
> +        .then(result => {
> +          done = true;
> +          return result;
> +        });

Why not fail instead of overloading successValue == undefined?

::: dom/media/PeerConnection.js
(Diff revision 1)
> -      if (assertion) {
> +      .then(assertion => this._gotIdentityAssertion(assertion));

this would also work and reduce some lines above:

.then(assertion => assertion && this._gotIdentityAssertion(assertion)); ?

::: dom/media/PeerConnection.js
(Diff revision 1)
> -      if (assertion) {
> +      idp.getIdentityAssertion(pc._impl.fingerprint)
> +        .then(assertion => {
> -        pc._gotIdentityAssertion(assertion);
> +          pc._gotIdentityAssertion(assertion);
> -      }
> +          sdp = idp.addIdentityAttribute(sdp);
> -      pc._onCreateOfferSuccess(new pc._win.mozRTCSessionDescription({ type: "offer",
> +          pc._onCreateOfferSuccess(new pc._win.mozRTCSessionDescription({ type: "offer",
> -                                                                      sdp: sdp }));
> +                                                                          sdp: sdp }));
> -    }.bind(this));
> +        }, e => {}); // errors are handled in the IdP
> +    } else {

With promises, it's easier for me to reason about the flow here, so in spite of having r+'ed much of this in the past, I now find the error strategy unsound.

Dispatching events with info on "errors" sounds useful - especially lacking any other outlet - but is no excuse for hanging createOffer (calling neither success nor failure callback) IMHO. I guess we could define it that way, but I sure hope we haven't, because it seems easy enough to reject with an error here.

Also, this chain needs terminating with a catch, or any trouble in _gotIdentityAssertion or addIdentityAttribute - even a coding error - is never heard from.

I suggest removing the bitbucket and have a final catch call onCreateOfferError with a reasonable error name and an informative message.

We can keep dispatching events as well if desirable. When I first looked at this, I assumed the events were for things that didn't have a natural outlet, but IMHO anything that blocks call-setup should cause call-setup calls to fail.

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> +  _isValidAssertion: function(assertion) {
> +    return assertion && assertion.idp &&
> +      typeof assertion.idp.domain === 'string' &&
> +      (!assertion.idp.protocol ||
> +       typeof assertion.idp.protocol === 'string') &&
> +      typeof assertion.assertion === 'string';
> +  },

This looks like just a structure- and type-validator, right? I got confused, thinking a "valid assertion" was a kind of state for an "assertion", like "verified identity".

Maybe _isValidAssertionType? I know we have names like ValidateConfiguration already, there's just something about an assertion. Or maybe it's just me.

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 1)
> -    let request = {
> -      type: "VERIFY",
> -      message: assertion,
> -      origin: origin
> +    let isSubsetOf = (outerSet, innerSet, comparator) => {
> +      return innerSet.every(i => {
> +        return outerSet.some(o => comparator(i, o));
> +      });

In other words: every inner item must match some outer item. Yeah that sounds right.

PS: Would be easier to read without returns and brackets I think.

Comment 19

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

https://reviewboard.mozilla.org/r/2337/#review1709

The "catch-and-continue" cases are not right.

::: dom/media/tests/mochitest/identity/test_idpproxy.html
(Diff revision 1)
> -    done();
> +    .then(assertion => {
> +      var wrapped = SpecialPowers.wrap(assertion);
> +      return makeSandbox()
> +        .then(idp => idp.validateAssertion(wrapped.assertion,
> +                                           'https://example.net'));
> +    }).then(response => {

This can be flattened.

::: dom/media/tests/mochitest/identity/test_setIdentityProviderWithErrors.html
(Diff revision 1)
> +            e => window.setTimeout(checkEvents, 0);

race prone?

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
> -          finished(true, "session description was OK");
> -        }, function(err) {
> -          finished(false, "error setting the session description: " + err);
> +    }, fail('getIdentityAssertion'))
> +    .then(() => {
> +      ok(true, 'session description was OK');

Catch-and-continue causes ok(true,"session description was OK") on getIdentityAssertion failure.

That is, if getIdentityAssertion could fail. Two wrongs make a right! ;-)

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
> -    callback(assertion);
> -  }
> -
> -  idp = new IdpProxy("example.com", "idp.html");
> -  idp.start(handleFailure);
> +    }, error => {
> +      var e = SpecialPowers.wrap(error);
> +      ok(false, 'error in sandbox: ' +
> +         (e.message ? (e.message + '\n' + e.stack) : e));
> +    });

I'm wary of the .then(x,y) pattern, because it's a catch-and-continue. In other words - since we're at the end of the getIdentityAssertion function here - it means:

getIdentityAssertion(myarray)
.then(assertionString => rockNroll(assertionString))

will call rockNroll(undefined) when there's a sandbox error, which seems wrong (it's undefined because your rejection handler doesn't return anything).

Options: You can't really re-throw your way out, because if you repeat this pattern then it hits up all of them. Best to refactor to catch errors only at the very end of the chain.

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
>  // that doesn't mean we can't have SDP that describes two
> +// this function synthesizes that SDP and tries to set it

period?

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
> +    }, fail('gUM'))
> +    .then(o => {
> +      offer = o;
> +      ok(offer, 'Got offer');
> +
> +      match = offer.sdp.match(fingerprintRegex);

Catch-and-continue causes offer = undefined on gUM failure.

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
> +    }, fail('createOffer'))
> +    .then(assertion => {
> +      ok(assertion, 'Should have assertion');

Catch-and-continue causes assertion = undefined on createOffer failure.

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revision 1)
> -        });
> -      });
> -    }, function(err) {
> +    }, fail('setRemoteDescription'))
> +    .then(() => {
> +      pcStrict.close();

This one's actually right, but I would make this the central .catch() for common errors, then() close everything like you have, and a final .catch() for when things go really wrong.

::: dom/media/tests/mochitest/identity/test_idpproxy.html
(Diff revision 1)
> -      ok(false, "IdpProxy didn't catch bad domain: " + domain);
> +      ok(false, 'IdpSandbox didn\'t catch bad domain: ' + domain);

I actually prefer the former here. Isn't that what the two kinds are for?

::: dom/media/tests/mochitest/identity/test_idpproxy.html
(Diff revision 1)
> -  idp.send(request, handleResponse);
> +function fail(reason) {

Maybe s/fail/test_fail/ ?

Comment 20

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

https://reviewboard.mozilla.org/r/2335/#review1707

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 1)
> +    var p = global.location.pathname;

nit: s/p /s / ? I usually think of p as a promise.

Comment 21

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

https://reviewboard.mozilla.org/r/2339/#review1727

::: dom/media/tests/mochitest/identity/idp-min.js
(Diff revision 1)
> +  // from two different locations in the tree.  

whitespace

Comment 22

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

https://reviewboard.mozilla.org/r/2341/#review1729

Ship It!

Comment 23

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2337/#review1749

> race prone?

Not needed actually.

> This can be flattened.

I don't see how.  The inner .then() needs access to something in the enclosing scope.

Comment 24

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

https://reviewboard.mozilla.org/r/2337/#review1755

> I don't see how.  The inner .then() needs access to something in the enclosing scope.

I was thinking:

.then(assertion => makeSandbox())
.then(idp => idp.validateAssertion(SpecialPowers.wrap(assertion).assertion,
                                   'https://example.net'))
.then(response => {

Comment 25

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #24)
> .then(assertion => makeSandbox())
> .then(idp => idp.validateAssertion(SpecialPowers.wrap(assertion).assertion,
>                                    'https://example.net'))
> .then(response => {

On the second line, `assertion` is undefined.  It's only defined within the closure on the first line.

Comment 26

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

(In reply to Martin Thomson [:mt] from comment #25)
> On the second line, `assertion` is undefined.

Doh!

Comment 27

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

You could lift it out, but not worth it.

Comment 28

[Martin Thomson [:mt:]]

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

/r/2325 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/2327 - Bug 975144 - Implementation of RTC identity DOM component
/r/2329 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope
/r/2331 - Bug 975144 - Tests for rtcIdentityProvider property
/r/2333 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/2335 - Bug 975144 - Updating test IdP for new API
/r/2337 - Bug 975144 - Updating RTC identity tests
/r/2339 - Bug 975144 - Adding IdP loading tests
/r/2341 - Bug 975144 - Enabling tests on e10s
/r/3569 - Bug 975144 - WebIDL changes for identity error handling refactor
/r/3571 - Bug 975144 - Moving to fold identity errors into the promises we return
/r/3573 - Bug 975144 - Updating identity tests to use promises

Pull down these commits:

hg pull review -r 6e08b6d787f753a8b8820b94b40bd4ddf2845e58

Comment 29

[Martin Thomson [:mt:]]

This has been working for me for a while, but try might have different ideas.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b66dded8756

Comment 30

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

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

https://reviewboard.mozilla.org/r/2323/#review2823

Had popcorn with this one. Overall looks good! Just some small things and some questions.

::: dom/media/PeerConnection.js
(Diff revisions 1 - 2)
> +        return p.then(sdp => {
> +          return new this._win.mozRTCSessionDescription({ type: "offer", sdp: sdp });
> +        });

one-liner?

::: dom/media/IdpSandbox.jsm
(Diff revisions 1 - 2)
> +    if (!Cu.isXrayWrapper(registrar)) {
> +      throw new Error('IdP setup failed');
> +    }
> +
> +    this._populateSandbox(result.request.URI);

What's the significance of using result.request.URI over say this.source? (I'm sure there's a good reason). 

Maybe a comment?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -  /**
> -   * Generate an error event of the identified type;
> +  reportError: function(type, message) {
> +    throw new this._win.DOMException('RTC identity error: ' + message, type);

I would switch argument order here, I think (not sure). Might as well get used to it?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -      callback(null);
> +      return Promise.resolve();

Promise.resolve(null);

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -    if (typeof name !== "string") {
> -      return "name not a string";
> +    let error = msg => {
> +      this.reportError('IdpError', 'assertion name error: ' + msg);
> +    };

One-liner?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -    this._sendToIdp(request, "validation", onVerification.bind(this));
> +    let compareFingerprints = (a, b) => {
> +      return (a.digest === b.digest) && (a.algorithm === b.algorithm);
> +    };

One-liner?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -    let request = {
> -      type: "VERIFY",
> -      message: assertion,
> -      origin: origin
> +    // everything in `innerSet` is found in `outerSet`
> +    let isSubsetOf = (outerSet, innerSet, comparator) => {
> +      return innerSet.every(i => {
> +        return outerSet.some(o => comparator(i, o));
> +      });

Mega-one-liner?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -   * Wraps a callback, adding a timeout and ensuring that the callback doesn't
> -   * receive any message other than one where the IdP generated a "SUCCESS"
> -   * response.
> +   * Wraps a promise, adding a timeout guard on it so that it can't take longer
> +   * than the specified time.  Returns a promise that always resolves; if there
> +   * is a problem the resolved value is undefined.

Update comment

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -      } else {
> +    let assertionPromise = this.start()

Why not p?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -    let onAssertion = function() {
> +    let validationPromise = this.start()

Why not p?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -   */
> +               let message = '' + (e.message || e || 'IdP error');

Did you mean to potentially output [object Object] here? (not saying that could happen...)

Could do typeof e == "string" or JSON.stringify(e) maybe?

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> -    this.reportError(type, "received response of type '" +
> +               return reject(error);

Why return?

::: dom/media/PeerConnection.js
(Diff revision 2)
> +        this._peerIdentity = new this._win.Promise((resolve, reject) => {
> +          this._resolvePeerIdentity = resolve;
> +          this._rejectPeerIdentity = reject;
> +        });
> +        this._rejectPeerIdentity(e);

Oops. You're rejecting the new promise here!

It's safe to reject the old promise first.

::: dom/media/PeerConnection.js
(Diff revision 2)
> +        // If we don't expect a specific peer identity, failure is not a
> +        // terminal state, so replace the promise to allow another attempt.

Puzzled by this comment, since this code will run even for "IncompatibleSessionDescriptionError". Can you clarify?

::: dom/media/tests/mochitest/identity/test_fingerprints.html
(Diff revisions 1 - 2)
> +         (e.message ? (e.message + '\n' + e.stack) : e));

e.name too?

::: dom/media/tests/mochitest/identity/test_setIdentityProviderWithErrors.html
(Diff revisions 1 - 2)
> +    function PEER_IDENTITY_IS_EMPTY(t) {
> +      return Promise.race([
> +        t.pcLocal._pc.peerIdentity.then(
> +          () => ok(false, t.pcLocal + ' incorrectly received valid peer identity'),
> +          e => ok(e, t.pcLocal + ' correctly failed to validate peer identity')),
> +        t.pcRemote._pc.peerIdentity.then(
> +          () => ok(false, t.pcRemote + ' incorrecly received a valid peer identity'),
> +          e => ok(false, t.pcRemote + ' incorrectly rejected peer identity'))
> +      ]);

Don't you mean Promise.all?

::: dom/media/PeerConnection.js
(Diff revision 2)
> -        // Do setRemoteDescription and identity validation in parallel
> +      // Do setRemoteDescription and identity validation in parallel
> -        let p = new this._win.Promise((resolve, reject) => {
> +      return this._chain(() => this._win.Promise.all([
> +        new this._win.Promise((resolve, reject) => {
>            this._onSetRemoteDescriptionSuccess = resolve;
>            this._onSetRemoteDescriptionFailure = reject;
>            this._impl.setRemoteDescription(type, desc.sdp);
> +        }),
> +        this._validateIdentity(desc.sdp, origin)
> +      ]));

You drop the tail here:

    .then(() => {});
    
setRemoteDescription's promise must resolve with undefined.

::: dom/media/PeerConnectionIdp.jsm
(Diff revision 2)
> +  // start the IdP and do some error fixup
> +  start: function() {
> +    return this._idp.start()
> +      .catch(e => this.reportError('IdpError', e.message));

Maybe s/reportError/throwError/ ?

Otherwise this reads like a catch-and-continue at the call-site (like it used to be), a bit confusing.

::: dom/media/PeerConnection.js
(Diff revisions 1 - 2)
> -  createOffer: function(optionsOrOnSuccess, onError, options) {
> +  _addIdentityAssertion: function(p, origin) {
> +    if (this._localIdp.enabled) {
> +      return this._localIdp.getIdentityAssertion(this._impl.fingerprint, origin)
> +        .then(() => p)
> +        .then(sdp => this._localIdp.addIdentityAttribute(sdp));
> +    }
> +    return p;
> +  },

Nice!

(and bail-pattern would save you an indent here)

::: dom/webidl/RTCPeerConnection.webidl
(Diff revision 2)
> -  readonly attribute RTCIdentityAssertion? peerIdentity;
> +  readonly attribute Promise<RTCIdentityAssertion> peerIdentity;
> +  [Pref="media.peerconnection.identity.enabled"]
> +  readonly attribute Promise<DOMString> idpLoginUrl;

Do you have a link to a spec, proposal or napkin so I can read about how these are supposed to work? ;-)
You'll need DOM review on these of course.

On naming: having the name of the promise be the name of its value seems a bit odd. Have you considered pc.havePeerIdentity? Then people can write:

    pc.havePeerIdentity.then(identity => {...});
    pc.haveIdpLoginUrl.then(url => {...});

Lastly, couldn't idpLoginUrl just be a string, since it's always fulfilled in response to an IdpError? E.g. users can just rely on .catch instead of this promise?

::: dom/media/PeerConnection.js
(Diff revisions 1 - 2)
> +    this._lastIdentityValidation = this._win.Promise.resolve();

Nit: Probably don't need this._win for this internal promise, since it's never passed out?

::: dom/media/PeerConnection.js
(Diff revisions 1 - 2)
>          // If we don't expect a specific peer identity, failure is not a
>          // terminal state, so replace the promise to allow another attempt.
>          this._peerIdentity = new this._win.Promise((resolve, reject) => {
>            this._resolvePeerIdentity = resolve;
>            this._rejectPeerIdentity = reject;
>          });
>          this._rejectPeerIdentity(e);
>          throw e;
> -    });
> +      });
> +    this._lastIdentityValidation = validation.catch(() => {});
> +
> +    // Only wait for IdP validation if we need identity matching
> +    return expectedIdentity ? validation : this._win.Promise.resolve();

Hmm, I see new code here that Reviewboard interdiff missed (Reviewboard bug?) Looks like I'll have to skim the whole patch as well. :-(

::: dom/media/tests/mochitest/identity/test_getIdentityAssertion.html
(Diff revisions 1 - 2)
> -        t.next();
> -      };
> -      t.pcLocal._pc.getIdentityAssertion();
> -      t.pcLocal._pc.getIdentityAssertion();
> +        getAssertion(t, '#loginerror')
> +          .then(a => ok(false, '#loginerror should not work'),
> +                e => is(e.name, 'IdpLoginError', 'name is IdpLoginError')),
> +        t.pcLocal._pc.idpLoginUrl

E.g. here, the getAssertion catch function could have just read out the string t.pcLocal._pc.idpLoginUrl synchronously, right?

(in reference to question elsewhere on "why is idpLoginUrl a promise?")

Comment 31

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2323/#review3003

> Nit: Probably don't need this._win for this internal promise, since it's never passed out?

It chains in when you have a target peer identity, so I think that we need it.  I had *so* much trouble with these that I'm very cautious about which compartment objects are created in now.

> one-liner?

I try very hard to maintain an 80 column limit, but I can fix that.

> Hmm, I see new code here that Reviewboard interdiff missed (Reviewboard bug?) Looks like I'll have to skim the whole patch as well. :-(

I'm not yet willing to say whether the RB interdiff is good or not.

> e.name too?

The stack is usually good enough here.  And it's a failure case thing.

> Don't you mean Promise.all?

Notice how one of these is ok(false) on both legs? I'll add a comment.

> Puzzled by this comment, since this code will run even for "IncompatibleSessionDescriptionError". Can you clarify?

Ahh, I simply forgot to check if we had a target peer identity before doing the replacement.

> Oops. You're rejecting the new promise here!
> 
> It's safe to reject the old promise first.

Yeah, oops.

> Maybe s/reportError/throwError/ ?
> 
> Otherwise this reads like a catch-and-continue at the call-site (like it used to be), a bit confusing.

I was planning to inline the method anyway.

> Mega-one-liner?

In this case, I think that having the blocks - unnecessary as they might be - makes the precedence of the various pieces clearer.

> Did you mean to potentially output [object Object] here? (not saying that could happen...)
> 
> Could do typeof e == "string" or JSON.stringify(e) maybe?

Yes, I considered that.  I was on the fence on it.  I guess that stringification is safe enough.

> Why return?

Yeah, return is odd.

> Do you have a link to a spec, proposal or napkin so I can read about how these are supposed to work? ;-)
> You'll need DOM review on these of course.
> 
> On naming: having the name of the promise be the name of its value seems a bit odd. Have you considered pc.havePeerIdentity? Then people can write:
> 
>     pc.havePeerIdentity.then(identity => {...});
>     pc.haveIdpLoginUrl.then(url => {...});
> 
> Lastly, couldn't idpLoginUrl just be a string, since it's always fulfilled in response to an IdpError? E.g. users can just rely on .catch instead of this promise?

I've a pull request on the spec here: https://github.com/w3c/webrtc-pc/pull/178

I'll change the login URL thing.  That's a small (and easy) change.

As for names, well...

Comment 32

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21f4d67e203c

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

/r/2325 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/2327 - Bug 975144 - Implementation of RTC identity DOM component
/r/2329 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope, r=gabor
/r/2331 - Bug 975144 - Tests for rtcIdentityProvider property, r=gabor
/r/2333 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/2335 - Bug 975144 - Updating test IdP for new API
/r/2337 - Bug 975144 - Updating RTC identity tests
/r/2339 - Bug 975144 - Adding IdP loading tests
/r/2341 - Bug 975144 - Enabling tests on e10s
/r/3569 - Bug 975144 - WebIDL changes for identity error handling refactor
/r/3571 - Bug 975144 - Moving to fold identity errors into the promises we return
/r/3573 - Bug 975144 - Updating identity tests to use promises

Pull down these commits:

hg pull review -r 1965abc08f1ff554499d6d083be35949de45d809

Comment 34

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

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

https://reviewboard.mozilla.org/r/2323/#review3013

::: dom/media/PeerConnection.js
(Diff revisions 2 - 3)
>            // Set new identity and generate an event.
>            this._impl.peerIdentity = msg.identity;
>            let assertion = new this._win.RTCIdentityAssertion(
>              this._remoteIdp.provider, msg.identity);
>            this._resolvePeerIdentity(assertion);
>          }
>        })
>        .catch(e => {
> -        // If we don't expect a specific peer identity, failure is not a
> -        // terminal state, so replace the promise to allow another attempt.
> +        this._rejectPeerIdentity(e);
> +        // If we don't expect a specific peer identity, failure to get a valid
> +        // peer identity is not a terminal state, so replace the promise to
> +        // allow another attempt.
> +        if (!this._impl.peerIdentity) {

Logic nit: I think the last line here would be better as:

    if (!expectedIdentity) { ?

to gel with the comment and to close a risk window where a failure in RTCIdentityAssertion() above (e.g. software bug) would put us in an odd state of "unresolved success".

::: dom/media/PeerConnectionIdp.jsm
(Diff revisions 2 - 3)
> -                 if (typeof e.loginUrl === 'string') {
> +            if (typeof e.loginUrl === 'string') {
> -                   this._resolveLoginUrl(e.loginUrl);
> +              this.idpLoginUrl = e.loginUrl;
> -                 }
> +            }

So if no loginUrl is returned with this IdpLoginError, we leave the previous value?

::: dom/media/tests/mochitest/identity/test_setIdentityProviderWithErrors.html
(Diff revisions 2 - 3)
> +  // Save the peerIdentity promises now, since when they reject they are
> +  // replaced and we expect them to be rejected this time
> +  var peerIdentityLocal = test.pcLocal._pc.peerIdentity;
> +  var peerIdentityRemote = test.pcRemote._pc.peerIdentity;

I missed that one. A bit of a trap this. I see no good way around it though.

::: dom/media/PeerConnection.js
(Diff revisions 2 - 3)
> -      ]));
> +      ])).then(() => {}); // must return undefined

// must resolve with undefined.

Comment 35

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/2325/#review3287

::: dom/webidl/RTCIdentityProvider.webidl
(Diff revision 3)
> + * This is currently only a proposal.

This needs either a spec link or a lot more documentation in the IDL explaining what all it all does.

And clearly the ChromeOnly bits are not going to be in the spec and hence need documentation in the IDL.

::: dom/webidl/RTCIdentityProvider.webidl
(Diff revision 3)
> +dictionary RTCIdentityAssertionResult {

This is only used in return values, as are the other dictionaries here, right?

Comment 36

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2325/#review3289

> This is only used in return values, as are the other dictionaries here, right?

Correct.

> This needs either a spec link or a lot more documentation in the IDL explaining what all it all does.
> 
> And clearly the ChromeOnly bits are not going to be in the spec and hence need documentation in the IDL.

I'll try to have comments added by the time you look at the other one.

Comment 37

[Martin Thomson [:mt:]]

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

/r/2325 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/2327 - Bug 975144 - Implementation of RTC identity DOM component
/r/2329 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope, r=gabor
/r/2331 - Bug 975144 - Tests for rtcIdentityProvider property, r=gabor
/r/2333 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/2335 - Bug 975144 - Updating test IdP for new API
/r/2337 - Bug 975144 - Updating RTC identity tests
/r/2339 - Bug 975144 - Adding IdP loading tests
/r/2341 - Bug 975144 - Enabling tests on e10s
/r/3569 - Bug 975144 - WebIDL changes for identity error handling refactor
/r/3571 - Bug 975144 - Moving to fold identity errors into the promises we return
/r/3573 - Bug 975144 - Updating identity tests to use promises

Pull down these commits:

hg pull review -r 39f67ec8ae8000e76b7a5cb35cc6466d7777cc5e

Comment 38

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/3569/#review3291

::: dom/webidl/RTCPeerConnection.webidl
(Diff revision 2)
> -  readonly attribute RTCIdentityAssertion? peerIdentity;
> +  readonly attribute Promise<RTCIdentityAssertion> peerIdentity;

Is this always the same promise?  Or sometimes a new one?  I assume the spec documents this all?

Comment 39

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/3569/#review3293

> Is this always the same promise?  Or sometimes a new one?  I assume the spec documents this all?

It changes in certain circumstances (sometimes when rejected actually).  It's here https://github.com/w3c/webrtc-pc/pull/178/files#diff-25266486aad3fa9244c53420694e037eR4299

Comment 40

[Martin Thomson [:mt:]]

Comment on attachment 8546927 [details]
MozReview Request: bz://975144/mt

Adding r=jib, bz

Comment 41

[Martin Thomson [:mt:]]

Some more testing as a sanity check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f640994cee34
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3839dfc40da4

Comment 42

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-975144-Disabling-b2g-tests-due-to-bug-1135339-r-.patch

Well, at some point in this process, b2g emulator regressed.  I'd like to land this without the test landed to avoid further delays.  :jib, I'd like a second opinion.

Comment 43

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

Comment on attachment 8567467 [details] [diff] [review]
0001-Bug-975144-Disabling-b2g-tests-due-to-bug-1135339-r-.patch

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

Makes sense.

Comment 44

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=24c42b4b9694

Comment 45

[Martin Thomson [:mt:]]

Oh, and I forgot to add a link to the final try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18588074d24a

Some odd stuff on some of those, but nothing related to these changes as far as I could see.

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d3bb6e25fd04
https://hg.mozilla.org/mozilla-central/rev/8859d04f962a
https://hg.mozilla.org/mozilla-central/rev/7cf45873e368
https://hg.mozilla.org/mozilla-central/rev/8fc35500a37b
https://hg.mozilla.org/mozilla-central/rev/c516431e7ad7
https://hg.mozilla.org/mozilla-central/rev/1b46f2423156
https://hg.mozilla.org/mozilla-central/rev/0e7fed532bd5
https://hg.mozilla.org/mozilla-central/rev/fb5b01aec70f
https://hg.mozilla.org/mozilla-central/rev/beacd5e98677
https://hg.mozilla.org/mozilla-central/rev/54054ad9997c
https://hg.mozilla.org/mozilla-central/rev/0cbedfe1417d
https://hg.mozilla.org/mozilla-central/rev/fdeb0075ab87
https://hg.mozilla.org/mozilla-central/rev/24c42b4b9694

Comment 47

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

Seriously, we need to do something about Review Board. I just got 10 review requests and almost fainted.

Comment 48

[Eric Shepherd [:sheppy]]

Will has updated the Sandbox page; teoli or I will get the new and changed interfaces documented.

Comment 50

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Updating RTC identity tests

Comment 51

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Adding IdP loading tests

Comment 52

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Enabling tests on e10s

Comment 53

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Updating identity tests to use promises

Comment 54

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - WebIDL changes for identity error handling refactor

Comment 55

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Moving to fold identity errors into the promises we return

Comment 56

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - WebIDL changes for RTC identity sandbox

Comment 57

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Implementation of RTC identity DOM component

Comment 58

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Adding rtcIdentityProvider to sandbox global scope, r=gabor

Comment 59

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Tests for rtcIdentityProvider property, r=gabor

Comment 60

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Rework RTC identity to use JS sandbox

Comment 61

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 975144 - Updating test IdP for new API