Closed Bug 794680 Opened 12 years ago Closed 12 years ago

Create a persona dialog for use on B2G

Categories

(Core Graveyard :: Identity, enhancement)

enhancement
Not set
normal

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: jedp, Assigned: jedp)

References

Details

(Whiteboard: [LOE:M] [qa-][feature-complete])

Attachments

(2 files, 11 obsolete files)

355 bytes, text/html
vingtetun
: review+
Details
100.20 KB, patch
Details | Diff | Splinter Review
Depends on:
- serving persona include.js #793909
- implementation of the Trusty UI #768943
Important to note - read https://bugzilla.mozilla.org/show_bug.cgi?id=768943#c116. For persona, the v1 blocker is to have a trusted UI dialog, but doesn't necessarily have to be a "global" abstract trusted UI component, as that does not block v1. The nav.mozPay implementation currently heavily hard codes against a trusted UI with mozPay, so we may need to implement a separate trusted UI component for persona. If that's not a good solution for v1 and we definitely need to refactor to global trusted UI component in bug 794999, then please let everyone know, nom the bug to block, and explain why.
(In reply to Jason Smith [:jsmith] from comment #1)
> Important to note - read
> https://bugzilla.mozilla.org/show_bug.cgi?id=768943#c116. For persona, the
> v1 blocker is to have a trusted UI dialog, but doesn't necessarily have to
> be a "global" abstract trusted UI component, as that does not block v1. The
> nav.mozPay implementation currently heavily hard codes against a trusted UI
> with mozPay, so we may need to implement a separate trusted UI component for
> persona. If that's not a good solution for v1 and we definitely need to
> refactor to global trusted UI component in bug 794999, then please let
> everyone know, nom the bug to block, and explain why.

Jonas - Does this align with your understanding? Or am I out in left field?
(In reply to Jason Smith [:jsmith] from comment #1)
> The nav.mozPay implementation currently heavily hard codes against 
> a trusted UI with mozPay, so we may need to implement a separate 
> trusted UI component for persona.

That's fine by me.

I've opened #795023 for a Trusted UI for nav.id
Depends on: 795023
No longer depends on: 768943
Yes, let's please not involve bug 794999. Instead bug 795023 seems like a more doable solution.
Honestly, i'm not even sure that we need to have bug 795023. We can probably just do the work in this bug. I think that would be less confusing.
(In reply to Jonas Sicking (:sicking) from comment #5)
> Honestly, i'm not even sure that we need to have bug 795023. We can probably
> just do the work in this bug. I think that would be less confusing.

Works for me. https://github.com/mozilla-b2g/gaia/issues/5323 tracks anything needed on the Gaia side.
Assignee: nobody → ferjmoreno
Whiteboard: [LOE:M]
Assignee: ferjmoreno → benadida
Status: NEW → ASSIGNED
Comment on attachment 667613 [details] [diff] [review]
WIP - 2012-10-03 - gecko patches for connecting to b2g identity ui

Don't forget to add the following to b2g/installer/package-manifest.in so that the files are packaged for distribution:
@BINPATH@/components/identity.xpt
@BINPATH@/components/nsDOMIdentity.js
@BINPATH@/components/nsIDService.js
@BINPATH@/components/Identity.manifest

Like we did for desktop:
https://hg.mozilla.org/mozilla-central/diff/a335f5f3e103/browser/installer/package-manifest.in
https://hg.mozilla.org/mozilla-central/diff/88070ff09ccd/browser/installer/package-manifest.in
Blocks: 795023
No longer depends on: 795023
Attachment #667613 - Attachment is obsolete: true
Attachment #670075 - Flags: review?(benadida)
(In reply to Jed Parsons [:jparsons] from comment #10)
> Created attachment 670075 [details] [diff] [review]
> gecko patch for connecting to b2g identity ui

I am pushing to try now ...
Attachment #667617 - Attachment is obsolete: true
Attachment #670075 - Attachment is obsolete: true
Attachment #670075 - Flags: review?(benadida)
Attachment #670250 - Flags: review?(benadida)
patch v2 adds logout - that was missing from the former patch
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2

looking good, thanks Jed.
Attachment #670250 - Flags: review?(benadida) → review+
note that this patch also addresses bug #798442
No longer depends on: 795854
4 day old patch with a review+... Does this need a checkin-needed flag?
Oh brilliant.  Oops.  Yes :)
Keywords: checkin-needed
(In reply to Dietrich Ayala (:dietrich) from comment #16)
> 4 day old patch with a review+... Does this need a checkin-needed flag?

That's cause I'm slow and I should have just pushed it. Will do that today. Thanks!
(In reply to Ben Adida [:benadida] from comment #18)
> (In reply to Dietrich Ayala (:dietrich) from comment #16)
> > 4 day old patch with a review+... Does this need a checkin-needed flag?
> 
> That's cause I'm slow and I should have just pushed it. Will do that today.
> Thanks!

Wait a second. Doesn't this need gaia approval? At-risk features need to get approval to land.
(In reply to Jason Smith [:jsmith] from comment #19)
> (In reply to Ben Adida [:benadida] from comment #18)
> > (In reply to Dietrich Ayala (:dietrich) from comment #16)
> > > 4 day old patch with a review+... Does this need a checkin-needed flag?
> > 
> > That's cause I'm slow and I should have just pushed it. Will do that today.
> > Thanks!
> 
> Wait a second. Doesn't this need gaia approval? At-risk features need to get
> approval to land.

When I mean to say that btw, I mean the pull request on the Gaia side. Not the code placed here.
Blocking+ does not need driver approval. Only non-blocking work needs approval at this point.
(In reply to Dietrich Ayala (:dietrich) from comment #21)
> Blocking+ does not need driver approval. Only non-blocking work needs
> approval at this point.

This is at-risk feature however. At-risk features do need driver approval.
(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Dietrich Ayala (:dietrich) from comment #21)
> > Blocking+ does not need driver approval. Only non-blocking work needs
> > approval at this point.
> 
> This is at-risk feature however. At-risk features do need driver approval.

Got the clarification from Dietrich - this one actually can land.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8830d30f62
Assignee: benadida → jparsons
Flags: in-testsuite+
Keywords: checkin-needed
(In reply to Jed Parsons [:jparsons] from comment #11)
> (In reply to Jed Parsons [:jparsons] from comment #10)
> > Created attachment 670075 [details] [diff] [review]
> > gecko patch for connecting to b2g identity ui
> 
> I am pushing to try now ...

jparsons, do you have your Try push handy? (This bug's push triggered some leaks on Mac OS, and I'm trying to narrow down which csets are definitely-innocent vs. maybe-guilty.)
Jason: since when is this at risk? We're on schedule with the implementation plan, as I've been reporting twice a week.
(In reply to Daniel Holbert [:dholbert] from comment #25)
> (In reply to Jed Parsons [:jparsons] from comment #11)
> > (In reply to Jed Parsons [:jparsons] from comment #10)
> > > Created attachment 670075 [details] [diff] [review]
> > > gecko patch for connecting to b2g identity ui
> > 
> > I am pushing to try now ...
> 
> jparsons, do you have your Try push handy? (This bug's push triggered some
> leaks on Mac OS, and I'm trying to narrow down which csets are
> definitely-innocent vs. maybe-guilty.)

Daniel, yes: https://tbpl.mozilla.org/?tree=Try&rev=9b6e173298dd

Please let me know if you see any leaks I need to fill.
This was indeed the culprit (at least, stuff went green when philor backed it out).  Backout cset is:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8f277d19df

This hit timeouts / test-failures in identity mochitests on Mochitest-2 & Mochitest-5, on all platforms. (Those are visible in the Try push, FWIW...)

The leaks aren't visible in the Try push (maybe they're from something that changed in the patch since then?), but they're pretty easy to hit now.  This leaked on basically all debug unit tests on all platforms.

Here are some sample logs with leaks:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182715&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182228&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182177&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182138&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182295&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182560&tree=Mozilla-Inbound

...and some sample logs with the test failures/timeouts:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182493&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182365&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=16182194&tree=Mozilla-Inbound
Flags: in-testsuite+
Version: unspecified → Trunk
The leaks aren't visible on that try push because we can only detect leaks on debug builds, and that was an opt-only try push.
Yup, just noticed that -- the try run didn't have any debug builds, hence no leaks reported.

FWIW, here's the m-i push where this landed.  Most of the orange there is from the leaks:   https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=31642ee5a1c9

(don't be alarmed by the fact that some test-runs are missing there -- those were just coalesced into a later cycle, I think)
Also: while you're fixing this, could you add a newline at the end of the added file "test_minimalidentity.js"? it's missing a final-newline, as shown by the last quoted line below (from the attached patch):
>+++ b/toolkit/identity/tests/unit/test_minimalidentity.js
>@@ -0,0 +1,95 @@
>+"use strict";
[...]
>+function run_test() {
>+  run_next_test();
>+}
>\ No newline at end of file
Daniel and Phil, thanks very much for the details.  I'll get on this tomorrow and stop the leaks.
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2

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

::: dom/identity/DOMIdentity.jsm
@@ +216,3 @@
>      for (let message of this.messages) {
> +      // func.call(aWindow.messageManager, message, this);
> +	    ppmm.addMessageListener(message, this);

This new line should be like the previous line so that it removes the listener when !aRegister.  This could cause a leak.

::: toolkit/identity/MinimalIdentity.jsm
@@ +35,5 @@
> +}
> +
> +function IDService() {
> +  Services.obs.addObserver(this, "quit-application-granted", false);
> +  Services.obs.addObserver(this, "identity-auth-complete", false);

Where is the identity-auth-complete observer getting used and removed?
Test failures were in the following suites (run from the obj.-dir):
    TEST_PATH=dom/identity/ make -C . mochitest-plain
    TEST_PATH=toolkit/identity/tests/mochitest/ make -C . mochitest-plain
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854

Zaach, could you squash all the commits in one, please?
Attachment #673209 - Flags: review?(21)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> Comment on attachment 673209 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
> 
> Zaach, could you squash all the commits in one, please?

Fernando: we kept it as two commits because the first one has been waiting for review for a few days, and we didn't want to mess up the review process. Do you really want this squashed?
(In reply to Ben Adida [:benadida] from comment #37)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> > Comment on attachment 673209 [details]
> > Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
> > 
> > Zaach, could you squash all the commits in one, please?
> 
> Fernando: we kept it as two commits because the first one has been waiting
> for review for a few days, and we didn't want to mess up the review process.
> Do you really want this squashed?

In that case, if I am not wrong, we have two options here:

1. Squash all the commits in one referring only to this bug and close Bug 795023.

or

2. Remove the last two commits from the pull request, cause they refer to Bug 795023 and send a new pull request with the commits squashed in one attaching it to Bug 795023.

I am afraid that since feature freeze there's need to be one PR per bugzilla bug :\, so it can be easily reverted in case of failure.

Dietrich can you confirm that, please?
Flags: needinfo?(dietrich)
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2

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

r- for the files that will land in b2g/. You may also want to remove some commented codes in dom/.

::: b2g/components/SignInToWebsite.jsm
@@ +9,5 @@
> +"use strict";
> +
> +const EXPORTED_SYMBOLS = ["SignInToWebsiteController"];
> +
> +const Cc = Components.classes;

Cc is unused.

@@ +24,5 @@
> +                                  "resource://gre/modules/identity/LogUtils.jsm");
> +
> +// JS shim that contains the callback functions that
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)

Can you extend on what is 'idP' ?

@@ +25,5 @@
> +
> +// JS shim that contains the callback functions that
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)
> +const kIdentityShimFile = "chrome://browser/content/identity.js";

Is this file missing from the PR?

@@ +27,5 @@
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)
> +const kIdentityShimFile = "chrome://browser/content/identity.js";
> +
> +// Type of MozChromEvents to handle payment dialogs.

nit: MozChrom -> MozChrome
nit: I believe you don't want to handle payment dialogs here :)

@@ +82,5 @@
> +    return uuidgen.generateUUID().toString();
> +  },
> +
> +  doWatch: function SignInToWebsiteController_doWatch(aOptions) {
> +    // for now, just say we're ready

Why? Can we remove this code for now, just until is is ready and it is clear why this stuff happens?

@@ +97,5 @@
> +  },
> +
> +  /*
> +   *
> +   */

No comment so I believe you can just remove those.

@@ +109,5 @@
> +  _openDialogAndSendMessage: function SignInToWebsiteController_openDialogAndSendMessage(aRpId, aMessage, aOptions) {
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    let content = browser.getContentWindow();
> +    if (!content) {
> +      // aErrorCb.onresult("NO_CONTENT_WINDOW");

No dead code please.

@@ +127,5 @@
> +      log("id after is ", id);
> +      let msg = evt.detail;
> +      if (msg.id != id) {
> +        log("mozContentEvent. evt.detail.id != ", id, msg);
> +        content.removeEventListener("mozContentEvent", getAssertion);

So if there is 1 mozContentEvent for something else you're not going to listen the assertion anymore?

@@ +157,5 @@
> +      mm.sendAsyncMessage(aMessage, aOptions);
> +
> +      log("done load frame script");
> +
> +      content.removeEventListener("mozContentEvent", getAssertion);

I would move this code right after the msg.id != id check. So it is obvious that it is removed as soon as we have a matching id.

::: b2g/components/test/unit/head_identity.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const Cc = Components.classes;

Cc is unused.

@@ +3,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;

Cr is unused.

@@ +48,5 @@
> +}
> +
> +// create a mock "doc" object, which the Identity Service
> +// uses as a pointer back into the doc object
> +function mock_doc(aIdentity, aOrigin, aDoFunc) {

Why is it not camelCase?

@@ +67,5 @@
> +
> +// mimicking callback funtionality for ease of testing
> +// this observer auto-removes itself after the observe function
> +// is called, so this is meant to observe only ONE event.
> +function makeObserver(aObserveTopic, aObserveFunc) {

Why is it camelCase while the other parts are not?

@@ +76,5 @@
> +
> +    observe: function (aSubject, aTopic, aData) {
> +      if (aTopic == aObserveTopic) {
> +        aObserveFunc(aSubject, aTopic, aData);
> +        Services.obs.removeObserver(observer, aObserveTopic);

Personally I will remove the observer before the aObserverFunc, you never know if this one will fail or not.

@@ +90,5 @@
> +function setup_test_identity(identity, cert, cb) {
> +  // set up the store so that we're supposed to be logged in
> +  let store = get_idstore();
> +
> +  function keyGenerated(err, kpo) {

What is kpo ?

@@ +101,5 @@
> +
> +// takes a list of functions and returns a function that
> +// when called the first time, calls the first func,
> +// then the next time the second, etc.
> +function call_sequentially() {

This function seems unused. Please remove it.

::: b2g/components/test/unit/test_signintowebsite.js
@@ +1,1 @@
> +"use strict";

It miss the file header.

@@ +26,5 @@
> +    do_test_finished();
> +    run_next_test();
> +  });
> +
> +  mockedDoc.doCoffee();

I have no fucking clue what this stuff does. Is there a way to give it a real name?

::: dom/identity/DOMIdentity.jsm
@@ +16,3 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "IdentityService",
>                                    "resource://gre/modules/identity/Identity.jsm");
> +*/

Do you really want to leave dead code?

@@ +137,2 @@
>                             .QueryInterface(Ci.nsIFrameLoaderOwner)
> +                           .frameLoader.messageManager;*/

Same comment as above.

::: dom/identity/nsDOMIdentity.js
@@ +491,4 @@
>                        .getInterface(Ci.nsIWebNavigation)
>                        .QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIContentFrameMessageManager);
> +    */

Same comment as above.

::: toolkit/identity/MinimalIdentity.jsm
@@ +78,5 @@
> +    }
> +    return null;
> +  },
> +
> +  // RP stuff

That's not really a useful comment for someone like me that jump into this code :)

::: toolkit/identity/tests/Makefile.in
@@ +12,5 @@
>  
>  MODULE = test_identity
>  XPCSHELL_TESTS = unit
>  
> +DIRS = chrome mochitest 

Remove the trailing whitespace. Only change in this file.
Attachment #670250 - Flags: review-
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854

r+ for the Gaia parts with the 2 nits addressed. 

I'm a bit uncomfortable with the persona frame beeing in-process since it can slow the entire phone if the site is slow to load (same as the payment frame). I would like cjones to have a word on that.
Attachment #673209 - Flags: review?(jones.chris.g)
Attachment #673209 - Flags: review?(21)
Attachment #673209 - Flags: review+
Why is this in-process to begin with?
(In reply to Vivien Nicolas (:vingtetun) from comment #40)
> r- for the files that will land in b2g/. You may also want to remove some
> commented codes in dom/.

This part of the code was already r+ (by me) and is awaiting some memory leak fixes (on Mac OS). I'll ask Jed to look at your comments, of course, for the next rev, but let's not revisit with a different set of criteria.
(In reply to Vivien Nicolas (:vingtetun) from comment #41)
> I'm a bit uncomfortable with the persona frame beeing in-process since it
> can slow the entire phone if the site is slow to load (same as the payment
> frame).

As you point out, we followed exactly the example from nav.pay. If there is a different path, then please let us know, but then nav.pay shouldn't have landed.

Quick reminder: we're working towards 10/26 feature freeze. There is time to fix bugs afterwards, but can we work to land the features ASAP, assuming no large problems?
(In reply to Ben Adida [:benadida] from comment #43)
> (In reply to Vivien Nicolas (:vingtetun) from comment #40)
> > r- for the files that will land in b2g/. You may also want to remove some
> > commented codes in dom/.
> 
> This part of the code was already r+ (by me) and is awaiting some memory
> leak fixes (on Mac OS). I'll ask Jed to look at your comments, of course,
> for the next rev, but let's not revisit with a different set of criteria.

That's why I said r- for the b2g/ files. The others are out of my scope but are in yours.
(In reply to Ben Adida [:benadida] from comment #44)
> (In reply to Vivien Nicolas (:vingtetun) from comment #41)
> > I'm a bit uncomfortable with the persona frame beeing in-process since it
> > can slow the entire phone if the site is slow to load (same as the payment
> > frame).
> 
> As you point out, we followed exactly the example from nav.pay. If there is
> a different path, then please let us know, but then nav.pay shouldn't have
> landed.
> 

Maybe. But I want to raised the issue before spreading it all over the place.


> Quick reminder: we're working towards 10/26 feature freeze. There is time to
> fix bugs afterwards, but can we work to land the features ASAP, assuming no
> large problems?

We are on the same line here.

So in order to go fast please let's fix the comments I made for *both* patches - there are some serious questions I have like the one with the missing identity.js file. And I would like Chris to have a chance to know about the non-e10s state of the art for such features.
Code in the dom module should be reviewed by a dom peer. There is commented out code in there. That's not going to pass review. jst can help cleaning up the code and find a reviewer.

nav.pay is out of process now (bug 804080). We can follow the same approach here and file a follow-up bug. Worst case we can ship without it, but we likely do want it if we can get it done in time. As Vivien points out, in process is a big risk for memory leaks and responsiveness (we can't reclaim leaked memory in the chrome process).
(In reply to Andreas Gal :gal from comment #47)
> Code in the dom module should be reviewed by a dom peer. There is commented
> out code in there. That's not going to pass review. jst can help cleaning up
> the code and find a reviewer.

I don't believe this is the case because:

(a) this code is entirely for nav.id, so it also fits inside the identity module

(b) I've agreed with DOM peers that anything that requires significant architectural changes would be reviewed by them, too. This does not qualify, as it is merely a reversal to the use of cpmm/ppmm for messaging, which was how the DOM peers had recommended implementation the first time around.

I'm pushing back on this because I'm trying to meet the deadline (this Friday). If we add reviewers now, it's pretty clear we won't land this in time, given that there is more code in the pipeline that's stuck on these reviews.

> nav.pay is out of process now (bug 804080). We can follow the same approach
> here and file a follow-up bug.

Great, thanks, I've done that in bug 804143.
(In reply to Ben Adida [:benadida] from comment #48)
> (In reply to Andreas Gal :gal from comment #47)
> > Code in the dom module should be reviewed by a dom peer. There is commented
> > out code in there. That's not going to pass review. jst can help cleaning up
> > the code and find a reviewer.
> 
> I don't believe this is the case because:
> 
> (a) this code is entirely for nav.id, so it also fits inside the identity
> module

Please just remove the dead code.

It would take far less time to do what Vivien requested, which matches longstanding project-wide standards on not commenting out code, than it took to argue about it on shaky "it also fits" grounds.

If the file in question is really part of two modules (not possible by construction according to Mozilla's definition of "module"), then both modules' reviewers would need to be happy.

> (b) I've agreed with DOM peers that anything that requires significant
> architectural changes would be reviewed by them, too. This does not qualify,
> as it is merely a reversal to the use of cpmm/ppmm for messaging, which was
> how the DOM peers had recommended implementation the first time around.

Things change, but project-wide review standards against commented-out code are old as the hills.

> I'm pushing back on this because I'm trying to meet the deadline (this
> Friday).

There is no way a Friday deadline requires commenting out instead of deleting code. Hg will remember, no need to keep the old code in a comment.

/be
> Hg will remember, no need to keep the old code in a comment.

Git remembers too ;-).

/be
Thanks, :vingtetun for your feedback.  I'm preparing another patch now that should address your questions and comments.
Attachment #670250 - Attachment is obsolete: true
Attachment #673921 - Flags: review?(benadida)
I have submitted v3 of this patch

(In reply to Vivien Nicolas (:vingtetun) from comment #40)
> Comment on attachment 670250 [details] [diff] [review]
> connect gecko to b2g identity ui, v2
> 
> Review of attachment 670250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the files that will land in b2g/. You may also want to remove some
> commented codes in dom/.

Yes.  done.

> ::: b2g/components/SignInToWebsite.jsm
> @@ +9,5 @@
> > +"use strict";
> > +
> > +const EXPORTED_SYMBOLS = ["SignInToWebsiteController"];
> > +
> > +const Cc = Components.classes;
> 
> Cc is unused.

removed

> @@ +24,5 @@
> > +                                  "resource://gre/modules/identity/LogUtils.jsm");
> > +
> > +// JS shim that contains the callback functions that
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
> 
> Can you extend on what is 'idP' ?

I've removed the comment.  It's not pertinent for the patch.  

> @@ +25,5 @@
> > +
> > +// JS shim that contains the callback functions that
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
> > +const kIdentityShimFile = "chrome://browser/content/identity.js";
> 
> Is this file missing from the PR?

yes, it sure was!

> @@ +27,5 @@
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
> > +const kIdentityShimFile = "chrome://browser/content/identity.js";
> > +
> > +// Type of MozChromEvents to handle payment dialogs.
> 
> nit: MozChrom -> MozChrome
> nit: I believe you don't want to handle payment dialogs here :)

true :)  fixed.

> @@ +82,5 @@
> > +    return uuidgen.generateUUID().toString();
> > +  },
> > +
> > +  doWatch: function SignInToWebsiteController_doWatch(aOptions) {
> > +    // for now, just say we're ready
> 
> Why? Can we remove this code for now, just until is is ready and it is clear
> why this stuff happens?

It was our milestone last week to get the watch() function fully implemented.  We've done that, and the comment has been removed

> @@ +97,5 @@
> > +  },
> > +
> > +  /*
> > +   *
> > +   */
> 
> No comment so I believe you can just remove those.
> 
> @@ +109,5 @@
> > +  _openDialogAndSendMessage: function SignInToWebsiteController_openDialogAndSendMessage(aRpId, aMessage, aOptions) {
> > +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> > +    let content = browser.getContentWindow();
> > +    if (!content) {
> > +      // aErrorCb.onresult("NO_CONTENT_WINDOW");
> 
> No dead code please.

sorry. removed.

> @@ +127,5 @@
> > +      log("id after is ", id);
> > +      let msg = evt.detail;
> > +      if (msg.id != id) {
> > +        log("mozContentEvent. evt.detail.id != ", id, msg);
> > +        content.removeEventListener("mozContentEvent", getAssertion);
> 
> So if there is 1 mozContentEvent for something else you're not going to
> listen the assertion anymore?

This has been fixed.  The listener should only be removed once a message with the correct id has been received.  So no, no listeners should have been removed if messages with a different id were received.

> @@ +157,5 @@
> > +      mm.sendAsyncMessage(aMessage, aOptions);
> > +
> > +      log("done load frame script");
> > +
> > +      content.removeEventListener("mozContentEvent", getAssertion);
> 
> I would move this code right after the msg.id != id check. So it is obvious
> that it is removed as soon as we have a matching id.

Agreed.  It is there now.

> ::: b2g/components/test/unit/head_identity.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +const Cc = Components.classes;
> 
> Cc is unused.

removed

> @@ +3,5 @@
> > +
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> > +const Cr = Components.results;
> 
> Cr is unused.

also removed

> @@ +48,5 @@
> > +}
> > +
> > +// create a mock "doc" object, which the Identity Service
> > +// uses as a pointer back into the doc object
> > +function mock_doc(aIdentity, aOrigin, aDoFunc) {
> 
> Why is it not camelCase?

corrected

> @@ +67,5 @@
> > +
> > +// mimicking callback funtionality for ease of testing
> > +// this observer auto-removes itself after the observe function
> > +// is called, so this is meant to observe only ONE event.
> > +function makeObserver(aObserveTopic, aObserveFunc) {
> 
> Why is it camelCase while the other parts are not?

fixed non_camel_case things

> @@ +76,5 @@
> > +
> > +    observe: function (aSubject, aTopic, aData) {
> > +      if (aTopic == aObserveTopic) {
> > +        aObserveFunc(aSubject, aTopic, aData);
> > +        Services.obs.removeObserver(observer, aObserveTopic);
> 
> Personally I will remove the observer before the aObserverFunc, you never
> know if this one will fail or not.

Thanks for catching that.  Done.

> @@ +90,5 @@
> > +function setup_test_identity(identity, cert, cb) {
> > +  // set up the store so that we're supposed to be logged in
> > +  let store = get_idstore();
> > +
> > +  function keyGenerated(err, kpo) {
> 
> What is kpo ?

The key generation functions are not needed in this test, so I've removed all this.

> @@ +101,5 @@
> > +
> > +// takes a list of functions and returns a function that
> > +// when called the first time, calls the first func,
> > +// then the next time the second, etc.
> > +function call_sequentially() {
> 
> This function seems unused. Please remove it.

I've added more tests that use it now.

> ::: b2g/components/test/unit/test_signintowebsite.js
> @@ +1,1 @@
> > +"use strict";
> 
> It miss the file header.
> 
> @@ +26,5 @@
> > +    do_test_finished();
> > +    run_next_test();
> > +  });
> > +
> > +  mockedDoc.doCoffee();
> 
> I have no **** clue what this stuff does. Is there a way to give it a
> real name?

I've added a comment explaining.  I merely added a bogus function (doCoffee) to smoke-test the mocked docs and their message handlers.  I guess I was thinking about coffee ...

> ::: dom/identity/DOMIdentity.jsm
> @@ +16,3 @@
> >  XPCOMUtils.defineLazyModuleGetter(this, "IdentityService",
> >                                    "resource://gre/modules/identity/Identity.jsm");
> > +*/
> 
> Do you really want to leave dead code?

nope.  removed.

> @@ +137,2 @@
> >                             .QueryInterface(Ci.nsIFrameLoaderOwner)
> > +                           .frameLoader.messageManager;*/
> 
> Same comment as above.

removed

> ::: dom/identity/nsDOMIdentity.js
> @@ +491,4 @@
> >                        .getInterface(Ci.nsIWebNavigation)
> >                        .QueryInterface(Ci.nsIInterfaceRequestor)
> >                        .getInterface(Ci.nsIContentFrameMessageManager);
> > +    */
> 
> Same comment as above.

removed

> ::: toolkit/identity/MinimalIdentity.jsm
> @@ +78,5 @@
> > +    }
> > +    return null;
> > +  },
> > +
> > +  // RP stuff
> 
> That's not really a useful comment for someone like me that jump into this
> code :)

Adding documentation

> ::: toolkit/identity/tests/Makefile.in
> @@ +12,5 @@
> >  
> >  MODULE = test_identity
> >  XPCSHELL_TESTS = unit
> >  
> > +DIRS = chrome mochitest 
> 
> Remove the trailing whitespace. Only change in this file.

removed
Attachment #673921 - Attachment is obsolete: true
Attachment #673921 - Flags: review?(benadida)
Attachment #673930 - Flags: review?(benadida)
Looks like I still have a leak:

https://tbpl.mozilla.org/?tree=Try&rev=6fa69e4dce90

Looking for it now
tbpl: https://tbpl.mozilla.org/?tree=Try&rev=af5200f4397c
Attachment #673930 - Attachment is obsolete: true
Attachment #673930 - Flags: review?(benadida)
Attachment #673991 - Flags: review?(benadida)
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854

Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*, run in the b2g process.

This is a P1 blocking security issue.
Attachment #673209 - Flags: review?(jones.chris.g) → review-
(In reply to Chris Jones [:cjones] [:warhammer] from comment #57)
> Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*,
> run in the b2g process.

Chris: makes sense, but can I ask that we allow this code to land so that testing can begin, and mark the followup issue, bug 804143 as the P1 blocking security issue?

Note that this is exactly what happened with payments, it was landed without OOP, and then OOP was added. I'm not arguing that we shouldn't do it, only that we should pipeline so that QA can start its work.
(In reply to Ben Adida [:benadida] from comment #58)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #57)
> > Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*,
> > run in the b2g process.
> 
> Chris: makes sense, but can I ask that we allow this code to land so that
> testing can begin, and mark the followup issue, bug 804143 as the P1
> blocking security issue?
> 

If it's a matter of setting remote=true somewhere, I would really prefer we didn't allow the possibility of forgetting this.

> Note that this is exactly what happened with payments, it was landed without
> OOP, and then OOP was added. I'm not arguing that we shouldn't do it, only
> that we should pipeline so that QA can start its work.

I didn't know about that!  If I had, I would have r-'d just as hard there ;).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> If it's a matter of setting remote=true somewhere, I would really prefer we
> didn't allow the possibility of forgetting this.

Agreed, but first echos from my team is that it's a bit more complicated than that. I think we'll get to it next week. This Friday is the agreed-upon feature freeze for identity & payments, which is why I'm pushing for this.

There's no doubt we will do this, we understand the non-negotiable requirement, but payments is going to get stuck on ID integration, and QA is stuck too.

Anyways, that's my last pitch, please consider it, and let me know your final call :)
Comment on attachment 673991 [details] [diff] [review]
connect gecko to gaia identity ui, v4


r+ with two nits below:

>+++ b/b2g/chrome/content/identity.js
>+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
>+                                   "@mozilla.org/childprocessmessagemanager;1",
>+                                   "nsIMessageSender");

looks like this isn't used, let's remove it.

>+    BrowserID.internal.watch(function(aParams) {

Let's add a pointer to the Internal API documentation:
https://github.com/mozilla/browserid/wiki/Front-End-Development
Attachment #673991 - Flags: review?(benadida) → review+
(In reply to Ben Adida [:benadida] from comment #60)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> > If it's a matter of setting remote=true somewhere, I would really prefer we
> > didn't allow the possibility of forgetting this.
> 
> Agreed, but first echos from my team is that it's a bit more complicated
> than that. I think we'll get to it next week. This Friday is the agreed-upon
> feature freeze for identity & payments, which is why I'm pushing for this.
> 

Can you help me understand what the complication is?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #62)
> Can you help me understand what the complication is?

I'll check. Looks like payments tweaks was tiny, I must have been looking at something else. Thanks for pushing, will get back to you asap.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #62)
> (In reply to Ben Adida [:benadida] from comment #60)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> > > If it's a matter of setting remote=true somewhere, I would really prefer we
> > > didn't allow the possibility of forgetting this.
> > 
> > Agreed, but first echos from my team is that it's a bit more complicated
> > than that. I think we'll get to it next week. This Friday is the agreed-upon
> > feature freeze for identity & payments, which is why I'm pushing for this.
> > 
> 
> Can you help me understand what the complication is?

Setting the frame to remote broke how we close the dialog, so we'll need to do some refactoring there. I'm taking a look at payments to see if they did anything additional to have it work with OOP.
That's surprising.  Are you sure that wasn't a bug that's already been fixed?
Once again, Chris, the issue is not that we can't solve this, I'm quite certain we can, the issue is that our landing pipeline is getting incredibly delayed, and we're about to blow past the deadlines.

Can you make a call on whether we can land what we have, and fix this in a separate p1 blocking bug? It would help us a lot.

If not, don't worry, we'll figure out the technical constraint, but we will likely miss the Friday deadline. That would be really unfortunate.
Setting aside the question of deadlines and landing without this issue fixed, I'm happy to provide assistance if you have any specific questions.
Ben, I haven't gotten a clear answer yet on whether this won't work if landed with remote=true.  I'm trying to get information to understand that, which is what I need to make the call.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #65)
> That's surprising.  Are you sure that wasn't a bug that's already been fixed?

It still occurs against the latest Gaia, at least.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #68)
> Ben, I haven't gotten a clear answer yet on whether this won't work if
> landed with remote=true.

I'm confused, in https://bugzilla.mozilla.org/show_bug.cgi?id=794680#c64 Zach said:

> Setting the frame to remote broke how we close the dialog, so
> we'll need to do some refactoring there. I'm taking a look
> at payments to see if they did anything additional to have
> it work with OOP.

In other words, setting remote=true breaks our code (which was modeled on an earlier version of the payments stuff.)

I think we know more or less what the path is to fixing this, but it goes a good bit beyond setting remote=true. Justin, thanks for the offer to help, we'll definitely take you up on it soon if we can't figure this out!

Do you think we need to fully figure this out before you can make a decision on landing this patch that doesn't have the fix? That seems like the equivalent of simply waiting until we have OOP implemented, and thus not landing this patch.

So I would propose that you make a call based on the information we have so far, the fact that it will take us a few days to get OOP & our functionality working properly together, and the promise that we will get it done but, in the meantime, we want to start enabling QA and payments integration.

I understand if that's not enough, but if so let's determine that now and move quickly to finding the solution.
Flags: needinfo?(dietrich)
(In reply to Ben Adida [:benadida] from comment #70)
> Do you think we need to fully figure this out before you can make a decision
> on landing this patch that doesn't have the fix? That seems like the
> equivalent of simply waiting until we have OOP implemented, and thus not
> landing this patch.
> 
> So I would propose that you make a call based on the information we have so
> far, the fact that it will take us a few days to get OOP & our functionality
> working properly together, and the promise that we will get it done but, in
> the meantime, we want to start enabling QA and payments integration.
> 

I'm fine to land the code without remote=true if you file a bug for it and ask for blocking-basecamp? on it.

Thanks for the quick fixes.
(In reply to Vivien Nicolas (:vingtetun) from comment #71)
> I'm fine to land the code without remote=true if you file a bug for it and
> ask for blocking-basecamp? on it.

Did you one better: just marked bug 804143 blocking-basecamp+
Comment on attachment 673991 [details] [diff] [review]
connect gecko to gaia identity ui, v4

DOM changes look good, but two nits below that should be fixed while we're here:

- In DOMIdentity.jsm:

 let DOMIdentity = {
   // nsIMessageListener
   receiveMessage: function DOMIdentity_receiveMessage(aMessage) {
+      log("**received message", aMessage);
     let msg = aMessage.json;

Fix indentation.
 
     // Target is the frame message manager that called us and is
     // used to send replies back to the proper window.
-    let targetMM = aMessage.target
+    /* let targetMM = aMessage.target
                            .QueryInterface(Ci.nsIFrameLoaderOwner)
-                           .frameLoader.messageManager;
+                           .frameLoader.messageManager;*/
+    let targetMM = aMessage.target;
 
Remove the commented out code, as others noted as well.

r=jst with that!
Addresses jst's comments.

Fixes a bug in DOMIdentity.jsm that was causing test_watch in test_minimalidentity to fail

I still seem to have a ppmm memory leak that's showing up in mochitests.
Attachment #673991 - Attachment is obsolete: true
cjones: one last request to get this patch landed while we work on OOP.

We're making progress, but we've got a heisenbug that sometimes causes a permission error, sometimes doesn't, and it's pretty clear we won't get to the bottom of it without some more involved work.

We'd love to still make the Friday feature deadline, and then follow up by fixing this bug. And of course we'll fix the bug as soon as possible.

So, there you have it, please r+ if you can, and we'll keep working on OOP.
I need a better diagnosis than "heisenbug".  jlebar has outstanding questions in the OOP bug.  This discussion may all be moot if it's something easy :).

I just think there's a huge disconnect here.  You're asking for permission to land an sg:crit patch.  The standard for that is extremely high.  The Friday deadline isn't at risk yet.
cjones: we don't have a better diagnosis at this time. That's the issue. Sometimes it works, sometimes a permission error is thrown.

It looks like you want us to solve this before we decide whether to land without the fix, which makes no sense: if we had a complete diagnosis, we would also be minutes away from a patch. And then we wouldn't need this discussion.

So I'm going to conclude the inevitable here, which is that you don't want to land this without this issue taken care of. I'll mark the OOP issue a blocker for this.

Yes, this almost certainly means missing Friday's deadline.
Depends on: 804143
There were open questions in bug 804143 for over a day.  jlebar is actively helping.  I don't believe you're much interested in devoting time to OOP, which is exactly what I'm afraid of.

The way these decisions work is, we need information to decide on a trade-off.  I'm asking for information because what I have currently is insufficient.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #78)
> There were open questions in bug 804143 for over a day.  jlebar is actively
> helping.  I don't believe you're much interested in devoting time to OOP,
> which is exactly what I'm afraid of.

That was my fault. I was having problems with my build most of yesterday so I was unable to try his suggestions.
(In reply to [:cjones] and [:zaach] from comment #78 and comment #79)

I also spent much of yesterday and yesterday night investigating this from the gecko side (b2g/components/SignInToWebsite.jsm), though without success.  I should have messaged out more clearly that I was actively working on it.  I'm grateful to you and jlebar for your help on this perplexing problem.
Thanks, sorry for the misunderstanding.  We really honestly just want to help :).
(In reply to Daniel Holbert [:dholbert] from comment #28)

I've found the memory leak and patched it.  Local mochitests look good.  Now trying to package things up so they work for both b2g device and desktop and also for firefox desktop.
My focus in this patch was to fix the memory leaks noted in Comment #28.

I still have mochitests 2 and 5 timing out.  Working on that next.

https://tbpl.mozilla.org/?tree=Try&rev=06653e30f528
Attachment #674740 - Attachment is obsolete: true
(In reply to Jed Parsons [:jparsons] from comment #83)

submitted follow-up bug 805602 to restore mochitests
Comment on attachment 675295 [details] [diff] [review]
connect gecko to gaia identity ui, v8


This looks great. r+ assuming fix to one small nit:

>+++ b/b2g/chrome/content/identity.js

>+  if (!content) {
>+    return;
>+  }

Now that we use the magic variable "content", I don't see how this check is useful. Let's remove it.
Attachment #675295 - Flags: review?(benadida) → review+
cjones: looks like this is already pref'ed off, and we will not pref it on until bug 804143 is addressed. If you can r+, that would be awesome.
Addresses issue in Comment #86
Attachment #675295 - Attachment is obsolete: true
By convention, all available prefs are defined in modules/libpref/src/init/all.js.  So we need an entry like this there:

pref("dom.identity.enabled", false);

r=me with that.
Per Comment #89, ensure that toolkit.identity is off by default.
Attachment #675457 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/14cf08f2845d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
QA Contact: jsmith
Whiteboard: [LOE:M] → [LOE:M] [qa-]
Whiteboard: [LOE:M] [qa-] → [LOE:M] [qa-][feature-complete]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.