Closed Bug 1399070 Opened 7 years ago Closed 7 years ago

browser.identity.launchWebAuthFlow() breaks with OOP

Categories

(WebExtensions :: General, defect, P1)

56 Branch
x86_64
Windows
defect

Tracking

(firefox55 unaffected, firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 --- unaffected
firefox56 --- verified
firefox57 --- verified

People

(Reporter: emilpasca, Assigned: mixedpuppy)

Details

Attachments

(2 files)

Attached file 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?
Flags: needinfo?(mixedpuppy)
Summary: [Firefox Notes] "Sync Your Notes" button is not actionable on Firefox Beta and Nightly → browser.identity.launchWebAuthFlow() breaks with OOP
identity needs to move to the parent.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Priority: -- → P1
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 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.
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.
(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.
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
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
Closed: 7 years ago
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)
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
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+
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: