1153314 - XHR from IdP sandbox fails

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Martin Thomson [:mt:]]

It turns out that the loading code is broken: it assigns a principal to the sandbox that isn't fully initialized.

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://1153314/mt

/r/6883 - Bug 1153294 - Exposing Document.documentLoadGroup to Chrome JS, r=sicking
/r/6885 - Bug 1153314 - Assign load group to IdP load channel
/r/6887 - Bug 1153314 - Test IdP login flow

Pull down these commits:

hg pull -r a107d64e1e2981694c05ccaf215575619660f3a7 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Martin Thomson [:mt:]]

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

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8590931 [details]
MozReview Request: bz://1153314/mt

/r/6883 - Bug 1153294 - Exposing Document.documentLoadGroup to Chrome JS, r=sicking
/r/6885 - Bug 1153314 - Assign load group to IdP load channel
/r/6887 - Bug 1153314 - Test IdP login flow

Pull down these commits:

hg pull -r 878d6fc42af7f8050ce2025b44a9de5f87b63f0f https://reviewboard-hg.mozilla.org/gecko/

Comment 4

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

https://reviewboard.mozilla.org/r/6887/#review5759

lgtm, though you're getting "doc is undefined" on try so something is wrong. I can't spot it though.

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 2)
> -  // world with files all containing near-identical code, let's use the hash/URL
> +  // world with files all containing near-identical code, let"s use the hash/URL

collateral damage

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +    bug: "TBD"

.

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +      dump('   dddddddddddddddddddddddddddddddddddddddddddddddddddddddd\n');

litter

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +  }, false);

Promise() only takes one argument.

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +    .then(waitForLoginDone)

Any reason not to inline waitForLoginDone here?

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +  return new Promise(resolve => {
> +    var listener = e => {
> +      dump('   dddddddddddddddddddddddddddddddddddddddddddddddddddddddd\n');
> +      is(e.origin, 'https://example.com', 'got the right message origin');
> +      is(e.data, 'LOGINDONE', 'got the right message');
> +      window.removeEventListener('message', listener);
> +      resolve();
> +    }
> +    window.addEventListener('message', listener);
> +  }, false);

How about:

    var waitForLoginDone = () =>
      Promise(resolve =>
        window.addEventListener('message', function listener() {
          is(e.origin, 'https://example.com', 'got the right message origin');
          is(e.data, 'LOGINDONE', 'got the right message');
          resolve(window.removeEventListener('message', listener));
        }));
?

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +function checkLogin(t, loadLoginUrl) {
> +  t.pcLocal.setIdentityProvider('example.com', 'idp.js#login:' + loadLoginUrl.name);

How about s/loadLoginUrl/onLoginUrl/ ? Or onLoginNeeded? I generally expect function args to speak to the caller, not the callee, so this one threw me for a minute. I also thought it was a URL rather than a callback at first.

::: dom/media/tests/mochitest/identity/test_loginNeeded.html
(Diff revision 2)
> +    .then(a => ok(a, 'got assertion'));

Any validation here other than truthy?

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 2)
> +    this.id = crypto.getRandomValues(new Uint8Array(10)).join(",").split(",")
> +      .map(x => parseInt(x, 10).toString(16)).join("");

nice

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 2)
> +        xhr.onload = e => resolve(xhr.status === 200);

I wont get into the === thing, even though I loathe it so... Seems established here.

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 2)
> -    dump('idp: result=' + JSON.stringify(result) + '\n');
> +      dump("idp: result=" + JSON.stringify(result) + "\n");

We like double-quotes again? How come? (not a nit, just curious. I was starting to get on-board the other direction).

::: dom/media/tests/mochitest/identity/login.html
(Diff revision 2)
> +        setTimeout(() => window.close(), 500);

Any intermittent risk here? I'm looking at you, B2G emulator!

Comment 5

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

https://reviewboard.mozilla.org/r/6885/#review5755

lgtm.

Comment 6

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/6887/#review5815

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

I pushed with uncommitted changes the first time.  Then I spent 15 minutes trying to diagnose treeherder.  I still can't login.

I note that that push has what appears to be infrastructure-bustage.  I'll push another one in a few.

> nice

Yeah, this is a horrific piece of code, and it doesn't need to be.  I'm just going to drop the hex thing and add dots between atoms.  Less code, bigger `id` value.

> Any intermittent risk here? I'm looking at you, B2G emulator!

Actually, the work is already done, so closing immediately is safe.  I wanted to be able to eyeball the window when debugging and simply forgot to remove it.

> Promise() only takes one argument.

That was intended as an argument to `window.addEventListener()`; I clearly screwed that up (and don't we love Javascript for letting me get away with it).

> How about s/loadLoginUrl/onLoginUrl/ ? Or onLoginNeeded? I generally expect function args to speak to the caller, not the callee, so this one threw me for a minute. I also thought it was a URL rather than a callback at first.

Good point.  That makes it much clearer.

> Any reason not to inline waitForLoginDone here?

Not really, but the extra documentation afforded by having it out of line has benefits too.

> Any validation here other than truthy?

typeof a === 'string' might work.  The assertion is supposed to be opaque.  test_getIdentityAssertion.html actually unrolls the assertion provided by the test IdP, but I want to make sure that the unpacking only happens in the one test, because it's brittle.

> collateral damage

Yeah, I've decided to go with Google style guidelines and use single quotes.  I did the double-quote thing here because that seemed to be more consistent, but trying to maintain consistency is a mugs game.

Comment 7

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

(In reply to Martin Thomson [:mt] from comment #6)
> > nice
> 
> Yeah, this is a horrific piece of code, and it doesn't need to be.  I'm just
> going to drop the hex thing and add dots between atoms.  Less code, bigger
> `id` value.

No that wasn't sarcasm, I actually enjoyed it. I tried to make it shorter but I couldn't.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8590931 [details]
MozReview Request: bz://1153314/mt

/r/6883 - Bug 1153294 - Exposing Document.documentLoadGroup to Chrome JS, r=sicking
/r/6885 - Bug 1153314 - Assign load group to IdP load channel
/r/6887 - Bug 1153314 - Test IdP login flow

Pull down these commits:

hg pull -r db2401276bca77d64f293c418288dbe5a9f6ebd8 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f5f54f07e9b

Bug 1153094 is a serious distraction.

Comment 10

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

Comment on attachment 8590931 [details]
MozReview Request: bz://1153314/mt

https://reviewboard.mozilla.org/r/6881/#review5863

Ship It!

Comment 11

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f23689de6216
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfa03945585

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f23689de6216
https://hg.mozilla.org/mozilla-central/rev/bbfa03945585

Comment 14

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1153294 - Exposing Document.documentLoadGroup to Chrome JS, r=sicking

Comment 15

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1153314 - Assign load group to IdP load channel

Comment 16

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1153314 - Test IdP login flow