753239 - Implement a DOM component for navigator.id

Status: RESOLVED FIXED

(Core Graveyard :: Identity, enhancement)

Description

[Anant Narayanan [:anant]]

Expose the navigator.id family of functions to content. The methods themselves will be implemented using some common code that lives in Identity.jsm, being built in 753238.

Comment 1

[Anant Narayanan [:anant]]

Attachment: WIP: skeleton for navigator.id DOM component & build changes

Setting up a skeleton - it builds and you can call navigator.id.watch/request!

Comment 2

[[:fabrice] Fabrice Desré]

From the code I see, DOMIdentity.jsm runs in the parent process, and nsDOMIdentity.js runs in the content process.

Comment 4

[Matthew N. [:MattN]]

Attachment: WIPv.1 Major functionality with RP tests

Majority of the navigator.id code is ready for review.  Draft spec is here: https://github.com/mozilla/id-specs/blob/dev/browserid/index.md

Known issues:
* IDP tests not done
* One remaining jsval in the IDL
* Some TODO comments

Comment 5

[Jason Smith [:jsmith]]

Could you explain the rationale for the k9o nomination?

Comment 6

[Matthew N. [:MattN]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Could you explain the rationale for the k9o nomination?

It's a P3 on the product draft:
"* P3: Native DOM bindings for sign into website"

Comment 7

[Jason Smith [:jsmith]]

(In reply to Matthew N. [:MattN] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Could you explain the rationale for the k9o nomination?
> 
> It's a P3 on the product draft:
> "* P3: Native DOM bindings for sign into website"

I believe we only block on P1s. Will know shortly in the triage to confirm.

Comment 8

[Ben Adida [:benadida]]

(In reply to Matthew N. [:MattN] from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Could you explain the rationale for the k9o nomination?
> 
> It's a P3 on the product draft:
> "* P3: Native DOM bindings for sign into website"

Yep, P3 in product, but it's been communicated to me as a P1 from B2G engineering.

Comment 9

[Asa Dotzler [:asa]]

b2g needs, desktop doesn't. blocking for b2g.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is on my list for this week, but we're going to need to find someone else to review the e10s bits.

Comment 11

[Josh Matthews [:jdm]]

What sort of review does the e10s code require? I took a look at it and didn't see anything wrong.

Comment 12

[Matthew N. [:MattN]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> This is on my list for this week, but we're going to need to find someone
> else to review the e10s bits.

Thanks.  I'll attach a new version with Felipe's changes to use fmm instead of cpmm and ppmm within the next few hours.  The change is needed to work on B2G.  You can  look at the code in the meantime at https://hg.mozilla.org/projects/pine/file/tip/dom/identity

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #11)
> What sort of review does the e10s code require? I took a look at it and
> didn't see anything wrong.

A review from somebody who has a clue how e10s works would be a good start.

Comment 14

[Matthew N. [:MattN]]

Attachment: v.1 More tests and fewer TODOs

Removed most of the TODOs and added IDP tests. I believe the only TODO that would block landing would be the one in the IDL: Ben, are we going to have the GenKeyPair callback return an object or string?

jst, can you give this an SR pass and/or get someone to help Kyle look at the e10s aspect?

Thanks all.

Comment 15

[Ben Adida [:benadida]]

(In reply to Matthew N. [:MattN] from comment #14)
> Ben, are we going to have
> the GenKeyPair callback return an object or string?

I think you mean *take* a string or object, because it returns nothing.

We are moving to having it take a string, but I'd like to not block on that, land this first, and then tweak implementation as the spec makes its final tweaks.

Comment 16

[Justin Dolske [:Dolske]]

Driveby question, since I don't know how nsIDOMGlobalPropertyInitializer works:

When dom.identity.enabled is set to false, navigator.id will not exist, right? And that's a startup check, so to enable/disable it one must restart the browser?

Comment 17

[Johnny Stenback (:jst)]

(In reply to Justin Dolske [:Dolske] from comment #16)
> Driveby question, since I don't know how nsIDOMGlobalPropertyInitializer
> works:
> 
> When dom.identity.enabled is set to false, navigator.id will not exist,
> right? And that's a startup check, so to enable/disable it one must restart
> the browser?

The way it's used in this patch navigator.id will exist, but it will be null. This code could be changed to listen for pref changes and register/unregster the category entry on pref change which would lead to navigator.id existing only in pages that were loaded while the pref was set, or if that gets complicated we could also come up with other ways to let the nsIDOMGlobalPropertyInitializer implementation signal that it doesn't actually want to be a property on the navigator object.

Comment 18

[Matthew N. [:MattN]]

Attachment: v.1.1 More tests, fewer TODOs and address comment from bug 764263

Incorporated Felipe's changes to address comments in bug 764263. Otherwise it's the same as v.1.

Ben would like to use navigator.id (unprefixed) while this is behind a DOM pref defaulting to off and then add the prefix ones we default to on in order to enable a multi-stage deployment. (Most existing sites and the shim only look for navigator.id IIRC).

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to Matthew N. [:MattN] from comment #18)
> Ben would like to use navigator.id (unprefixed) while this is behind a DOM
> pref defaulting to off and then add the prefix ones we default to on in
> order to enable a multi-stage deployment. (Most existing sites and the shim
> only look for navigator.id IIRC).

I don't understand what the advantage would be to developers and our users: they will enable the pref and see navigator.id and then assume it will come some day and at some point we will stop shipping navigator.id but navigator.mozId. Using navigator.id would just give a false impression that the feature will soon come without being behind a pref.
If we want to prefix, we should prefix from the beginning. Developers will see that and take it into account and even the shim can be updated to take the prefix into account too.

Comment 20

[Ben Adida [:benadida]]

(In reply to Mounir Lamouri (:mounir) from comment #19)
> I don't understand what the advantage would be to developers and our users:
> they will enable the pref

Who's going to enable this pref? I don't think we need to worry too much about this.

The goal of this approach is:

a) we can test the native implementation against real sites

b) once we turn it on by default, we still have a way, in the JS shim, to decide whether to hand off to native implementation or not, depending on potential incompatibilities.

In any case, we shouldn't turn on navigator.id by default until it's standardized with other browser vendors.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 635100 [details] [diff] [review]
v.1.1 More tests, fewer TODOs and address comment from bug 764263

+  _configureMessages: function DOMIdentity__configureMessages(aWindow, aRegister) {
+    if (!aWindow.messageManager)
+      return;
+
+    let func = aRegister ? "addMessageListener" : "removeMessageListener";
+
+    for (let message of this.messages) {
+      aWindow.messageManager[func](message, this);

Maybe save some cycles here and do:

    let func = aWindow.messageManager[aRegister ? "addMessageListener" : "removeMessageListener"]

    for (let message of this.messages) {
      func(message, this);

- In dom/identity/Identity.manifest:

>+category JavaScript-navigator-property id @mozilla.org/identity;1
>+category JavaScript-navigator-property mozId @mozilla.org/identity;1

Maybe throw "dom" into those contract ids to avoid polluting the global @mozilla.org namespace? I.e. @mozilla.org/dom/identity;1? And of course change every other place where this is used.

>+nsDOMIdentity.prototype = {
>+
>+  // nsIDOMIdentity
>+  /**
>+   * Relying Party (RP) APIs
>+   */
>+
>+  watch: function nsDOMIdentity_watch(aOptions) {

This function needs to protect against malicious callers that try to swamp this function with massive amounts of data, or massive number of calls. As we discussed on irc, we should probably throw if this function is ever called more than once per window.

+    for (let cbName of requiredCallbacks) {
+      if (typeof(aOptions[cbName].handleEvent) !== "function") {

if aOptions[cbName] is not set this will throw, check it before accessing .handleEvent. Same thing in a few more places below.

+    // loggedInEmai
+    // TODO: check email format? See nsHTMLInputElement::IsValidEmailAddress

I think we should at least do some sanity checking here, most importantly check that we're not being flooded with megabytes of data to push through our IPC layer or what not. Maybe put an arbitrary limit on the lenght of this string for now, i.e. 1k or somesuch? And file a followup bug to actually validate the email address.

+  request: function nsDOMIdentity_request(aOptions) {

Same here, do something sane if called lots of times.

+    // TODO: "This function must be invoked from within a click handler."
+    // This is doable once nsEventStateManager::IsHandlingUserInput is scriptable.

Do we need this before landing this? If we do, exposing this through nsDOMWindowUtils might be a better option than making nsEventStateManager itself scriptable etc.

Also, do some rudimentary validation of what we're given in the options here.

+  logout: function nsDOMIdentity_logout() {

We probably want to protect against this being called more than once too (i.e. make this a no-op if called more than once?)

+  beginProvisioning: function nsDOMIdentity_beginProvisioning(aCallback) {
+    this._log("beginProvisioning");
+    this._beginProvisioningCallback = aCallback;
+    this._mm.sendAsyncMessage("Identity:IDP:BeginProvisioning",
+                              this.DOMIdentityMessage());

Should we let this be called more than once? Same thing in the other functions below here...

+  // nsIFrameMessageListener
+  receiveMessage: function nsDOMIdentity_receiveMessage(aMessage) {

This living on the same object that's exposed to the webpage is not ok. There's nothing that prevents the webpage from calling QueryInterface() on this object to get to this method (same for init() etc as well). More about this below...

+  // nsIDOMGlobalPropertyInitializer
+  init: function nsDOMIdentity_init(aWindow) {

This method needs to return an object that is *not* the same object where you have other internal methods. It needs to create and return a *separate* object, and you have two options as to what that will look like. Either you return a vanilla JS object and use __exposedProps__ to explicitly control what you want to expose, or you use XPCOM, but limit the interfaces in question to the interfaces that you explicitly want untrusted code to call. If you go the __exposedProps__ way, which is probably the safer of the two, then you should so something like this:

function DOMIdentity(realIdentity) {
  this._realIdentity = realIdentity;
}
DOMIdentity.prototype = {
    __exposedProps__: {
	watch: 'r',
	request: 'r',
        ...
    },
    watch: function() {
      this._realIdentity.watch();
    },
    ...
}

and then return an instance of that function (or an instance of another XPCOM object) to the caller of init().

The key point here is that you need navigator.id to be an object that does not expose any internal XPCOM goop, as there's nothing that prevents untrusted code from calling QueryInterfae() etc here given that you've marked this object as a DOM_OBJECT in its classinfo flags. The object suggested above has nothing to do with XPCOM, and it explicitly defines what methods you want to expose to untrusted code in its __exposedProps__ property, so fill that in with what this API needs to look like (i.e. mimic nsIDOMIdentity), and make sure you never return any internal XPCOM objects to untrusted code. Exposing XPCOM stuff from JS is generally fine, but make sure they're completely disconnected from internal stuff, and know that untrusted code can call QueryInterface(), at least for now (some day we might be able to block that, but doing so gets complicated). Using __exposedProps__ gives you another layer of protection, so I would highly recommend using it.

+    this._log("init was called from " + aWindow.document.location);
+    if (!Services.prefs.getBoolPref("dom.identity.enabled"))
+      return null;
+
+    this._debug = Services.prefs.getBoolPref("toolkit.identity.debug");

Wrap the getBoolPref calls in a try/catch, getBoolPref will throw if the pref does not exist.

and this._log() above relies on this._debug, which is set after the log call. Move those around.

r- until that stuff has been dealt with. I do want to look over this a bit more once the above has been fixed.

Comment 22

[Justin Dolske [:Dolske]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #17)

> > When dom.identity.enabled is set to false, navigator.id will not exist,
> > right? And that's a startup check, so to enable/disable it one must restart
> > the browser?
> 
> The way it's used in this patch navigator.id will exist, but it will be
> null. This code could be changed to listen for pref changes and
> register/unregster the category entry on pref change

Ok. I think it's ok as-is. Sounds like making it observe the pref dynamically would be tricky, and probably leads to some weird cases.

Comment 23

[Matthew N. [:MattN]]

WIP patch to address the review comments except for:
* the major leak issue
* updating the tests to accommodate the new exceptions being thrown to prevent abuse
was pushed to pine at https://hg.mozilla.org/projects/pine/rev/6208138dca3b

Comment 24

[Matthew N. [:MattN]]

Attachment: WIP __exposedProps__ fix

I'm trying to implement the __exposedProps__ approach but I have some questions because I don't exactly understand what magic I want:
1) Do we even need the IDL anymore with this approach? 
2) Is there some XPCOM stuff we can get rid of with this approach? ie. "flags: Ci.nsIClassInfo.DOM_OBJECT"?
3) XPConnect was taking care of the data types for me because I was using the IDL.  Do I lose this benefit with this approach?

This patch is a WIP on top of the pine commit in comment 23 to try and prevent exposing Chrome stuff to content.  Indentation and style may be a bit off as I try to get this right.  I also may have missed updating some references to member properties.

Overall, am I splitting the two objects in a sensible/secure way? What leftover cruft can I remove?

Thanks in advance.

Comment 25

[Matthew N. [:MattN]]

Attachment: v.2 Address comment 21 review

Updated patch to address review comments and finished updating the tests to deal with the new security limits.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 637814 [details] [diff] [review]
v.2 Address comment 21 review

This is looking good! A couple of nits and minor things below...

- In dom/identity/DOMIdentity.jsm:

+  doBeginProvisioningCallback: function IDPPC_doBeginProvCB(aID, aCertDuration) {
+    let message = new IDDOMMessage(this.id);
+    message.identity = aID;
+    message.certDuration = aCertDuration;
+    this._mm.sendAsyncMessage("Identity:IDP:CallBeginProvisioningCallback",
+                             message);

Throw in another space to make the second line argument line up.

+IDPAuthenticationContext.prototype = {
+  get id() this._id,
+  get origin() this._origin,
+
+  doBeginAuthenticationCallback: function IDPAC_doBeginAuthCB(aIdentity) {
+    let message = new IDDOMMessage(this.id);
+    message.identity = aIdentity;
+    this._mm.sendAsyncMessage("Identity:IDP:CallBeginAuthenticationCallback",
+                             message);

Same thing.

- In nsDOMIdentityInternal.prototype:

+  // nsIDOMGlobalPropertyInitializer
+  init: function nsDOMIdentityInternal_init(aWindow) {
...
+
+    this._log("init was called from " + aWindow.document.location);
+
+    this._debug =
+      Services.prefs.getPrefType(PREF_DEBUG) == Ci.nsIPrefBranch.PREF_BOOL
+      && Services.prefs.getBoolPref(PREF_DEBUG);

this._log checks this._debug, so you should init this._debug before you call _log, or your log message will never be logged.

+  classInfo: XPCOMUtils.generateCI({
+    classID: Components.ID("{8bcac6a3-56a4-43a4-a44c-cdf42763002f}"),
+    contractID: "@mozilla.org/dom/identity;1",
+    interfaces: [Ci.nsIDOMIdentity],
+    flags: Ci.nsIClassInfo.DOM_OBJECT,
+    classDescription: "Identity DOM Implementation"
+  })

Given that you no longer expose this object to the webpage itself you don't need the DOM_OBJECT flag any more, and in fact you might be able to get away with having no classinfo at all. So remove DOM_OBJECT for sure, and if your tests still work w/o classInfo, drop that entirely as well. And nsIDOMIdentity is no longer used here since you simply use a vanilla JS object with __exposedProps__ as the object you hand out to the webpage. You can drop that interface entirely from the codebase, I think.

r=jst with that taken care of.

Comment 27

[Matthew N. [:MattN]]

Attachment: v.3 Address comments

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #26)
> So remove DOM_OBJECT for sure, and if your
> tests still work w/o classInfo, drop that entirely as well.

I didn't test without classInfo yet.

Comment 28

[Matthew N. [:MattN]]

Attachment: v.1 mochitests

These additional mochitests use the DOM APIs but also check the state of the identity JS modules.  They are in toolkit/identity/tests since we don't want the DOM code to have to know about all the internals of the JS modules and locating them near the code makes them easier to find.  We already have shallow mochitests in dom/identity/tests in attachment 638043 [details] [diff] [review].

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to Ben Adida [:benadida] from comment #20)
> (In reply to Mounir Lamouri (:mounir) from comment #19)
> > I don't understand what the advantage would be to developers and our users:
> > they will enable the pref
> 
> Who's going to enable this pref? I don't think we need to worry too much
> about this.

I don't see the point to expose an object foo to then change it to expose mozFoo and finally revert to foo. The fact that foo is behind a pref the first time doesn't make it more understandable.

> The goal of this approach is:
> 
> a) we can test the native implementation against real sites

Can't the shim just use mozId if present?

Comment 30

[Ben Adida [:benadida]]

(In reply to Mounir Lamouri (:mounir) from comment #29)
> I don't see the point to expose an object foo to then change it to expose
> mozFoo and finally revert to foo.

I explained it above. The point is that, while it's pref'ed off, I'd like to be able to pref it on and test actual sites. And then when we pref it on, I'd like to add one more layer of indirection because I expect the native implementation won't be quite up to the level of polish as the shim for a little while.

> Can't the shim just use mozId if present?

Eventually yes. But not right away, because level of polish of native is not where we need it to be.

Comment 31

[Mounir Lamouri (:mounir)]

(In reply to Ben Adida [:benadida] from comment #30)
> (In reply to Mounir Lamouri (:mounir) from comment #29)
> > I don't see the point to expose an object foo to then change it to expose
> > mozFoo and finally revert to foo.
> 
> I explained it above. The point is that, while it's pref'ed off, I'd like to
> be able to pref it on and test actual sites. And then when we pref it on,
> I'd like to add one more layer of indirection because I expect the native
> implementation won't be quite up to the level of polish as the shim for a
> little while.

And that would be doable because the shim is doing something like:
if ('id' in navigator) {
 // I will use the native implementation.
}

Right?

If that's the case, I don't understand why you guys don't change the shim to have it doing:
if (('id' in navigator) || ('mozId' in navigator) || ('webkitId' in navigator) || [add other prefixes here]) {
 // I will use the native implementation.
}

And then polish the native implementation. You can still keep the implementation pref'ed off and then pref it on when the implementation is good enough.

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 638880 [details] [diff] [review]
v.1 mochitests

Review of attachment 638880 [details] [diff] [review]:
-----------------------------------------------------------------

A few comments, though I didn't review in excruciating detail. :)

r+ with fixes.

::: toolkit/identity/tests/mochitest/head_identity.js
@@ +33,5 @@
> +const IdentityStore = Cu.import("resource://gre/modules/identity/IdentityStore.jsm").IdentityStore;
> +const RelyingParty = Cu.import("resource://gre/modules/identity/RelyingParty.jsm").RelyingParty;
> +const XPCOMUtils = Cu.import("resource://gre/modules/XPCOMUtils.jsm").XPCOMUtils;
> +const IDService = Cu.import("resource://gre/modules/identity/Identity.jsm").IdentityService;
> +const IdentityProvider = Cu.import("resource://gre/modules/identity/IdentityProvider.jsm").IdentityProvider;

*headscratch* Oh, Cu.import returns the global.

I think of these can just be plain Cu.import(). You'll get the module's exported functions and all is fine. You shouldn't need Cu.import().foo unless |foo| isn't being exported.

@@ +182,5 @@
> +  } else {
> +    Services.prefs.clearUserPref("toolkit.identity.debug");
> +    Services.prefs.clearUserPref("dom.identity.enabled");
> +    SimpleTest.finish();
> +  }

I'd suggest tossing in some try/catch here, to ensure that if |test()| throws an exception your cleanup code will run.

::: toolkit/identity/tests/mochitest/test_authentication.html
@@ +42,5 @@
> +    is(aSubject.wrappedJSObject.provId, _provId);
> +
> +    run_next_test();
> +  });
> +info("foo");

foo?

@@ +57,5 @@
> +	IDService.IDP._doAuthentication(_provId, TEST_IDPPARAMS);
> +      }
> +    });
> +} catch (ex) {
> +  info(ex);

catch -> only info()? Seems like this should be a hard-fail. Or not bother catching. Presumably this is all just some leftover debugging stuff...

@@ +59,5 @@
> +    });
> +} catch (ex) {
> +  info(ex);
> +}
> +info("bar");

bar?

@@ +119,5 @@
> +      //IDService.IDP.completeAuthentication(_authId); // TODO: switch to dom call
> +    },
> +
> +    doError: function(err) {
> +      ok(false, "OW! My doError callback hurts!: " + err);

My doctor said I'm not supposed to get exceptions in it! </simpsons>

@@ +152,5 @@
> +}
> +
> +//TESTS.push(test_begin_authentication_flow);
> +//TESTS.push(test_complete_authentication_flow);
> +todo(false, "Tests are disabled");

How about a bug-on-file to fix and enable?

::: toolkit/identity/tests/mochitest/test_provisioning.html
@@ +26,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +const DOMIdentity = Cu.import("resource://gre/modules/DOMIdentity.jsm")
> +                      .DOMIdentity;

Same comment as for head_identity.js, plain Cu.import() should be sufficient here.

::: toolkit/identity/tests/mochitest/test_relying_party.html
@@ +26,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +const DOMIdentity = Cu.import("resource://gre/modules/DOMIdentity.jsm")
> +                      .DOMIdentity;

Same as previous.

@@ +152,5 @@
> +function test_request() {
> +  // set up a watch, to be consistent
> +  let mockedDoc = mock_watch(null, function(action, params) {
> +    // this isn't going to be called for now
> +    // XXX but it is called - is that bad?

A fine question!

Comment 33

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #32)
> Comment on attachment 638880 [details] [diff] [review]
> v.1 mochitests
> ::: toolkit/identity/tests/mochitest/head_identity.js
> @@ +33,5 @@
> > +const IdentityStore = Cu.import("resource://gre/modules/identity/IdentityStore.jsm").IdentityStore;
> > +const RelyingParty = Cu.import("resource://gre/modules/identity/RelyingParty.jsm").RelyingParty;
> > +const XPCOMUtils = Cu.import("resource://gre/modules/XPCOMUtils.jsm").XPCOMUtils;
> > +const IDService = Cu.import("resource://gre/modules/identity/Identity.jsm").IdentityService;
> > +const IdentityProvider = Cu.import("resource://gre/modules/identity/IdentityProvider.jsm").IdentityProvider;
> 
> *headscratch* Oh, Cu.import returns the global.
> 
> I think of these can just be plain Cu.import(). You'll get the module's
> exported functions and all is fine. You shouldn't need Cu.import().foo
> unless |foo| isn't being exported.

That's what I thought except it doesn't work, hence the mess.  I think it's because the JS file is unprivileged.  It also doesn't work to explicitly provide |this| or |window| as the 2nd argument.

> ::: toolkit/identity/tests/mochitest/test_authentication.html
> @@ +42,5 @@
> > +    is(aSubject.wrappedJSObject.provId, _provId);
> > +
> > +    run_next_test();
> > +  });
> > +info("foo");
> 
> foo?
> 
> @@ +57,5 @@
> > +	IDService.IDP._doAuthentication(_provId, TEST_IDPPARAMS);
> > +      }
> > +    });
> > +} catch (ex) {
> > +  info(ex);
> 
> catch -> only info()? Seems like this should be a hard-fail. Or not bother
> catching. Presumably this is all just some leftover debugging stuff...

Yep, this was leftover stuff from porting the test.  I could have swore I finished but I guess not.  I've fixed the tests now and removed this garbage.

https://hg.mozilla.org/projects/pine/rev/b32f5a90a155

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #33)
> That's what I thought except it doesn't work, hence the mess.  I think it's
> because the JS file is unprivileged.  It also doesn't work to explicitly
> provide |this| or |window| as the 2nd argument.

"doesn't work" how? Are you just referring to the "leaked global variable" warning that the test harness complains about, or does the exported symbol actually not appear if you do a plain import()? The latter would merit further investigation...

Comment 35

[Matthew N. [:MattN]]

The exported symbol doesn't appear.  This is what other tests were doing as well: https://mxr.mozilla.org/mozilla-central/search?string=%3D+Cu.import&find=test

Comment 36

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #35)
> The exported symbol doesn't appear.  This is what other tests were doing as
> well:
> https://mxr.mozilla.org/mozilla-central/search?string=%3D+Cu.import&find=test

I filed bug 772288 on this.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #35)
> The exported symbol doesn't appear.  This is what other tests were doing as
> well:
> https://mxr.mozilla.org/mozilla-central/search?string=%3D+Cu.import&find=test

Weird. Those tests look like xpcshell, so I'm not sure that that's done for the same reason (it may just be to avoid scope pollution or somesuch).

Comment 38

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #37)
> (In reply to Matthew N. [:MattN] from comment #35)
> > The exported symbol doesn't appear.  This is what other tests were doing as
> > well:
> > https://mxr.mozilla.org/mozilla-central/search?string=%3D+Cu.import&find=test
> 
> Weird. Those tests look like xpcshell, so I'm not sure that that's done for
> the same reason (it may just be to avoid scope pollution or somesuch).

That's true although I could have sworn I saw a mochitest doing this.  There is now a patch to have Cu.import(url, window) work.

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1b925bd4ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e5b438c911

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=37267a3dc396

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Sorry, but test_authentication.html was nearly perma-orange on OSX. I had to back this out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a22d693c1ea1

https://tbpl.mozilla.org/php/getParsedLog.php?id=13403276&tree=Mozilla-Inbound

Comment 40

[Matthew N. [:MattN]]

Take 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a335f5f3e103
https://hg.mozilla.org/integration/mozilla-inbound/rev/576bbe43bb64

The problem was some code running after the test ended.  I removed some unnecessary observers from the test and made sure to cleanup state at the end.

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a335f5f3e103
https://hg.mozilla.org/mozilla-central/rev/576bbe43bb64

Comment 42

[Jason Smith [:jsmith]]

From an end-user perspective, is it possible to verify this fix (I think it is, but I want to double-check)? What is needed to be tested?

Comment 43

[Matthew N. [:MattN]]

(In reply to Jason Smith [:jsmith] from comment #42)
> From an end-user perspective, is it possible to verify this fix (I think it
> is, but I want to double-check)? What is needed to be tested?

The only thing you can verify is that when you set the pref dom.identity.enabled to true, websites using BrowserID will no longer work (they won't use the shim and will do nothing on login since the UI has not landed).

SignInToWebsite UI:
* Bug 764213: Implement Desktop UI for website sign-in with Persona
* Bug 767276: UX for Signin-to-Website on Fennec

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 638880 [details] [diff] [review]
v.1 mochitests

>+++ b/toolkit/identity/tests/mochitest/Makefile.in
>+_TEST_FILES = \
>+		head_identity.js \
>+		test_authentication.html \
>+		test_provisioning.html \
>+		test_relying_party.html \
>+		$(NULL)

Reopening, because this bug's mochitests are almost certainly not being run, since they set _TEST_FILES instead of MOCHITEST_FILES in the makefile. (There was a global renaming last week to change everything to use MOCHITEST_FILES)

This needs a followup patch along the lines of the one in bug 772981 in order for the mochitests to be run.  (possibly wanting review from froydnj or glandium, since they were involved with the renaming and can sanity-check that the updated makefile looks correct)

Comment 45

[Daniel Holbert [:dholbert]]

The Mochitest makefiles affected are dom/identity/tests/Makefile.in (from 7d1b925bd4ee ) and toolkit/identity/tests/mochitest/Makefile.in (from 52e5b438c911 )

It also might be as bad as I'd made it out to be in comment 44 -- given the fact that a new test was orange in comment 39, I take it the tests are running after all, but there may be other mochitest-foo that's not getting set up correctly. (I'm not sure exactly what the s/_TEST_FILES/MOCHITEST_FILES/ etc. changes do; I'd assumed straggling tests wouldn't run, but I guess it's not quite that bad.)

Comment 46

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #45)
> It also might be as bad as I'd made it out

er, meant to say "might not be"

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Sounds like something best tracked in a followup bug - these patches are landed, so this should stay FIXED.

Comment 48

[Daniel Holbert [:dholbert]]

Sounds good to me. (I'd initially reopened because I suspected that this bug's tests weren't being run at all, but per comment 45-46, I'm pretty sure that I was wrong about that.)