Track visible password fields by RenderFrameHost, not frame tree node

Track visible password fields by RenderFrameHost, not frame tree node

Previously, VisiblePasswordObserver was tracking each frame tree node
that shows or removes password fields. However, the same frame tree node
can be associated with different RenderFrameHosts over time: see the
comment on RenderFrameHost::GetFrameTreeNodeId. This means that we could
receive a message that a RenderFrameHost corresponding to frame tree
node X was deleted, and we'd treat that as if frame tree node X no
longer has visible password fields. But frame tree node X could have
been transferred to a different process during a navigation, meaning
that there might be a different RenderFrameHost for frame tree node X
that does have a visible password field. In this case we would
incorrectly remove the "Not secure" warning when the original
RenderFrameHost is deleted.

This CL tracks password fields by RenderFrameHost instead of by frame
tree node id, so that we don't confuse messages from different RFHs
corresponding to the same frame tree node during cross-process
navigations.

BUG=664674
TEST=With the #mark-non-secure-as flag set to "Display a verbose state
when password or credit card fields are detected on an HTTP page",
navigate to http://nytimes.com, then to
http://http-password.badssl.com. Ensure that a "Not secure" warning
shows up in the omnibox on the latter page and does not
disappear. Repeat several times since the reproduction is flaky.

Committed: https://crrev.com/31558028cf555b5ff9a29917b206c52ac8faee20
Cr-Commit-Position: refs/heads/master@{#432167}

[estark, 2016-11-14 23:30:17]

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

[commit-bot: I haz the power, 2016-11-14 23:31:31]

Dry run: 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-11-15 01:03:54]

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

[commit-bot: I haz the power, 2016-11-15 01:03:55]

Dry run: This issue passed the CQ dry run.

[estark, 2016-11-15 01:05:53]

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

[estark, 2016-11-15 01:05:53]

vabr, PTAL? I tried to explain in the CL description, but the gist of it is that
when we track password fields by frame tree node, we get mixed up during
cross-process navigations, when a frame tree node gets transferred from one
RenderFrameHost to another. I think tracking password fields by RenderFrameHost
resolves this.

Thanks!

[estark, 2016-11-15 01:08:46]

estark@chromium.org changed reviewers:
+ engedy@chromium.org
- vabr@chromium.org

[estark, 2016-11-15 01:08:46]

Oops, vabr is OOO so I'm switching over to engedy instead.

engedy: vabr has a bit more context about this feature so please let me know if
you have any questions I can answer to provide more context/background. Thanks!

[vabr (Chromium), 2016-11-15 08:00:21]

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

[vabr (Chromium), 2016-11-15 08:00:22]

LGTM, this is a canonical CL: clear code, proper test and a helpful CL
description!

Cheers,
Vaclav

https://codereview.chromium.org/2500213002/diff/1/components/password_manager...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/2500213002/diff/1/components/password_manager...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:352:
std::unique_ptr<ContentPasswordManagerDriver> driver(
nit: auto driver = base::MakeUnique(main_rfh()...)
avoids duplicating the (long) type name.

[estark, 2016-11-15 08:55:39]

On 2016/11/15 08:00:22, vabr (travelling until 21 Nov) wrote:
> LGTM, this is a canonical CL: clear code, proper test and a helpful CL
> description!

Woohoo! Thanks! :)

> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2500213002/diff/1/components/password_manager...
> File
>
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2500213002/diff/1/components/password_manager...
>
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:352:
> std::unique_ptr<ContentPasswordManagerDriver> driver(
> nit: auto driver = base::MakeUnique(main_rfh()...)
> avoids duplicating the (long) type name.

Done.

[estark, 2016-11-15 08:56:07]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-15 08:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2500213002/#ps20001
(title: "vabr comment")

[commit-bot: I haz the power, 2016-11-15 08:56:25]

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-11-15 10:15:59]

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

[commit-bot: I haz the power, 2016-11-15 10:16:00]

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

[engedy, 2016-11-15 10:18:06]

FWIW, LGTM. :-)

[estark, 2016-11-15 11:53:21]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-15 11:53:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2500213002/#ps40001
(title: "fix unit test memory leak")

[commit-bot: I haz the power, 2016-11-15 11:53:30]

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-11-15 12:33:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-15 12:35:13]

Description was changed from

==========
Track visible password fields by RenderFrameHost, not frame tree node

Previously, VisiblePasswordObserver was tracking each frame tree node
that shows or removes password fields. However, the same frame tree node
can be associated with different RenderFrameHosts over time: see the
comment on RenderFrameHost::GetFrameTreeNodeId. This means that we could
receive a message that a RenderFrameHost corresponding to frame tree
node X was deleted, and we'd treat that as if frame tree node X no
longer has visible password fields. But frame tree node X could have
been transferred to a different process during a navigation, meaning
that there might be a different RenderFrameHost for frame tree node X
that does have a visible password field. In this case we would
incorrectly remove the "Not secure" warning when the original
RenderFrameHost is deleted.

This CL tracks password fields by RenderFrameHost instead of by frame
tree node id, so that we don't confuse messages from different RFHs
corresponding to the same frame tree node during cross-process
navigations.

BUG=664674
TEST=With the #mark-non-secure-as flag set to "Display a verbose state
when password or credit card fields are detected on an HTTP page",
navigate to http://nytimes.com, then to
http://http-password.badssl.com. Ensure that a "Not secure" warning
shows up in the omnibox on the latter page and does not
disappear. Repeat several times since the reproduction is flaky.
==========

to

==========
Track visible password fields by RenderFrameHost, not frame tree node

Previously, VisiblePasswordObserver was tracking each frame tree node
that shows or removes password fields. However, the same frame tree node
can be associated with different RenderFrameHosts over time: see the
comment on RenderFrameHost::GetFrameTreeNodeId. This means that we could
receive a message that a RenderFrameHost corresponding to frame tree
node X was deleted, and we'd treat that as if frame tree node X no
longer has visible password fields. But frame tree node X could have
been transferred to a different process during a navigation, meaning
that there might be a different RenderFrameHost for frame tree node X
that does have a visible password field. In this case we would
incorrectly remove the "Not secure" warning when the original
RenderFrameHost is deleted.

This CL tracks password fields by RenderFrameHost instead of by frame
tree node id, so that we don't confuse messages from different RFHs
corresponding to the same frame tree node during cross-process
navigations.

BUG=664674
TEST=With the #mark-non-secure-as flag set to "Display a verbose state
when password or credit card fields are detected on an HTTP page",
navigate to http://nytimes.com, then to
http://http-password.badssl.com. Ensure that a "Not secure" warning
shows up in the omnibox on the latter page and does not
disappear. Repeat several times since the reproduction is flaky.

Committed: https://crrev.com/31558028cf555b5ff9a29917b206c52ac8faee20
Cr-Commit-Position: refs/heads/master@{#432167}
==========

[commit-bot: I haz the power, 2016-11-15 12:35:14]

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