Update Identity implementation

RESOLVED FIXED in Firefox 61

Status

()

Core
WebRTC
P2
normal
RESOLVED FIXED
a month ago
10 days ago

People

(Reporter: drno, Assigned: drno)

Tracking

Trunk
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

(Assignee)

Description

a month ago
Firefox WebRTC Identity implementation is not up to spec any more.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a month ago
The last mochitest is now failing like this:

Unexpected Logs
---------------
dom/media/tests/mochitest/identity/test_setIdentityProviderWithErrors.html
  FAIL Error in test execution: IdpError: Non-200 response from server: 404 Async*createAnswer@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:1149:12 ... PeerConnectionTest.prototype.createAnswer@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:310:10 ... PC_REMOTE_CREATE_ANSWER@http://mochi.test:8888/tests/dom/media/tests/mochitest/templates.js:287:12 ... execute/</<@http://mochi.test:8888/tests/dom/media/tests/mochitest/head.js:831:31 ...

Looks like the webserver basically delivers a 404. But I don't understand why this so far should have delivered a 200 ok.

Martin any idea what is going on here?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 4

a month ago
Looks like we should also replace the IplErros with RTCError. But that might better resolved in a separate new bug.

Comment 5

a month ago
mozreview-review
Comment on attachment 8960083 [details]
Bug 1446880: update WebRTC Peer Identity WebIDL.

https://reviewboard.mozilla.org/r/228874/#review234542
Attachment #8960083 - Flags: review?(bugs) → review+

Comment 6

a month ago
mozreview-review
Comment on attachment 8960084 [details]
Bug 1446880: update setIdentityProvider() to take RTCIdentityProviderOptions.

https://reviewboard.mozilla.org/r/228876/#review234642

I honestly can't see why this would make the test fail.  Maybe we should dig into the problem.  We should chat in person.
Attachment #8960084 - Flags: review?(martin.thomson) → review+
Comment hidden (mozreview-request)

Comment 8

a month ago
mozreview-review
Comment on attachment 8960084 [details]
Bug 1446880: update setIdentityProvider() to take RTCIdentityProviderOptions.

https://reviewboard.mozilla.org/r/228876/#review234700

::: dom/media/tests/mochitest/identity/test_setIdentityProviderWithErrors.html:22
(Diff revisions 1 - 2)
>    var test = new PeerConnectionTest();
>    test.setMediaConstraints([{audio: true}], [{audio: true}]);
>    // No IdP for local.
>    // Remote generates a bad assertion, but that only fails to validate
>    test.pcRemote.setIdentityProvider('example.com',
> -                                    { procotol: 'idp.js#bad-validate',
> +                                    { protocol: 'idp.js#bad-validate',

OOps :)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a month ago
Bobby in attachment 8960626 [details] I'm trying to pass an object into the getIdentityAssertion() call as the spec currently demands.

Currently my guess is that my cloneInto() call works, but I'm failing to grab a reference to the object inside the IDP context which I can give to the function call. But it's also possible that I'm completely off track here so far. Any advice would be greatly appreciated.
Flags: needinfo?(bobbyholley)
Sorry for the lag, was on PTO.

I think I need to understand a bit more about the setup here.

Assuming this.protocol, this.username, and this.peerIdentity are all primitives, clonedOptions should be a vanilla object within |this._idp.sandbox|. But it's not clear to me what sandbox that is, what scope generateAssertion lives in, or what actual failure you're hitting for that matter. :-)
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 12

17 days ago
(In reply to Bobby Holley (:bholley) from comment #11)
> I think I need to understand a bit more about the setup here.

Sorry, yes should have included more details from the get go.
 
> Assuming this.protocol, this.username, and this.peerIdentity are all
> primitives,

These are all strings.

> clonedOptions should be a vanilla object within
> |this._idp.sandbox|. But it's not clear to me what sandbox that is, what
> scope generateAssertion lives in, or what actual failure you're hitting for
> that matter. :-)

:mt is the expert on this whole IDP sandbox topic. But this sandbox appears to get created here https://searchfox.org/mozilla-central/source/dom/media/IdpSandbox.jsm#112 or more precisely |this._idp.sandbox| gets populated here https://searchfox.org/mozilla-central/source/dom/media/IdpSandbox.jsm#197

So the dump() logging inside the IDP shows "[object Object]" even when using JSON.stringify() in the logging statement. I assume that means something converts the clonedOptions into a string before passing it over.

I don't really doubt that clonedOptions got cloned into the sandbox. But is it possible to a working reference to the cloned objection get passed through the function call into the IDP so that the code inside sandbox can access it right away?
I think I have seen other cases where things get cloned into well known global names inside the IDP/sandbox. If that is the only way to reference objects from inside the IDP/sandbox, then I would have to call into a helper function inside the IDP/sandbox which resolves that global name back into a function parameter.
Flags: needinfo?(martin.thomson) → needinfo?(bobbyholley)
We talked through this on IRC. Sounds like Nils had modified the WebIDL interface exposed to the content window, but hadn't modified the definition of the callback interface implemented by the JS code inside the sandbox.
Flags: needinfo?(bobbyholley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

16 days ago
smaug: turns out I did not update everything. And I decided to amend the first commit. But I can't figure out how to cancel your existing review and ask for re-review. Could you please re-review?
Flags: needinfo?(bugs)

Comment 19

16 days ago
mozreview-review
Comment on attachment 8960626 [details]
Bug 1446880: updated IDP interface to use RTCIdentityProviderOptions.

https://reviewboard.mozilla.org/r/229396/#review239050
Attachment #8960626 - Flags: review?(martin.thomson) → review+
r+
Flags: needinfo?(bugs)

Comment 22

15 days ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/50c255c237f1
update WebRTC Peer Identity WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/7142d472f2a4
update setIdentityProvider() to take RTCIdentityProviderOptions. r=mt
https://hg.mozilla.org/integration/autoland/rev/3d162d7e2721
updated IDP interface to use RTCIdentityProviderOptions. r=mt
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

15 days ago
mozreview-review
Comment on attachment 8965207 [details]
Bug 1446880: updated exception test to new setIdentityProvider API.

https://reviewboard.mozilla.org/r/233904/#review239522
Attachment #8965207 - Flags: review?(jib) → review+

Comment 29

15 days ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/41ddd2c3d501
update WebRTC Peer Identity WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/64c883d5d65f
update setIdentityProvider() to take RTCIdentityProviderOptions. r=mt
https://hg.mozilla.org/integration/autoland/rev/444064b5910e
updated IDP interface to use RTCIdentityProviderOptions. r=mt
https://hg.mozilla.org/integration/autoland/rev/22194a92dbc7
updated exception test to new setIdentityProvider API. r=jib

Comment 30

14 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41ddd2c3d501
https://hg.mozilla.org/mozilla-central/rev/64c883d5d65f
https://hg.mozilla.org/mozilla-central/rev/444064b5910e
https://hg.mozilla.org/mozilla-central/rev/22194a92dbc7
Status: NEW → RESOLVED
Last Resolved: 14 days ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Updated

10 days ago
Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.