Support electrolysis for WebRTC identity

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

(Blocks 2 bugs, {dev-doc-needed})

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(13 attachments, 2 obsolete attachments)

1.40 KB, patch
jib
: review+
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 995328
Assignee

Updated

5 years ago
Depends on: 966066
Assignee

Comment 1

5 years ago
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).
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
Attachment #8412907 - Flags: review?(wmccloskey)
Attachment #8412907 - Flags: review?(jib)
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.
Attachment #8412907 - Flags: review?(jib) → review+
Assignee

Comment 3

5 years ago
(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.
Assignee

Comment 4

5 years ago
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.
Attachment #8412907 - Attachment is patch: false
Attachment #8412907 - Flags: review?(wmccloskey)
Assignee

Updated

5 years ago
Depends on: 1063838
Assignee

Comment 5

5 years ago
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.
Attachment #8412907 - Attachment is obsolete: true
Attachment #8412907 - Flags: review+ → review-
Assignee

Comment 6

5 years ago
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.
Assignee

Updated

5 years ago
No longer depends on: 1063838
Assignee

Updated

5 years ago
Depends on: 1116269
Assignee

Updated

5 years ago
Depends on: 1107953
Assignee

Comment 8

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

Comment 9

5 years ago
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.
Assignee

Comment 10

5 years ago
Posted file MozReview Request: bz://975144/mt (obsolete) —
Attachment #8546927 - Flags: review?(jonas)
Attachment #8546927 - Flags: review?(jib)
Attachment #8546927 - Flags: review?(gkrizsanits)
Attachment #8546927 - Flags: review?(bobbyholley)
Assignee

Comment 11

5 years ago
/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
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).
Attachment #8546927 - Flags: review?(bobbyholley) → review+
Attachment #8546927 - Flags: review?(gkrizsanits) → review+
I don't know what else needs documentation here but for sure we added a new Sandbox global property: rtcIdentityProvider
Keywords: dev-doc-needed
https://reviewboard.mozilla.org/r/2327/#review1557

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

Technically redundant
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.
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/ ?
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.
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
Attachment #8546927 - Flags: review?(jib) → review-
Assignee

Comment 23

5 years ago
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.
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 => {
Assignee

Comment 25

5 years ago
(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.
(In reply to Martin Thomson [:mt] from comment #25)
> On the second line, `assertion` is undefined.

Doh!
You could lift it out, but not worth it.
Assignee

Updated

4 years ago
Attachment #8546927 - Flags: review?(jib)
Attachment #8546927 - Flags: review-
Attachment #8546927 - Flags: review+
Assignee

Comment 28

4 years ago
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
Assignee

Comment 29

4 years ago
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 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?")
Attachment #8546927 - Flags: review?(jib)
Assignee

Comment 31

4 years ago
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...
Assignee

Comment 33

4 years ago
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
Attachment #8546927 - Flags: review?(jib)
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.
Attachment #8546927 - Flags: review?(jib) → review+
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?
Assignee

Comment 36

4 years ago
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.
Assignee

Comment 37

4 years ago
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
Attachment #8546927 - Flags: review?(jonas)
Attachment #8546927 - Flags: review?(jib)
Attachment #8546927 - Flags: review?(bzbarsky)
Attachment #8546927 - Flags: review+
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?
Assignee

Comment 39

4 years ago
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
Assignee

Comment 40

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

Adding r=jib, bz
Attachment #8546927 - Flags: review?(jib)
Attachment #8546927 - Flags: review?(bzbarsky)
Attachment #8546927 - Flags: review+
Assignee

Comment 42

4 years ago
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.
Attachment #8567467 - Flags: review?(jib)
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.
Attachment #8567467 - Flags: review?(jib) → review+
Assignee

Comment 45

4 years ago
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.
Depends on: 1135825
Seriously, we need to do something about Review Board. I just got 10 review requests and almost fainted.
Will has updated the Sandbox page; teoli or I will get the new and changed interfaces documented.
Assignee

Comment 49

4 years ago
Attachment #8546927 - Attachment is obsolete: true
Attachment #8618077 - Flags: review+
Attachment #8618078 - Flags: review+
Attachment #8618079 - Flags: review+
Attachment #8618080 - Flags: review+
Attachment #8618081 - Flags: review+
Attachment #8618082 - Flags: review+
Attachment #8618083 - Flags: review+
Attachment #8618084 - Flags: review+
Attachment #8618085 - Flags: review+
Attachment #8618086 - Flags: review+
Attachment #8618087 - Flags: review+
Attachment #8618088 - Flags: review+
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.