PasswordFormManager takes WeakPtr<PasswordManagerDriver>

PasswordFormManager takes WeakPtr<PasswordManagerDriver>

PFM instances are owned by PasswordManager, which lives as long as a
WebContents does. Drivers associated to PFMs are bound to frames.
PasswordManager keeps PFMs alive until the main frame navigation,
and even longer (if they carry the provisionally saved password).
This means that a PFM can outlive the associated driver.

On its creation, the PFM calls the PasswordStore to get the stored
credentials. While PFM lives on the UI thread, the store works on
DB thread, and the results are returned asynchronously. The PFM normally
uses the services of the driver to process the obtained results. If
the results come later than the frame and driver were deleted, this leads
to use-after-free. Also, if the frame is already deleted, there is no
harm in skipping the work done by driver on the results, because that
is always bound to operations on the frame, like (auto)filling.

Therefore, this CL makes PFM hold a WeakPtr (instead of a naked pointer)
to the driver, and check before each use its validity.

BUG=439534
Committed: https://crrev.com/8a62de58cfbf91ec21c769723bb54e111bd0dadf
Cr-Commit-Position: refs/heads/master@{#307461}

[vabr (Chromium), 2014-12-08 19:11:30]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2014-12-08 19:12:00]

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

[vabr (Chromium), 2014-12-08 19:12:01]

Hi Evan,

Could you please take a look?

Thanks!
Vaclav

[Evan Stade, 2014-12-08 20:02:50]

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:536: if
(driver_) {
move this up to ~L521 for earlier abort?

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_driver.h:68:
base::WeakPtr<PasswordManagerDriver> GetWeakPtr();
is there a reason not to use SupportsWeakPtr/AsWeakPtr?

[Evan Stade, 2014-12-08 20:03:04]

aside from those two issues, lgtm

[vabr (Chromium), 2014-12-09 12:55:46]

Thanks, Evan!
Both comments addressed, so sending to CQ.

Cheers,
Vaclav

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:536: if
(driver_) {
On 2014/12/08 20:02:50, Evan Stade wrote:
> move this up to ~L521 for earlier abort?

Done.
Skipping setting |manager_action_| is actually an important and wanted
side-effect. Thanks for catching that.

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/786823002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_manager_driver.h:68:
base::WeakPtr<PasswordManagerDriver> GetWeakPtr();
On 2014/12/08 20:02:50, Evan Stade wrote:
> is there a reason not to use SupportsWeakPtr/AsWeakPtr?

Done. Thanks for pointing this out.
There is not really a reason against SupportsWeakPtr. The delay between the
driver (members) destruction and the SWP base desctruction (invalidating the
weak pointers) is not important, because both the destruction and potential use
of the driver happen on the UI thread, and thus should not be interleaved.

[vabr (Chromium), 2014-12-09 12:55:51]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-12-09 12:56:48]

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

[commit-bot: I haz the power, 2014-12-09 13:37:54]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-12-09 13:39:01]

Patchset 3 (id:??) landed as
https://crrev.com/8a62de58cfbf91ec21c769723bb54e111bd0dadf
Cr-Commit-Position: refs/heads/master@{#307461}