929243 - Allow navigator.id to work natively again outside of B2G via Identity.jsm

Status: RESOLVED WONTFIX

(Core Graveyard :: Identity, defect)

Description

[Austin King [:ozten]]

Port Identity at revision 2d96ee8d9dd4 back onto the tip of the default branch.

Comment 1

[Austin King [:ozten]]

Attachment: 929243-port-2d96ee8d9dd4

Work in progress: Current patch has errors when loading a page that uses navigator.id such as

    JavaScript error: http://192.168.186.138:10002/include.js, line 1040:
    NS_ERROR_FACTORY_NOT_REGISTERED: 'Factory not registered' when calling method: [xpcIJSGetFactory::get]
    JavaScript error: http://192.168.186.138:10001/, line 180: 
    NS_ERROR_FACTORY_NOT_REGISTERED: 'Factory not registered' when calling method: [xpcIJSGetFactory::get]

Comment 2

[Austin King [:ozten]]

Looks like my component IDs didn't match between dom/identity/Identity.manifest and nsIDService.js.

After fixing that (using 210853d9-2c97-4669-9761-b1ab9cbf57ef), the Factory errors are fixed.

However, I get the JS shim and there is no doorhanger.

Comment 3

[Austin King [:ozten]]

Attachment: 929243-port-2d96ee8d9dd4_Component.ID

I missed a second component id. With them both fixed, I don't see  the JS Shim.

Still no Doorhanger, but I'm getting further than before.

Updating attachment.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 820782 [details] [diff] [review]
929243-port-2d96ee8d9dd4_Component.ID

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

::: browser/base/content/test/general/browser_identity_UI.js
@@ +15,5 @@
>  var tests = [
>    {
>      name: "normal domain",
>      location: "http://test1.example.org/",
> +    host: "test1.example.org",

I don't think you want to change this file at all as it's not related to BrowserID stuff.

::: dom/identity/DOMIdentity.jsm
@@ +6,5 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  // This is the parent process corresponding to nsDOMIdentity.
> +let EXPORTED_SYMBOLS = ["DOMIdentity"];

These should stay as "this.*" otherwise they will break B2G. This is the new way of define globals in JSMs so they can share a JS compartment (on B2G). See bug 798491 if you're interested.

::: dom/identity/Identity.manifest
@@ +5,4 @@
>  
>  # nsIDService.js (initialization on startup)
> +component {baa581e5-8e72-406c-8c9f-dcd4b23a6f82} nsIDService.js
> +contract @mozilla.org/dom/identity/service;1 {baa581e5-8e72-406c-8c9f-dcd4b23a6f82}

This stuff doesn't need to change (or nsIDService.js)

::: dom/identity/tests/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# TODO port Makefile.in contents below

Bringing back these tests that were removed is tracked in bug 805602. I've made that block the desktop meta-bug.

::: toolkit/identity/tests/chrome/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  MOCHITEST_CHROME_MANIFESTS += ['chrome.ini']
>  
> +# TODO MOCHITEST_CHROME_FILES = \

These were moved to toolkit/identity/tests/chrome/chrome.ini so I don't think there is anything to do here.

Comment 5

[Austin King [:ozten]]

Attachment: 929243-fix-tip-bustedness

Sorry, I was trying to do too much in the last patch.

This patch is MattN's work with a few tweaks. Add an error param to callback and remove commented out code. Remove dump.

Comment 6

[Austin King [:ozten]]

Attachment: 929243-fix-tip-bustedness_updated

Merged tip which removes part of this patch. Thanks Matt!

Comment 7

[Austin King [:ozten]]

https://tbpl.mozilla.org/?tree=Try&rev=4350487d495c

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 821077 [details] [diff] [review]
929243-fix-tip-bustedness_updated

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

Sorry for the delay. I kept putting it off because there are too many thing going on here and the reasoning isn't clear. Separate patches per issue would help greatly and I promise I'll review faster then.

::: browser/modules/SignInToWebsite.jsm
@@ -25,5 @@
>  
>  this.SignInToWebsiteUX = {
>  
>    init: function SignInToWebsiteUX_init() {
> -

Nit: revert this whitespace change

::: browser/modules/test/Makefile.in
@@ +4,5 @@
>  
>  ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>  MOCHITEST_BROWSER_FILES += \
>    browser_taskbar_preview.js \
> +  browser_SignInToWebsite.js \

Revert this as I already moved this to browser.ini in http://hg.mozilla.org/mozilla-central/diff/4e075a69e78b/browser/modules/test/browser.ini

::: dom/identity/nsDOMIdentity.js
@@ +358,5 @@
> +
> +      this._identityInternal._mm.sendAsyncMessage("Identity:IDP:CompleteAuthentication",
> +                                                  this.DOMIdentityMessage());
> +    } catch (ex) {
> +      dump(ex + "\n");

Can you remove this try…catch?

@@ +622,5 @@
>      // innerwindow id to construct the unique id.
>      this._id = uuidgen.generateUUID().toString();
>      this._innerWindowID = util.currentInnerWindowID;
> +#else
> +    this._id = util.outerWindowID;

We'll have to get a DOM peer to review this but I can take a look first after you address the other comments.

@@ +651,5 @@
>  
>      // Setup observers so we can remove message listeners.
>      Services.obs.addObserver(this, "inner-window-destroyed", false);
> +} catch (ex) {
> +  dump(ex + "\n");

I put this try…catch in for debugging. It will need to go before landing.

::: toolkit/identity/IdentityProvider.jsm
@@ -85,5 @@
>    shutdown: function RP_shutdown() {
>      this.reset();
>  
> -    if (this._sandboxConfigured) {
> -      // Tear down message manager listening on the hidden window

Where will this get cleaned up if it's not done here? I think we'll end up leaking if we do this. Can you deal with whatever issue this is fixing in a separate patch (even in the same bug) and describe the problem?

Comment 9

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #8)
> Comment on attachment 821077 [details] [diff] [review]
> ::: toolkit/identity/IdentityProvider.jsm
> @@ -85,5 @@
> >    shutdown: function RP_shutdown() {
> >      this.reset();
> >  
> > -    if (this._sandboxConfigured) {
> > -      // Tear down message manager listening on the hidden window
> 
> Where will this get cleaned up if it's not done here? I think we'll end up
> leaking if we do this. Can you deal with whatever issue this is fixing in a
> separate patch (even in the same bug) and describe the problem?

Sorry, ignore this, I understand what's happening now. The messages are already getting listened to from the hidden window so we no longer need to do that extra configuration like we did before.

I confirmed I was able to login to 123done.org with an eyedee.me account with the patch :)

Once we have the cleaned up version we can get final review and land it.

Comment 10

[Matthew N. [:MattN]]

Attachment: v.1 Fix codepath to Identity.jsm and use outerWindowID again where it is unique enough

jst, are you fine with using outerWindowID again outside of B2G to identify the window calling the navigator.id APIs? 

This goes back to how this code was when it initially landed and makes it much easier for front-ends to efficiently identity the <browser> associated with a navigator.id call using getOuterWindowWithId e.g. https://mxr.mozilla.org/mozilla-central/source/browser/modules/SignInToWebsite.jsm?rev=4e075a69e78b#124

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 8345083 [details] [diff] [review]
v.1 Fix codepath to Identity.jsm and use outerWindowID again where it is unique enough

Sorry for the very long delay here, this one fell through the cracks on my end :(

I think the answer to your question is no, whether or not the window id is unique enough here is not a question of whether or not this code runs on b2g or not, it's a question of whether or not we're using multiple processes or not and now with e10s for desktop inching closer and closer to reality, we'll need to keep this code working correctly there as well. Given that, I don't think it's worth supporting two different notions of unique id's here :(

Comment 12

[Matthew N. [:MattN]]

With Persona shutting down I don't think this is going to happen.