1498233 - Password Doorhanger is not displayed using custom tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Custom Tabs, defect)

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: Google Pixel(Android 9), Samsung Galaxy S8(Android 8.0);
Build: Nightly 64.0a1 (2018-10-10);

Steps to reproduce:
1. Open Twitter from Gmail using custom tabs;
3. Enter credentials and tap on "Log in".

Expected result:
A Password Doorhanger is displayed on the left side of the URL Bar asking to save the credentials.

Actual result:
The password doorhanger is not displayed.

Comment 1

[Stefan Deiac]

Last good revision: f69ecb2abf86e239c528a27f394e88019bd7cdae
First bad revision: cfa50dc1f61600b08d966210d0319b052ac86627
Pushlog:https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f69ecb2abf86e239c528a27f394e88019bd7cdae&tochange=cfa50dc1f61600b08d966210d0319b052ac86627

Comment 2

[David Durst [:ddurst]]

NI mconley for regression.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey jimchen, I seem to recall you working on login manager stuff for custom tabs... any idea how this revision https://hg.mozilla.org/integration/autoland/rev/bd55b04144dc might have broken the login prompt for custom tabs? I really don't know where to start to debug this.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Nothing from that patch jumps out. TBH another revision [1] that landed around the same time looks more relevant because that patch actually touched mobile code.

[1] https://hg.mozilla.org/mozilla-central/rev/8900229a39b1b2a783d310cd6795a878d62c621a

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks, jchen.

Hey Stefan, can you please confirm the regression range?

cc'ing MattN just in case.

Comment 6

[Stefan Deiac]

Hey Mike, the regression range it's good.
Device: OnePlus 5T (Android 8.1.0).

Comment 7

[Marion Daly [:mdaly]]

Hi Matt, can you take a look at this?

Comment 8

[Matthew N. [:MattN]]

I really would rather not as I warned during review that the change would likely break something outside desktop Firefox so Mike offered to deal with that before landing[1] and I won't be able to do Android development for a month anyways. Also, this is probably much easier for an Android developer to fix. If Password Manager was actually resourced/supported properly them we probably wouldn't be in this situation btw.

Backing out that bug would be best IMO but I have no idea if that's possible anymore.

I suspect there isn't Android test coverage for this which is a problem to be solved by the Android team.

[1] https://mozilla.logbot.info/fx-team/20180927#c15385109

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I'm responsible for this, I'll own it.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

After some doing, I've got a local Fennec build where I'm able to reproduce the issue. Starting investigation...

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Maybe I'm doing this all wrong, but debugging our Custom Tabs implementation is a real pain because I can't actually attach WebIDE to an instance of it.

So I used logcat, and here's the interesting part:

11-02 12:14:54.063  7091  7125 E GeckoConsole: [JavaScript Error: "TypeError: tabbrowser.getBrowserForOuterWindowID is not a function" {file: "resource://gre/modules/LoginManagerParent.jsm" line: 306}]

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1498233 - Stub out getBrowserForOuterWindowID for GeckoViewTab. r?jchen

GeckoViewTab stubs out gBrowser on the root window for WebExtension compatibility.
LoginManagerParent also looks for gBrowser on windows that are handling username
and password form fills, and expects it to implement getBrowserForOuterWindowID.

This patch makes the stub implement getBrowserForOuterWindowID, which just returns
the lone GeckoViewTab browser.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fafb01078db
Stub out getBrowserForOuterWindowID for GeckoViewTab. r=jchen

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1fafb01078db

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9022250 [details]
Bug 1498233 - Stub out getBrowserForOuterWindowID for GeckoViewTab. r?jchen

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 492950

User impact if declined: Users that have Firefox for Android set as their default browser application won't get "Save Password" dialogs when in Custom Tab mode.

This also probably impacts GeckoView users.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): GeckoView needed to implement a method that LoginManagerParent is now relying on. LoginManagerParent assumed anything implementing a gBrowser-like interface would implement this function.

String changes made/needed: None.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9022250 [details]
Bug 1498233 - Stub out getBrowserForOuterWindowID for GeckoViewTab. r?jchen

custom tabs regression fix, approved for 64.0b9

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/410f4e7ae2fe

Comment 18

[Stefan Deiac]

Verified as fixed in the latest version of Nightly 65.0a1 (2018-11-09) on Nokia 6 (Android 7.1.1) and OnePlus 5T (Android 8.1.0).

Comment 19

[miralobontiu]

Verified as fixed on 64.0b9 on Sony Xperia Z5 (Android 6.0.1).

Thank you!