Remove ContentPasswordManagerDriver::GetAutofillManager

Remove ContentPasswordManagerDriver::GetAutofillManager

Move it to PasswordManagerClient, and make it return the AutofillManager for the main frame.

The bug occurred because the PasswordFormManager outlived the driver. The PasswordFormManager can't outlive the main frame.

BUG=438970
Committed: https://crrev.com/630520b1bc288370479f8ec9445ef8cdf768f647
Cr-Commit-Position: refs/heads/master@{#307100}

[Evan Stade, 2014-12-05 00:46:19]

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

[Evan Stade, 2014-12-05 00:46:19]

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (left):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:783:
driver_->GetAutofillManager();
driver_ can be dead by now. client_ cannot. That's why this fix works. That
said, this isn't the greatest fix ever because holding onto a potentially stale
pointer is a recipe for trouble. But it doesn't seem like the other uses of
|driver_| in this file will lead to trouble currently. Still, probably best to
get rid of it and find a different way to expose AllowPasswordGenerationForForm.
Perhaps by passing the render_frame_host, and then doing a lookup for driver
each time, and handling nullptr?

[vabr (Chromium), 2014-12-05 10:13:09]

Thanks, Evan!

This LGTM, with some comments.

Cheers,
Vaclav

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (left):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:783:
driver_->GetAutofillManager();
On 2014/12/05 00:46:19, Evan Stade wrote:
> driver_ can be dead by now. client_ cannot. That's why this fix works. That
> said, this isn't the greatest fix ever because holding onto a potentially
stale
> pointer is a recipe for trouble. But it doesn't seem like the other uses of
> |driver_| in this file will lead to trouble currently. Still, probably best to
> get rid of it and find a different way to expose
AllowPasswordGenerationForForm.
> Perhaps by passing the render_frame_host, and then doing a lookup for driver
> each time, and handling nullptr?

The plan for removing driver_ sounds good. I filed http://crbug.com/439387 for
that. Feel free to include a TODO(vabr) with the bug link here.

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:159: virtual
autofill::AutofillManager* GetAutofillManager();
optional: I wonder if "main frame" should also appear in the method name. On one
hand, without that the name is nicely short, and if one already knows that there
is just one client per WebContents, then it's rather clear that the returned
AutofillManager is the one for the (only special) main frame.
On the other hand, this is not called that often, so
GetAutofillManagerForMainFrame (or similar) will probably be bearable, and
easier to understand.

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/stub_password_manager_client.h
(right):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/stub_password_manager_client.h:35:
autofill::AutofillManager* GetAutofillManager() override;
Because you already include the "return nullptr" implementation of
GetAutofillManager in the interface itself, there is no need to repeat it in the
stub.

Just FYI: I spoke to dconnelly@, our Bling expert for password manager, and
because the potentially returned nullptr is handled gracefully on the callsite,
he prefers having the nullptr-returning stub in the interface (as you currently
do, and as opposed to the original state when the method was in the driver).

[Evan Stade, 2014-12-05 22:00:57]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-12-05 22:02:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/780183002/20001

[Evan Stade, 2014-12-05 22:38:12]

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_form_manager.cc (left):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:783:
driver_->GetAutofillManager();
On 2014/12/05 10:13:09, vabr (Chromium) wrote:
> On 2014/12/05 00:46:19, Evan Stade wrote:
> > driver_ can be dead by now. client_ cannot. That's why this fix works. That
> > said, this isn't the greatest fix ever because holding onto a potentially
> stale
> > pointer is a recipe for trouble. But it doesn't seem like the other uses of
> > |driver_| in this file will lead to trouble currently. Still, probably best
to
> > get rid of it and find a different way to expose
> AllowPasswordGenerationForForm.
> > Perhaps by passing the render_frame_host, and then doing a lookup for driver
> > each time, and handling nullptr?
> 
> The plan for removing driver_ sounds good. I filed http://crbug.com/439387 for
> that. Feel free to include a TODO(vabr) with the bug link here.

thanks, done

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_manager_client.h:159: virtual
autofill::AutofillManager* GetAutofillManager();
On 2014/12/05 10:13:09, vabr (Chromium) wrote:
> optional: I wonder if "main frame" should also appear in the method name. On
one
> hand, without that the name is nicely short, and if one already knows that
there
> is just one client per WebContents, then it's rather clear that the returned
> AutofillManager is the one for the (only special) main frame.
> On the other hand, this is not called that often, so
> GetAutofillManagerForMainFrame (or similar) will probably be bearable, and
> easier to understand.

Done.

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
File components/password_manager/core/browser/stub_password_manager_client.h
(right):

https://codereview.chromium.org/780183002/diff/1/components/password_manager/...
components/password_manager/core/browser/stub_password_manager_client.h:35:
autofill::AutofillManager* GetAutofillManager() override;
On 2014/12/05 10:13:09, vabr (Chromium) wrote:
> Because you already include the "return nullptr" implementation of
> GetAutofillManager in the interface itself, there is no need to repeat it in
the
> stub.
> 
> Just FYI: I spoke to dconnelly@, our Bling expert for password manager, and
> because the potentially returned nullptr is handled gracefully on the
callsite,
> he prefers having the nullptr-returning stub in the interface (as you
currently
> do, and as opposed to the original state when the method was in the driver).

Done.

[commit-bot: I haz the power, 2014-12-05 23:02:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-05 23:03:43]

Patchset 2 (id:??) landed as
https://crrev.com/630520b1bc288370479f8ec9445ef8cdf768f647
Cr-Commit-Position: refs/heads/master@{#307100}