Make ChromePasswordManagerClient::BindCredentialManager handle missing client gracefully

Make ChromePasswordManagerClient::BindCredentialManager handle missing client gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH) instances. During that time, the WebContents associated with such RFH also may or may not get some tab helpers attached. The order of attaching the tab helpers and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not specified.

The binding methods are often implemented by the tab helpers, as is the case of ChromePasswordManagerClient in particular. The binding methods are static, but might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be called before the tab helpers are added, there is no way ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether the tab helper will be available once the Mojo interface will be interacted with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs to be able to handle its absence gracefully. This is the case for, e.g., ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612
Committed: https://crrev.com/316b880c55452eb694a27ba4d1aa9e74ec9ef342
Cr-Commit-Position: refs/heads/master@{#421519}

[vabr (Chromium), 2016-09-15 18:18:44]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 18:19:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-09-15 18:21:20]

vabr@chromium.org changed reviewers:
+ rockot@chromium.org

[vabr (Chromium), 2016-09-15 18:21:21]

Hi Ken,

This is what I suggested in #9. Could you please take a look?

Cheers,
Vaclav

[vabr (Chromium), 2016-09-15 18:22:08]

On 2016/09/15 18:21:21, vabr (Chromium) wrote:
> Hi Ken,
> 
> This is what I suggested in #9. Could you please take a look?
> 
> Cheers,
> Vaclav

I forgot to mention that in addition to the test, I also verified manually, that
it fixes the reported crash reproduced as described on the bug.

[Ken Rockot(use gerrit already), 2016-09-15 18:24:22]

LGTM as non-owner

[vabr (Chromium), 2016-09-15 18:37:25]

On 2016/09/15 18:24:22, Ken Rockot wrote:
> LGTM as non-owner

Thanks!

Both files are owned by *, so your approval is enough.
I will proceed with landing.

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-09-15 18:37:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 18:37:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vabr (Chromium), 2016-09-15 18:38:24]

On 2016/09/15 18:37:25, vabr (Chromium) wrote:
> I will proceed with landing.
...after I fix the Win compile issue with coercing pointers to Booleans.

[vabr (Chromium), 2016-09-15 18:39:49]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-09-15 18:39:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2344943002/#ps20001
(title: "Fix Win compile")

[commit-bot: I haz the power, 2016-09-15 18:40:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 19:09:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 19:10:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vabr (Chromium), 2016-09-15 19:29:22]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 19:29:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-09-15 19:40:04]

Sorry to bother you again, Ken.

I had to keep binding
ContentPasswordManagerDriverFactory::BindPasswordManagerDriver even if
ChromePasswordManagerClient is not available at the moment, because otherwise a
ton of tests were failing.

ContentPasswordManagerDriverFactory::BindPasswordManagerDriver handles missing
ContentPasswordManagerDriverFactory gracefully, so this won't cause any crashes.

However, it seems strange that ContentPasswordManagerDriverFactory would be
present and ChromePasswordManagerClient would not. The most plausible
explanation seems to be that both are available, but some time after
RegisterRenderFrameMojoInterfaces is called.

In other words, with my patch I now inserted the assumption that
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is always run
after TabHelpers::AttachTabHelpers. This seems to be the case when I run Chrome
manually, but not in the tests.

So patch 3 removes this assumption from the case of
ContentPasswordManagerDriverFactory. I wonder if the right thing to do is to
drop this assumption completely and let also
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces handle the missing
ChromeContentBrowserClient gracefully, the same way
ContentPasswordManagerDriverFactory and ChromeTranslateClient do.

To me the latter seems preferred, what do you think?

Cheers,
Vaclav

P.S. No need to rush with a reply, I'll be OOO for about a week.

[commit-bot: I haz the power, 2016-09-15 20:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 20:10:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2016-09-15 21:04:22]

On 2016/09/15 19:40:04, vabr (OOO until 27 Sep) wrote:
> Sorry to bother you again, Ken.
> 
> I had to keep binding
> ContentPasswordManagerDriverFactory::BindPasswordManagerDriver even if
> ChromePasswordManagerClient is not available at the moment, because otherwise
a
> ton of tests were failing.
> 
> ContentPasswordManagerDriverFactory::BindPasswordManagerDriver handles missing
> ContentPasswordManagerDriverFactory gracefully, so this won't cause any
crashes.
> 
> However, it seems strange that ContentPasswordManagerDriverFactory would be
> present and ChromePasswordManagerClient would not. The most plausible
> explanation seems to be that both are available, but some time after
> RegisterRenderFrameMojoInterfaces is called.
> 
> In other words, with my patch I now inserted the assumption that
> ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is always run
> after TabHelpers::AttachTabHelpers. This seems to be the case when I run
Chrome
> manually, but not in the tests.
> 
> So patch 3 removes this assumption from the case of
> ContentPasswordManagerDriverFactory. I wonder if the right thing to do is to
> drop this assumption completely and let also
> ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces handle the
missing
> ChromeContentBrowserClient gracefully, the same way
> ContentPasswordManagerDriverFactory and ChromeTranslateClient do.
> 
> To me the latter seems preferred, what do you think?
> 
> Cheers,
> Vaclav
> 
> P.S. No need to rush with a reply, I'll be OOO for about a week.

The failing CredentialManagerBrowserTest instances are telling me that handling
the absent ChromeContentBrowserClient inside
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is likely the only
option I have, actually. I'll update the CL with that solution and ping you for
review once that is done.

[vabr (Chromium), 2016-09-28 10:55:30]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 10:55:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-09-28 11:04:29]

Description was changed from

==========
Do not bind password manager Mojo interfaces if ChromePasswordManagerClient is
absent

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces should not bind
interfaces implemented for password manager, if the central class,
ChromePasswordManagerClient, is absent for the current WebContents. This is,
e.g., the case of WebContents associated to a <webview>. In those situations,
it is expected that password manager and Credential Manager API is not
available.

BUG=644612
==========

to

==========
Make ChromePasswordManagerClient::BindCredentialManager handle missing client
gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding
methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH)
instances. During that time, the WebContents associated with such RFH also may
or may not get some tab helpers attached. The order of attaching the tab helpers
and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not
specified.

The binding methods are often implemented by the tab helpers, as is the case of
ChromePasswordManagerClient in particular. The binding methods are static, but
might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be
called before the tab helpers are added, there is no way
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether
the tab helper will be available once the Mojo interface will be interacted
with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces
must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs
to be able to handle its absence gracefully. This is the case for, e.g.,
ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the
case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612
==========

[commit-bot: I haz the power, 2016-09-28 11:40:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 11:40:42]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-09-28 12:22:41]

Hello Ken,

I did the code changes and modified the CL description accordingly. Could you
please review again?

Thanks!
Vaclav

[Ken Rockot(use gerrit already), 2016-09-28 14:31:52]

lgtm

[vabr (Chromium), 2016-09-28 14:35:33]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-09-28 14:35:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 14:40:43]

Description was changed from

==========
Make ChromePasswordManagerClient::BindCredentialManager handle missing client
gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding
methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH)
instances. During that time, the WebContents associated with such RFH also may
or may not get some tab helpers attached. The order of attaching the tab helpers
and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not
specified.

The binding methods are often implemented by the tab helpers, as is the case of
ChromePasswordManagerClient in particular. The binding methods are static, but
might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be
called before the tab helpers are added, there is no way
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether
the tab helper will be available once the Mojo interface will be interacted
with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces
must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs
to be able to handle its absence gracefully. This is the case for, e.g.,
ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the
case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612
==========

to

==========
Make ChromePasswordManagerClient::BindCredentialManager handle missing client
gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding
methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH)
instances. During that time, the WebContents associated with such RFH also may
or may not get some tab helpers attached. The order of attaching the tab helpers
and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not
specified.

The binding methods are often implemented by the tab helpers, as is the case of
ChromePasswordManagerClient in particular. The binding methods are static, but
might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be
called before the tab helpers are added, there is no way
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether
the tab helper will be available once the Mojo interface will be interacted
with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces
must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs
to be able to handle its absence gracefully. This is the case for, e.g.,
ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the
case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612
==========

[commit-bot: I haz the power, 2016-09-28 14:40:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-28 14:43:05]

Description was changed from

==========
Make ChromePasswordManagerClient::BindCredentialManager handle missing client
gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding
methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH)
instances. During that time, the WebContents associated with such RFH also may
or may not get some tab helpers attached. The order of attaching the tab helpers
and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not
specified.

The binding methods are often implemented by the tab helpers, as is the case of
ChromePasswordManagerClient in particular. The binding methods are static, but
might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be
called before the tab helpers are added, there is no way
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether
the tab helper will be available once the Mojo interface will be interacted
with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces
must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs
to be able to handle its absence gracefully. This is the case for, e.g.,
ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the
case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612
==========

to

==========
Make ChromePasswordManagerClient::BindCredentialManager handle missing client
gracefully

ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces associates binding
methods with Mojo interfaces. It does so for newly created RenderFrameHost (RFH)
instances. During that time, the WebContents associated with such RFH also may
or may not get some tab helpers attached. The order of attaching the tab helpers
and calling ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces is not
specified.

The binding methods are often implemented by the tab helpers, as is the case of
ChromePasswordManagerClient in particular. The binding methods are static, but
might attempt to call non-static methods on the tab helpers.

Because ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces might be
called before the tab helpers are added, there is no way
ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces could tell whether
the tab helper will be available once the Mojo interface will be interacted
with. Therefore ChromeContentBrowserClient::RegisterRenderFrameMojoInterfaces
must always associate the binding method with the interface.

In the cases where the tab helper does not get created, the binding method needs
to be able to handle its absence gracefully. This is the case for, e.g.,
ChromeTranslateClient or ContentPasswordManagerDriverFactory. It was not the
case for ChromePasswordManagerClient, which caused user-reproducible errors.

This CL adds the graceful handling to ChromePasswordManagerClient.

BUG=644612

Committed: https://crrev.com/316b880c55452eb694a27ba4d1aa9e74ec9ef342
Cr-Commit-Position: refs/heads/master@{#421519}
==========

[commit-bot: I haz the power, 2016-09-28 14:43:06]

Patchset 4 (id:??) landed as
https://crrev.com/316b880c55452eb694a27ba4d1aa9e74ec9ef342
Cr-Commit-Position: refs/heads/master@{#421519}