Closed Bug 1446880 Opened 6 years ago Closed 6 years ago

Update Identity implementation

Categories

(Core :: WebRTC, enhancement, P2)

Product:
Core
Component:
WebRTC
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(4 files)

Bug 1446880: update WebRTC Peer Identity WebIDL.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1446880: update setIdentityProvider() to take RTCIdentityProviderOptions.
59 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1446880: updated IDP interface to use RTCIdentityProviderOptions.
59 bytes, text/x-review-board-request
mt
: review+
Details
Bug 1446880: updated exception test to new setIdentityProvider API.
59 bytes, text/x-review-board-request
jib
: review+
Details 
Firefox WebRTC Identity implementation is not up to spec any more.
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)
Looks like we should also replace the IplErros with RTCError. But that might better resolved in a separate new bug.
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 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 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 :)
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)
(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)
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 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)
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 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+
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
Flags: needinfo?(drno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: