Thanks for the report! I could reproduce this with the current Chrome Stable (55.0.2883.91), from Android Google Search App (AGSA) and using https://rsolomakhin.github.io/autofill/. What is even worse, if the Custom Tab gets "upgraded" to a full Chrome tab, passwords are still neither filled not saved. It takes creating a new tab to get password manager working again.

I kept chrome://password-manager-internals open when loading https://rsolomakhin.github.io/autofill/, and the only logs there were consistently:

SSL errors present: false
IsPasswordManagerEnabledForCurrentPage: true

Compared to a successful load of https://rsolomakhin.github.io/autofill/ with password manager working, the initial logs from the renderer about parsing forms were missing, as was the browser-process response about creating PasswordFormManager instances. I am not sure why renderer code would not run in Chrome Custom Tabs.

I was trying to reproduce this with Chrome Canary, but I could not get AGSA to use it through Custom Tabs, it only opened a tab directly in there (with password manager working).


This is a rather bad issue, because it affects all websites, so bumping the priority to 1. Given the broad usage of Custom Tabs by AGSA in particular, having this broken could harm trust in password manager a lot.

Right now I spotted that the original description says "passwords ARE filled". In my case, they don't. Not sure what is the reason for this discrepancy, but it is consistent in my case. I can get filled usernames (on demand), but not passwords.

Looping the Custom Tabs team in, in case they have useful pointers for where to look at.

I was not able to reproduce with both http://1.chromium-test1.appspot.com/testing/psl-matching/login and https://rsolomakhin.github.io/autofill/ on the following devices:
1. Nexus 4 / Android 4.2.1
2. Nexus 5X / Android 6.0.1
3. Pixel XL / 7.1.1

I tried this on both the current Canary (57.0.2964.0) and Stable (55.0.2883.91) and in both cases the Password Manager offered to save the password. Videos showing this behavior are here: http://go/chrome-androidlogs1/6/676814

I used the sample custom tabs client from https://github.com/GoogleChrome/custom-tabs-client to launch the custom tabs.

If you have specific devices / Android versions that this reproduces on, I can give those a try too.

This seems to be a race condition... I have been able to reproduce this a couple of times on my Pixel with Chrome Dev on http://1.chromium-test1.appspot.com/testing/psl-matching/login 

But when I switched to developer mode and attached Android Studio to record a screencast, I was offered to save the password. Now I cannot reproduce it anymore - even when I disable developer mode. :-(

And now I was able to reproduce it again (I searched for the URL http://1.chromium-test1.appspot.com/testing/psl-matching/login in AGSA). I wonder whether this might be related to prerendering.

go/ulkmkb is a recording.

Thank you battre@ ! I was able to reproduce this pretty consistently on a Samsung Galaxy Tab S2 (SM-T815) / Android  6.0.1 with the following steps:
1. With the custom tabs sample client app, "warmup" a custom tab, and then launch http://1.chromium-test1.appspot.com/testing/psl-matching/login 
2. Login to the page with any username or password.

The save password infobar does not appear if "warmup" is called before launching the custom tab. If "mayLaunchUrl" is called, or the custom tab is directly launched, the save password infobar appears.

Bisect Range - https://chromium.googlesource.com/chromium/src/+log/55.0.2851.0..55.0.2852.0?pretty=fuller&n=10000
Bisected to CL - https://chromium.googlesource.com/chromium/src/+/97e8f751bc527212094f6fe65f1d96f709708991

Adding relevant people from the CL, and lizeb@ since this is related to prerendering. Also marking as a RBS for M56.

Investigated around code logic of password manager and prerendering, but still can't understand the wholesale logic well, just have some guesses and questions:
  - Maybe this has some relationship with IPC messages order. The bisected CL migrates password manager IPC messages into mojo interfaces, thus they can't keep ordering with legacy IPCs, for example the prerendering IPC PrerenderMsg_SetIsPrerendering(NO_PRERENDER) which seems could change blink webview's visibility. Before that CL, I suppose render frame always receives PrerenderMsg_SetIsPrerendering(NO_PRERENDER) before any other password manager IPCs because all these legacy IPCs are sharing one underlying pipe, but after that CL, this is not always true, some password manager IPCs(mojo call) could be received before PrerenderMsg_SetIsPrerendering(NO_PRERENDER).

  - I'm wondering how the WebContents instance created for prerendering would be when a real url launching happened. Will it be re-used to launch the url?  Maybe current mechanism of setting up password manager mojo interfaces has some problems regarding with prerendering.

re #9: we don't do prerendering on warmup(). Prerendering happens with mayLaunchUrl (subject to throttling though). Warmup creates a spare renderer and afair navigates it to about:blank, so there is still some possibility that visibility-related IPCs are not in sync with Password MAnager Mojo IPCs. I don't know how the latter ones work.

Somewhat irrelevant: vasilii@ landed a patch to kill prerenders when credential manager API is invoked from the page: https://codereview.chromium.org/2447143002

Let me make some explanations about password manager mojo framework firstly.
Browser side:
autofill.mojom.PasswordManagerDriver mojo interface is registered at [1] in ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces().
Render side:
autofill::PasswordAutofillAgent instance will be created alongside with each render frame, and its ctor will try to connect to autofill.mojom.PasswordManagerDriver mentioned above and send the first mojo call at [2].

[1] https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?rcl=0&l=2999
[2] https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_autofill_agent.cc?rcl=0&l=576
[3] https://cs.chromium.org/chromium/src/components/password_manager/content/browser/content_password_manager_driver_factory.cc?rcl=1484022024&l=91

There are 2 possible scenarios could break above mojo connection, and once the connection got broken, can't recover again.
Scenario A:
    Render side tries to connect at above [2], triggers browser side executing ContentPasswordManagerDriverFactory::BindPasswordManagerDriver() to establish the mojo connection, but, found ContentPasswordManagerDriverFactory not ready for current web contents yet at [3], then the mojo connection is rejected.
Scenario B:
    The mojo connection has been established successfully, but, for some reason, browser side PasswordManagerDriver gets destroyed, this will break the mojo connection. Later even the corresponding PasswordManagerDriver gets created again, renderer side won't re-connect and just keeps using the broken mojo connection unaware that it has become broken.

My question is whether there could possibly have some special thing for the warmup web contents to make above scenarios possible to happen? Will the warmup WebContents and the spare render process be re-used for a real url launching later? I'm not familiar with android implementations...

I found that org.chromium.chrome.browser.tab.Tab::setContentViewCore calls native function TabAndroid::InitWebContents, which will create the ContentPasswordManagerDriverFactory instance for the web contents.

Any thoughts about #11? Thanks!

I spoke with yusufo@ on this.

This bug existed in M55 and was marked as a polish bug by the privacy team, so I don't think it's urgent to fix in M56, hence removing RB-Stable.  If we come up with a fix and you'd like to merge feel free to request one, once a fix has been identified.  If you want to make M56 though, you only have until Monday to land the fix on trunk...

sbirch@, please let me know if you disagree with any of the rationale here.

Thanks all for the investigations so far.

@pasko -- reading that "warmup" causes the password manager failure (#8) and that "warmup" does not cause pre-rendering (#10), could there be anything else caused by "warmup" which results in one of the situations described in #11?

@amineer -- we use Hotlist-Polish, perhaps mistakenly, to mean "user-facing" bugs, as opposed to unimportant ones. We figured out that if polish bugs are the target of Fixits, that's a better interpretation. Still, I don't think this is severe enough to warrant a RBS. We are apparently not making this in 56 (also, I'm on leave until mid Feb).

Managed to reproduce failures. I tried to investigate saving. 

We try to send the submitted from to the driver (
GetPasswordManagerDriver()->PasswordFormSubmitted(*submitted_form);, https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_autofill_agent.cc?rcl=ea5098bb3372baa8d32b117f1b687e739e075988&l=1160), but the driver doesn't received the message. 

Seems like the pointer to the driver is ok. But the received doesn't accept the message ( receiver_->Accept(builder.message()) returns false (PasswordManagerDriverProxy::PasswordFormSubmitted),
https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/content/common/autofill_driver.mojom.cc?rcl=92db4dac9f97bddf7abe43c69c19cdb213536c05&l=1101)

leon.han@: any thoughts? Thanks.

leon.han@intel.com: As I said before, |receiver_| doesn't accept the message here https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/content/common/autofill_driver.mojom.cc?rcl=92db4dac9f97bddf7abe43c69c19cdb213536c05&l=1101. I didn't manage to understand what class the receiver is (PasswordManagerDriverRequestValidator? PasswordManagerDriverStubDispatch? PasswordManagerDriverStub?). So, I don't know what "Accept" function is called. Could you give me a hint?

The function asked for in #16 is InterfaceEndpointClient::Accept(Message* message). It returns false, because of encountered_error_ is true. 

FYI. 

Links to "warmup" code: 
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java?q=warmupInternal&l=248

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java?q=handleSynchronousStartup&l=120

Added mojo since this bug might affect on other components as well.

Short summary of the problem:
It is reproducible only for Chrome custom tabs when "warmup" (preparing browser to launch). Password manager's mojo messages are not passed between the browser and the rendered. Technically, receiver_->Accept(builder.message()) returns false (https://cs.chromium.org/chromium/src/out/Debug/gen/components/autofill/content/common/autofill_driver.mojom.cc?rcl=92db4dac9f97bddf7abe43c69c19cdb213536c05&l=1101). It happen because of encountered_error_ is true in InterfaceEndpointClient::Accept (https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc?rcl=a008362df6fcd4e9114f534ca38d4bb8dc26f1e8&l=219). The questions is why the error happens.

One more point. The comment after "bool ok = receiver_->Accept(builder.message());" says:
// This return value may be ignored as !ok implies the Connector has
// encountered an error, which will be visible through other means.
How should it be visible? 

leon.han@intel.com: Friendly ping. Will you have a chance to debug and fix this bug? Or shall I ask somebody else? 

Seems like custom tabs are getting more and more popular. Instability of Password Manager reduces user trust. It would be nice to fix that soon.

I could help you with Android debugging.

Added the printing of stack trace to mojo::InterfaceEndpointClient::NotifyError. Output is attached.

Deleted: log_sym.txt 27.6 KB

log_sym.txt 27.6 KB View Download

Hi, kolos@, so sorry for late reply because I'm back from holidays and just saw your comments this morning.

Answer to comment #19:
Calling PasswordManagerDriverPtr::encountered_error() could get whether the underlying connection is broken or not.
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h?l=148&gs=cpp%253Amojo%253A%253Aclass-InterfacePtr%253C%25231%253E%253A%253Aencountered_error()-const%2540chromium%252F..%252F..%252Fmojo%252Fpublic%252Fcpp%252Fbindings%252Finterface_ptr.h%257Cdef&gsn=encountered_error&ct=xref_usages

PasswordManagerDriverPtr::encountered_error() returning false means that the underlying mojo message pipe is broken, and the possible scenarios are described at comment #11:
Scenario A means establishment of the mojo connection failed from scratch, and Scenario B means the mojo connection got established once and then was disconnected later because PasswordManagerDriver impl in browser side gets destroyed.

Either above A or B, once mojo connection got broken, it CAN NOT recover by some methods like re-connect etc.

So this bug's root cause should be that something special in warm-up related code logic triggered such scenarios.

kolos@: I'd like to contact you via email later for help about debugging. Thanks!

Hi, I found one possible code path which could lead to Scenario A:
Custom tab creates the spare web contents, 
    WarmupManager.createSpareWebContents() --> WebContentsFactory.createWebContentsWithWarmRenderer() --> WebContents::Create() -->  WebContentsImpl::CreateWithOpener() --> WebContentsImpl::Init() --> RenderFrameHostManager::InitRenderView()  ===> This will create the renderer process, in which PasswordAutofillAgent ctor will try to connect to PasswordManagerDriver impl in browser side.
But, the point is: At this timing, the ContentPasswordManagerDriverFactory instance corresponding with this web contents has not been created yet, so the message pipe would be closed at https://cs.chromium.org/chromium/src/components/password_manager/content/browser/content_password_manager_driver_factory.cc?rcl=dcc57ec782030dcf6df61f6fcea7c41f0685a516&l=91
The ContentPasswordManagerDriverFactory instance will be created later when Tab.java::initialize() gets actually called.

Perhaps we need to find out a proper way creating ContentPasswordManagerDriverFactory early enough to enable establish PasswordManagerDriver mojo connection?

Sequence for non-custom tab:
  Creates WebContents --> Creates ContentPasswordManagerDriverFactory"Tab.java::initialize()" --> Starts renderer process

This has no problem.

Great thanks Leon for taking look into this! We are unfamiliar with mojo internals and fixing this would take a lot of time.

Here is the information about debugging custom tabs. 

Build chrome_public_apk and custom_tabs_client_example_apk (https://chromium.googlesource.com/chromium/src/+/master/docs/android_build_instructions.md). You might also need Android SDK and ADT Tools (http://developer.android.com/sdk/index.html)

Install chromium and custom tab app to your device or emulator:
adb install -r out/Debug/apks/ChromePublic.apk && adb install -r out/Debug/apks/CustomTabsClientExample.apk
OR
build/android/adb_install_apk.py out/Debug/apks/ChromePublic.apk


I debugged via logging:
// Java code:
import org.chromium.base.Log;
int i = 1;
Log.d("MYDEBUG", "Hello world %d", i);

// Chromium code:
#include "base/logging.h"
int i = 2;
LOG(WARNING) << " MYDEBUG HELLO WORLD " << i;

// Blink code:
#include "wtf/Assertions.h"
int i = 3;
WTF_LOG_ERROR("MYDEBUG LOG BLINK INT: NONE %d", i);

To filter out the mess of messages, listen logcat and filter with grep, for example: "adb logcat | grep "MYDEBUG""

You could also use gdb, but I failed to load debug symbol. Let me know if you managed.
To run gdb:
build/android/adb_gdb --output-directory=out/Debug --package-name=org.chromium.customtabsclient --start --activity=org.chromium.customtabsclient.MainActivity



How to reproduce the failure:
1. Open Chrome Custom Tab Example app.
2. Select org.chromium.chrome package.
3. Connect to the service.
4. Paste http://1.chromium-test1.appspot.com/testing/psl-matching/login to URL field (or change it here before building https://cs.chromium.org/chromium/src/third_party/custom_tabs_client/src/Application/src/main/res/layout/main.xml?rcl=f68fb050a78be781cbaacb7a34a96ca902dda0f3&l=30)
5.  Launch URL in a Chrome custom tab. Password Manager should save and autofill (Sometimes, it doesn't, then try step 5 several times).
6. Come back to app and press "Warmup". 
7.  Launch URL in a Chrome custom tab again. Password manager will not work. (Sometimes it does, then press "Warmup" a couple of times).

In 5&7, I tried to enter random username&password and submit. PasswordAutofillAgent tries to send the submitted password form to the browser (https://cs.chromium.org/chromium/src/components/autofill/content/renderer/password_autofill_agent.cc?rcl=92c26eed9fd8834e1da3283e8bd45952aa4f0d6b&l=1158), but the browser doesn't receive this message.

I am glad to answer questions, if any. 

Some more info about "warmup"
Warmup carries out most of the application level initialization that would normally happen on app launch, but it does it while Chrome is in the background.

- It loads native libraries
- Initializes the browser process synchronously
- Creates a spare renderer and initializes it (because we don't have a url yet, it doesn't navigate)
- Inflates the UI view hierarchy and caches it, so that it can be attached to the window when an Activity is created.

Regards,
Maxim

With above detailed explanations from kolos@, I can reproduce/debug well, big thanks!
And the suspect point at #26 is confirmed: Before a spare WebContents gets its ContentPasswordManagerDriverFactory ready, renderer side PasswordManagerDriverPtr tried to connect, so the connect request was just dropped and renderer side keeps holding an invalid PasswordManagerDriverPtr, thus all PasswordManagerDriver mojo calls just fail in silence.

Tomorrow I'll prepare a fix CL and confirm more for sure, then upload it for review.

Sounds good! Thanks. Looking forward for your CL.

leon.han@intel.com: Could you please send me a link to CL? Thx.

I just uploaded the CL at https://codereview.chromium.org/2680163006/, and have confirmed it does fix this bug, PTAL and also help to double check if possible, Thanks.

Thanks for the link. I will double check soon.

The fix resolved the issue. Thanks to leon.han@

Pri-0 bugs are critical regressions or serious emergencies, and this bug has not been updated in three days. Could you please provide an update, or adjust the priority to a more appropriate level if applicable?

If a fix is in active development, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2364c1b8adb850f0d1b1e29f16ecfd75679280d5

commit 2364c1b8adb850f0d1b1e29f16ecfd75679280d5
Author: leon.han <leon.han@intel.com>
Date: Tue Feb 14 00:30:26 2017

Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG= 676814 
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2680163006
Cr-Commit-Position: refs/heads/master@{#450171}

[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/chrome/renderer/autofill/fake_content_password_manager_driver.cc
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/chrome/renderer/autofill/fake_content_password_manager_driver.h
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/autofill/content/common/autofill_driver.mojom
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/2364c1b8adb850f0d1b1e29f16ecfd75679280d5/components/password_manager/content/browser/content_password_manager_driver_unittest.cc

Hi, do we need to merge the fix to M57 branch? Thanks.

I think it would be great to merge to M57 but I don't know whether it is too late.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cfb799c32b29c6c499636de1303e5fbbbdd92f5

commit 3cfb799c32b29c6c499636de1303e5fbbbdd92f5
Author: Han Leon <leon.han@intel.com>
Date: Tue Feb 14 09:43:09 2017

[M57] Eliminate PasswordAutofillAgentConstructed() in mojo interface PasswordManagerDriver

PasswordAutofillAgentConstructed() is just for autofill::PasswordAutofillAgent to
get logging state from browser side when PasswordAutofillAgent is created.
It's useful before due to lack of pending mechanism for legacy IPCs,
but now in mojo world we can achieve this goal without it.

This CL lets browser side just send logging state as soon as it's ready,
even at that timing point renderer process has not started or has not created
PasswordAutofillAgent, the mojo call
autofill::mojom::PasswordAutofillAgent::SetLoggingState() will be cached
inside mojo message pipe and be executed immediately once PasswordAutofillAgent
instantiates and binds itself with the pending mojo connection.

This keeps exactly the same behavior with before:
autofill::mojom::PasswordAutofillAgent::SetLoggingState() is always the first
mojo call arriving at autofill::PasswordAutofillAgent from browser side.

BUG= 676814 
TEST=components_unittests
TBR=vabr@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2680163006
Cr-Commit-Position: refs/heads/master@{#450171}
(cherry picked from commit 2364c1b8adb850f0d1b1e29f16ecfd75679280d5)

Review-Url: https://codereview.chromium.org/2697803002 .
Cr-Commit-Position: refs/branch-heads/2987@{#496}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/chrome/renderer/autofill/fake_content_password_manager_driver.cc
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/chrome/renderer/autofill/fake_content_password_manager_driver.h
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/autofill/content/common/autofill_driver.mojom
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/3cfb799c32b29c6c499636de1303e5fbbbdd92f5/components/password_manager/content/browser/content_password_manager_driver_unittest.cc

Verified fix in 58.0.3012.0. Thanks!

Verified fix in M57 57.0.2987.54