XHR from IdP sandbox fails

RESOLVED FIXED in Firefox 40

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
It turns out that the loading code is broken: it assigns a principal to the sandbox that isn't fully initialized.
(Assignee)

Comment 1

3 years ago
Created 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 a107d64e1e2981694c05ccaf215575619660f3a7 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590931 - Flags: review?(jib)
(Assignee)

Comment 2

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2ded78bb7a8
(Assignee)

Comment 3

3 years ago
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/
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!
https://reviewboard.mozilla.org/r/6885/#review5755

lgtm.
(Assignee)

Comment 6

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

Comment 8

3 years ago
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/
(Assignee)

Comment 9

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

Bug 1153094 is a serious distraction.
Comment on attachment 8590931 [details]
MozReview Request: bz://1153314/mt

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

Ship It!
Attachment #8590931 - Flags: review?(jib) → review+
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23689de6216
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfa03945585
https://hg.mozilla.org/mozilla-central/rev/f23689de6216
https://hg.mozilla.org/mozilla-central/rev/bbfa03945585
Assignee: nobody → martin.thomson
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 13

3 years ago
Comment on attachment 8590931 [details]
MozReview Request: bz://1153314/mt
Attachment #8590931 - Attachment is obsolete: true
Attachment #8620019 - Flags: review+
Attachment #8620020 - Flags: review+
Attachment #8620021 - Flags: review+
(Assignee)

Comment 14

3 years ago
Created attachment 8620019 [details]
MozReview Request: Bug 1153294 - Exposing Document.documentLoadGroup to Chrome JS, r=sicking
(Assignee)

Comment 15

3 years ago
Created attachment 8620020 [details]
MozReview Request: Bug 1153314 - Assign load group to IdP load channel
(Assignee)

Comment 16

3 years ago
Created attachment 8620021 [details]
MozReview Request: Bug 1153314 - Test IdP login flow
You need to log in before you can comment on or make changes to this bug.