Closed Bug 804143 Opened 9 years ago Closed 8 years ago
[Identity] Make id flow OOP
+++ 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.
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).
blocking-basecamp: ? → -
(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.
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?
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.
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.
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?
"Permission denied to access object"
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.
We need an assignee for this bug.
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.
Chris, I'm not sure I understand comment 10 in relation to this bug?
Assignee: nobody → zack.carter
Status: NEW → ASSIGNED
(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.
> 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
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, 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.  http://hg.mozilla.org/integration/mozilla-inbound/file/b3c8f279987b/b2g/components/PaymentGlue.js#l133
Hrmm. It appears Jed is still encountering the permission error, even using the provided 'content'. Investigating...
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?
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?
cjones: I'm very happy with that compromise. Let's do it.
This is based off of the patch in 807078.
github PR for gaia: https://github.com/mozilla-b2g/gaia/pull/6125
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?
Attachment #677222 - Flags: review?(benadida) → review-
Good point Ben, there's no reason for detail to be defined there. Updated patch.
Comment on attachment 677530 [details] [diff] [review] refactor dialog close messaging Review of attachment 677530 [details] [diff] [review]: ----------------------------------------------------------------- awesome. r+
Attachment #677530 - Flags: review?(benadida) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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)
The Gaia PR hasn't landed. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please put [leave open] in the whiteboard for bugs like these so they aren't auto-closed when landing on m-c.
(In reply to Jason Smith [:jsmith] from comment #27) > The Gaia PR hasn't landed. Reopening. I just landed it. Re-closing.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → FIXED
8 years ago
Sanity tests have revealed a basic sign in works out of process, so this is verified then.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.