Last Comment Bug 804143 - [Identity] Make id flow OOP
: [Identity] Make id flow OOP
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Identity (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Zachary Carter [:zaach]
: Jason Smith [:jsmith]
Mentors:
Depends on: 804080 807078
Blocks: 767818 basecamp-id 794680
  Show dependency treegraph
 
Reported: 2012-10-22 06:42 PDT by Ben Adida [:benadida]
Modified: 2013-01-04 13:05 PST (History)
16 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
refactor dialog close messaging (2.54 KB, patch)
2012-10-31 16:55 PDT, Zachary Carter [:zaach]
benadida: review-
Details | Diff | Splinter Review
refactor dialog close messaging (4.70 KB, patch)
2012-11-01 12:52 PDT, Zachary Carter [:zaach]
benadida: review+
Details | Diff | Splinter Review

Description Ben Adida [:benadida] 2012-10-22 06:42:15 PDT
+++ This bug was initially created as a clone of Bug #804080 +++

As pointed out by Vivien in https://bugzilla.mozilla.org/show_bug.cgi?id=794680#c41, there is no reason why the payment and identity flows should be in-process. This bug applies to the identity flow.
Comment 1 Andreas Gal :gal 2012-10-22 06:46:45 PDT
We will not block on this, but we really really want this and we will approve a patch for landing as long it comes within the next 2-3 weeks. Ben, does your team understand what has to be done here? (it was trivial for mozPay, I don't know how hard it would be here).
Comment 2 Ben Adida [:benadida] 2012-10-22 14:20:05 PDT
(In reply to Andreas Gal :gal from comment #1)
> Ben, does your team understand what has to be done here?

Yes, I believe we do. If we need help, we will definitely reach out.
Comment 3 Zachary Carter [:zaach] 2012-10-23 12:13:55 PDT
I sent this to jlebar; posting here to continue discussion:
> Hey Justin,
>
> We've been following the payments code closely to implement identity, so switching to OOP should work in theory. However, we're getting a permission error in a place that, supposedly, payments does not, so I wanted to run the code by you to see if this actually should be failing.
>
> We're injecting a script into the remote frame and we receive a permission error when we try to obtain a reference to most recent browser window from that script. Here's the line that fails:
> https://hg.mozilla.org/users/jparsons_mozilla.com/b2g-payments-identity/file/292a047bfef5/b2g/chrome/content/identity.js#l79
>
> payments.js uses the same technique. Any insight here?
Comment 4 Justin Lebar (not reading bugmail) 2012-10-23 12:17:01 PDT
You cannot access DOM objects which live in a different process.

A remote frame's window and DOM objects live in a different process -- that's what makes the frame remote.

That code tries to access contentWindow() on a remote frame.  Because of the above, that will never work.
Comment 5 Zachary Carter [:zaach] 2012-10-23 12:48:23 PDT
Th(In reply to Justin Lebar [:jlebar] from comment #4)
> You cannot access DOM objects which live in a different process.
> 
> A remote frame's window and DOM objects live in a different process --
> that's what makes the frame remote.
> 
> That code tries to access contentWindow() on a remote frame.  Because of the
> above, that will never work.

That makes sense. However, payments.js uses this same technique: http://hg.mozilla.org/integration/mozilla-inbound/file/b3c8f279987b/b2g/chrome/content/payment.js#l47

We can probably work around this, but it was confusing seeing it landed in payments.
Comment 6 Justin Lebar (not reading bugmail) 2012-10-23 12:58:04 PDT
Oh, I missed the part about this script running injected into a remote frame.  That should work in both cases.  You just can't access the content window from the parent process.

What's the error being thrown?
Comment 7 Zachary Carter [:zaach] 2012-10-23 14:33:58 PDT
"Permission denied to access object"
Comment 8 Justin Lebar (not reading bugmail) 2012-10-23 15:14:45 PDT
What is the result of the getMostRecentWindow call?  (What properties does the object have?  Can you toString it?)

What are you trying to accomplish with that function call?  There may be a more straightforward way of doing it.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-23 15:43:51 PDT
We need an assignee for this bug.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-23 15:48:02 PDT
And Alex and Lucas need to be aware that core parts of gecko will be updating randomly, apart from the rest of the system.  And clee needs to be aware that the user will be charged for those random invisible updates.
Comment 11 Lucas Adamski [:ladamski] 2012-10-23 15:57:21 PDT
Chris, I'm not sure I understand comment 10 in relation to this bug?
Comment 12 Zachary Carter [:zaach] 2012-10-24 12:24:01 PDT
(In reply to Justin Lebar [:jlebar] from comment #8)
> What is the result of the getMostRecentWindow call?  (What properties does
> the object have?  Can you toString it?)
Couldn't toString it, but it's an [object ChromeWindow] type.

> 
> What are you trying to accomplish with that function call?  There may be a
> more straightforward way of doing it.

We attach an event handler to the frame so gaia can signal when it has closed the dialog. The permission error occurs when we try to obtain a reference to the frame's content window.
Comment 13 Justin Lebar (not reading bugmail) 2012-10-24 12:27:15 PDT
> We attach an event handler to the frame so gaia can signal when it has closed the dialog. 

Which frame?  The frame containing the identity code?  Can you just refer to |content| from within this code?  https://developer.mozilla.org/en-US/docs/The_message_manager#Content_scripts
Comment 14 Zachary Carter [:zaach] 2012-10-24 14:17:43 PDT
I was getting permission errors when I tried that before, but I think there was another problem (referencing the "browser" global) that I didn't catch. With the original/Payment approach, after removing some logging (and fixing my build) things seemed to work, but using the content variable directly seems to work consistently – thanks!

Jed also pointed out that we do things a little differently than Payments in the chrome code that injects the script into the frame (they bind the event handler that does the injection to an object[1], we don't). If this works we may not have to worry about that, but I wanted to point it out in case it might be relevant.

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/b3c8f279987b/b2g/components/PaymentGlue.js#l133
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-24 14:35:57 PDT
\o/
Comment 16 Zachary Carter [:zaach] 2012-10-24 16:20:09 PDT
Hrmm. It appears Jed is still encountering the permission error, even using the provided 'content'. Investigating...
Comment 17 Ben Adida [:benadida] 2012-10-24 16:53:33 PDT
cjones: hopefully you can see here that we are, in fact, working hard on this, but that there is enough uncertainty that Friday is at significant risk.

Is there any more information we can give you to help you make the decision on landing the parent patch while we feverishly work on this?
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-24 17:19:07 PDT
I do see that, thanks :).

I heard earlier today that the plan was to land this code pref'd off.  Is that correct?

How about we compromise and make fixing this bug a requirement for pref'ing on?
Comment 19 Ben Adida [:benadida] 2012-10-24 19:49:29 PDT
cjones: I'm very happy with that compromise. Let's do it.
Comment 20 Zachary Carter [:zaach] 2012-10-31 16:55:52 PDT
Created attachment 677222 [details] [diff] [review]
refactor dialog close messaging

This is based off of the patch in 807078.
Comment 21 Zachary Carter [:zaach] 2012-10-31 17:09:12 PDT
github PR for gaia: https://github.com/mozilla-b2g/gaia/pull/6125
Comment 22 Ben Adida [:benadida] 2012-10-31 21:21:58 PDT
Comment on attachment 677222 [details] [diff] [review]
refactor dialog close messaging

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

r- for now, let's discuss the topic below.

::: b2g/components/SignInToWebsite.jsm
@@ +224,5 @@
>          mm.removeMessageListener(kIdentityControllerDoMethod, aMessageCallback);
> +
> +        // tell gaia to close the dialog
> +        let detail = message.json;
> +        GaiaInterface.sendChromeEvent(detail);

this is passing along the detail object provided in the message from identity.js, but there's no guarantee that this tells Gaia to close the dialog. Any reason why the detail object needs to be generated in identity.js? Or should it just be specified here?
Comment 23 Zachary Carter [:zaach] 2012-11-01 12:52:59 PDT
Created attachment 677530 [details] [diff] [review]
refactor dialog close messaging

Good point Ben, there's no reason for detail to be defined there. Updated patch.
Comment 24 Ben Adida [:benadida] 2012-11-01 20:54:31 PDT
Comment on attachment 677530 [details] [diff] [review]
refactor dialog close messaging

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

awesome. r+
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-11-02 17:19:15 PDT
https://hg.mozilla.org/mozilla-central/rev/605db61115a4
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-11-02 17:21:20 PDT
BTW, please make sure your future patches follow the directions below so they have all the needed metadata in them for committing. It makes life much easier for those checking in on your behalf :)
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

(For example, it wouldn't leave me guessing how you want your name/email address to appear)
Comment 27 Jason Smith [:jsmith] 2012-11-03 01:09:20 PDT
The Gaia PR hasn't landed. Reopening.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-11-03 05:27:58 PDT
Please put [leave open] in the whiteboard for bugs like these so they aren't auto-closed when landing on m-c.
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-11-03 07:49:47 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/1240dc686edb
Comment 30 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-11-07 11:14:13 PST
(In reply to Jason Smith [:jsmith] from comment #27)
> The Gaia PR hasn't landed. Reopening.

I just landed it. Re-closing.
Comment 31 Jason Smith [:jsmith] 2013-01-04 13:05:49 PST
Sanity tests have revealed a basic sign in works out of process, so this is verified then.

Note You need to log in before you can comment on or make changes to this bug.