browser.identity.launchWebAuthFlow() breaks with OOP

VERIFIED FIXED in Firefox 56

Status

P1
normal
VERIFIED FIXED
a year ago
4 months ago

People

(Reporter: emilpasca, Assigned: mixedpuppy)

Tracking

56 Branch
mozilla57
x86_64
Windows

Firefox Tracking Flags

(firefox55 unaffected, firefox56 verified, firefox57 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8907009 [details]
fx-notes-sync-01.xpi

[Affected versions]:
- Nightly (57.0a1-20170910220126)
- Firefox Beta  (56.0b2-20170810113054)
- Firefox Notes v1.8.0dev

[Prerequisites]:
- Have a Firefox profile with the latest Firefox Notes add-on version (1.8.0dev).

[Steps to reproduce]:
1. Open the browser with the profile from prerequisites.
2. Click the "Sync Your Notes" button from the "Notes" sidebar.
3. Observe the browser behavior.

[Expected result]:
- Firefox Notes "Sign in" window is displayed.

[Actual result]:
- Nothing happens.

[Regression]:
Last good revision: 392ed89ec2730a48d10b1cec741e86a242d28aa3
First bad revision: a625a2e9b3333a8e76982ea65f077cfded6ac224
Pushlog: https://goo.gl/AUTdwb

[Notes]:
- Attached Browser Console messages:
`[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowWatcher.openWindow]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://extensions/content/ext-c-identity.js :: openOAuthWindow :: line 69"  data: no]  (unknown)
Unchecked lastError value: DataCloneError: The object could not be cloned.  ExtensionCommon.jsm:407
Error: An unexpected error occurred`
- Attached a screen recording of the issue: https://goo.gl/uoR9i4
- Attached the add-on XPI
- Refs: https://github.com/mozilla/notes/issues/229

Kris, can you please have a look at this?

Updated

a year ago
Flags: needinfo?(mixedpuppy)
Summary: [Firefox Notes] "Sync Your Notes" button is not actionable on Firefox Beta and Nightly → browser.identity.launchWebAuthFlow() breaks with OOP
(Assignee)

Comment 1

a year ago
identity needs to move to the parent.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

a year ago
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
The patch splits the identity api in two.  This has to happen since the getRedirectURL is a synchronous call, thus can only happen in the child.  Using window opener must happen in the parent.  The schema is located in browser since eventually we may have mobile support if needed.

Comment 4

a year ago
mozreview-review
Comment on attachment 8907386 [details]
Bug 1399070 move launchWebAuthFlow to parent to fix opening auth window when remote,

https://reviewboard.mozilla.org/r/179074/#review184716

::: commit-message-a73cc:1
(Diff revision 1)
> +Bug 1399070 move identity to parent so openWindow will work in oop, r?zombie

nit: update the commit message please

::: toolkit/components/extensions/ext-identity.js
(Diff revision 1)
>                return Promise.reject({message: "redirect_uri is missing"});
>              }
>            } catch (e) {
>              return Promise.reject({message: "redirect_uri is invalid"});
>            }
> -          if (!redirectURI.href.startsWith(this.getRedirectURL())) {

If this warning is still important, you could leave all these checks in the child side of the api, which would just call the parent side via `context.childManager.callParentAsyncFunction()`, something like [1].
 
1) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-c-storage.js#95
Attachment #8907386 - Flags: review?(tomica) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> The schema is located in
> browser since eventually we may have mobile support if needed.

I'm guessing you meant *not* located in the browser.
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8907386 [details]
Bug 1399070 move launchWebAuthFlow to parent to fix opening auth window when remote,

https://reviewboard.mozilla.org/r/179074/#review184716

> If this warning is still important, you could leave all these checks in the child side of the api, which would just call the parent side via `context.childManager.callParentAsyncFunction()`, something like [1].
>  
> 1) http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-c-storage.js#95

Yeah, I looked at that for a bit and decided it is not important.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
(In reply to Tomislav Jovanovic :zombie from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > The schema is located in
> > browser since eventually we may have mobile support if needed.
> 
> I'm guessing you meant *not* located in the browser.

I meant it is in ext-browser.json instead of ext-toolkit.json since this is not ready for android yet.

Comment 9

a year ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7b5261e3f38e
move launchWebAuthFlow to parent to fix opening auth window when remote, r=zombie
(Assignee)

Comment 10

a year ago
Comment on attachment 8907386 [details]
Bug 1399070 move launchWebAuthFlow to parent to fix opening auth window when remote,

Approval Request Comment
[Feature/Bug causing the regression]: identity api
[User impact if declined]: inability for extensions using identity api to work
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet, early request to catch eyes
[Needs manual test from QE? If yes, steps to reproduce]: yes, OP has good STR
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: moderate, but code is relatively unchanged.  It was split into content and parent parts.
[Why is the change risky/not risky?]: ^
[String changes made/needed]: none
Attachment #8907386 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/7b5261e3f38e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Andrei, can we verify this fix before we uplift for the 56 RC?
Flags: needinfo?(andrei.vaida)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Andrei, can we verify this fix before we uplift for the 56 RC?

This request will be picked up by the Engineering QA team. Emil will take point.
Flags: needinfo?(andrei.vaida) → needinfo?(emil.pasca)
(Reporter)

Comment 14

a year ago
I have verified this issue on Mac OS 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Nightly(57.0a1-20170917220255) and it is no longer reproducible.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: needinfo?(emil.pasca)
Comment on attachment 8907386 [details]
Bug 1399070 move launchWebAuthFlow to parent to fix opening auth window when remote,

Please uplift to m-r for today's 56 RC build. We want this API to work on release!
Attachment #8907386 - Flags: approval-mozilla-release? → approval-mozilla-release+
(Reporter)

Comment 17

a year ago
I have verified this issue on Mac OS 10.12, Windows 10 x64 and Ubuntu 14.04 x64 with the latest Firefox Beta (56.0-20170918210324) and it is no longer reproducible.
status-firefox56: fixed → verified

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.