The default bug view has changed. See this FAQ.

[Identity] Make id flow OOP

VERIFIED FIXED in Firefox 18

Status

()

Core
Identity
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: benadida, Assigned: zaach)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

5 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).
blocking-basecamp: ? → -
(Reporter)

Comment 2

5 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.
(Reporter)

Updated

5 years ago
blocking-basecamp: - → +
(Assignee)

Comment 3

5 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?
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.
(Assignee)

Comment 5

5 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.
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?
(Assignee)

Comment 7

5 years ago
"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?
(Reporter)

Updated

5 years ago
Assignee: nobody → zack.carter
Status: NEW → ASSIGNED
(Assignee)

Comment 12

5 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.
(Reporter)

Updated

5 years ago
Blocks: 794680
> 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
(Assignee)

Comment 14

5 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
\o/
(Assignee)

Comment 16

5 years ago
Hrmm. It appears Jed is still encountering the permission error, even using the provided 'content'. Investigating...
(Reporter)

Comment 17

5 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?
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?
(Reporter)

Comment 19

5 years ago
cjones: I'm very happy with that compromise. Let's do it.
(Assignee)

Updated

5 years ago
Depends on: 807078
No longer depends on: 804080
(Assignee)

Updated

5 years ago
Depends on: 804080
(Assignee)

Comment 20

5 years ago
Created attachment 677222 [details] [diff] [review]
refactor dialog close messaging

This is based off of the patch in 807078.
Attachment #677222 - Flags: review?(benadida)
Attachment #677222 - Flags: feedback?(jparsons)
(Assignee)

Comment 21

5 years ago
github PR for gaia: https://github.com/mozilla-b2g/gaia/pull/6125
(Reporter)

Comment 22

5 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?
Attachment #677222 - Flags: review?(benadida) → review-
(Assignee)

Comment 23

5 years ago
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.
Attachment #677222 - Attachment is obsolete: true
Attachment #677222 - Flags: feedback?(jparsons)
Attachment #677530 - Flags: review?(benadida)
Attachment #677530 - Flags: feedback?(jparsons)
(Reporter)

Comment 24

4 years ago
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+

Updated

4 years ago
Blocks: 794552
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/605db61115a4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite-
Keywords: checkin-needed
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)

Updated

4 years ago
Keywords: verifyme
QA Contact: jsmith
The Gaia PR hasn't landed. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
Keywords: verifyme
Please put [leave open] in the whiteboard for bugs like these so they aren't auto-closed when landing on m-c.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1240dc686edb
status-firefox18: --- → fixed
status-firefox19: --- → fixed
(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
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
Attachment #677530 - Flags: feedback?(jparsons)
Sanity tests have revealed a basic sign in works out of process, so this is verified then.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.