Ben Adida [:benadida] Reporter
	Description • 12 years ago

+++ 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.

	Andreas Gal :gal
	Comment 1 • 12 years ago

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).

	Ben Adida [:benadida] Reporter
	Comment 2 • 12 years ago

(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.

	Zachary Carter [:zaach] Assignee
	Comment 3 • 12 years ago

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?

	Justin Lebar (not reading bugmail)
	Comment 4 • 12 years ago

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.

	Zachary Carter [:zaach] Assignee
	Comment 5 • 12 years ago

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.

	Justin Lebar (not reading bugmail)
	Comment 6 • 12 years ago

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?

	Zachary Carter [:zaach] Assignee
	Comment 7 • 12 years ago

"Permission denied to access object"

	Justin Lebar (not reading bugmail)
	Comment 8 • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 9 • 12 years ago

We need an assignee for this bug.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 10 • 12 years ago

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.

	Lucas Adamski [:ladamski]
	Comment 11 • 12 years ago

Chris, I'm not sure I understand comment 10 in relation to this bug?

	Zachary Carter [:zaach] Assignee
	Comment 12 • 12 years ago

(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.

	Justin Lebar (not reading bugmail)
	Comment 13 • 12 years ago

> 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

	Zachary Carter [:zaach] Assignee
	Comment 14 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 15 • 12 years ago

\o/

	Zachary Carter [:zaach] Assignee
	Comment 16 • 12 years ago

Hrmm. It appears Jed is still encountering the permission error, even using the provided 'content'. Investigating...

	Ben Adida [:benadida] Reporter
	Comment 17 • 12 years ago

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?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 18 • 12 years ago

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?

	Ben Adida [:benadida] Reporter
	Comment 19 • 12 years ago

cjones: I'm very happy with that compromise. Let's do it.

	Zachary Carter [:zaach] Assignee
	Comment 20 • 12 years ago

This is based off of the patch in 807078.

	Zachary Carter [:zaach] Assignee
	Comment 21 • 12 years ago

github PR for gaia: https://github.com/mozilla-b2g/gaia/pull/6125

	Ben Adida [:benadida] Reporter
	Comment 22 • 12 years ago

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?

	Zachary Carter [:zaach] Assignee
	Comment 23 • 12 years ago

Good point Ben, there's no reason for detail to be defined there. Updated patch.

	Ben Adida [:benadida] Reporter
	Comment 24 • 12 years ago

Comment on attachment 677530 [details] [diff] [review]
refactor dialog close messaging

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

awesome. r+

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/605db61115a4

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 12 years ago

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)

	Jason Smith [:jsmith]
	Comment 27 • 12 years ago

The Gaia PR hasn't landed. Reopening.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 12 years ago

Please put [leave open] in the whiteboard for bugs like these so they aren't auto-closed when landing on m-c.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/1240dc686edb

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 30 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #27)
> The Gaia PR hasn't landed. Reopening.

I just landed it. Re-closing.

	Jason Smith [:jsmith]
	Comment 31 • 12 years ago

Sanity tests have revealed a basic sign in works out of process, so this is verified then.