PasswordAutofillAgent: do not provisionally save forms in NULL frames

PasswordAutofillAgent: do not provisionally save forms in NULL frames

Based on a recent crash reports, there is a suspicion that PasswordAutofillAgent::provisionally_saved_forms_ gets NULL WebFrame pointers assigned as keys. That's likely happening in TextDidChangeInTextField(), when ProvisionallySavePassword() is called with element.document().frame(). That can be NULL, as the comment at blink::Document::frame() says.

This CL makes sure that forms for which we don't know the frame are not provisionally saved.

It is still unclear to me, how to reproduce this crash, so I have no idea how to reproduce it in tests. Therefore this CL does not add tests. There is a bug (400186) filed for making PasswordAutofillAgent observe frames directly, which should ultimately clean up this mess anyway.

BUG=420519
Committed: https://crrev.com/ecf42baef5acafb27ab773f0b6d129b48d47f4a8
Cr-Commit-Position: refs/heads/master@{#299443}

[vabr (Chromium), 2014-10-10 10:05:27]

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

[vabr (Chromium), 2014-10-10 10:05:27]

Hi Garrett,

PTAL. Note that although there is a DCHECK in PasswordAutofillAgent against the
NULL frame, this troubles the release builds run by the users.

Cheers,
Vaclav

[Mike West, 2014-10-13 13:12:59]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-13 13:13:00]

LGTM % questions.

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:537: if
(element_frame) {
You can put the whole assignment into the 'if'.

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:542:
PasswordToLoginMap::iterator iter = password_to_username_.find(element);
Do we need to do the rest of this if we're not in a Frame?

[vabr (Chromium), 2014-10-13 13:53:17]

Thanks, Mike.
Comment addressed, question answered.

Garrett,
Please have a look if you can.
Given that this was approved by Mike, is rather a simple change, and I'd like to
speed up the crasher investigation (it's stable release 40 blocking), I might
consider TBR-ing you tonight. If I do and you don't approve of that, please feel
free to object and revert, and I won't do such TBRs in the future. :)

Cheers,
Vaclav

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:537: if
(element_frame) {
On 2014/10/13 13:13:00, Mike West wrote:
> You can put the whole assignment into the 'if'.

Done.

https://codereview.chromium.org/649583003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:542:
PasswordToLoginMap::iterator iter = password_to_username_.find(element);
On 2014/10/13 13:13:00, Mike West wrote:
> Do we need to do the rest of this if we're not in a Frame?

The second part is there for cases, where the user edits a password, and Chrome
remembers that in order not to overwrite it by autofilling. That is independent
of the attempt to save the typed password for later. Therefore I'm keeping the
code to execute here.

[Garrett Casto, 2014-10-13 18:24:58]

LGTM

If you wanted to TBR this, I'd be okay with it given the review from another
committer. I'm also fine adding you to the OWNERS file for this directory if you
would like.

[vabr (Chromium), 2014-10-14 07:15:25]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-10-14 07:15:46]

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

[commit-bot: I haz the power, 2014-10-14 07:27:05]

Committed patchset #2 (id:160001)

[commit-bot: I haz the power, 2014-10-14 08:15:17]

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