Notify SSLManager when all password fields on a page are gone

Notify SSLManager when all password fields on a page are gone

For HTTP-bad phase 1 (https://crbug.com/647754), a "Not secure" warning
in the omnibox should show up when a password field is visible on an
HTTP page, and disappear when all password fields are gone.

This CL adds a WebContentsObserver to ContentPasswordManagerDriver which
keeps track of frames in the current page that have visible password
fields. When all frames' password fields have been hidden, the observer
notifies the WebContents, which notifies the SSLManager, which removes
the relevant flag from the SSLStatus.

This is based on top of https://codereview.chromium.org/2468833002/,
which sends a Mojo IPC from Blink whenever a frame's password fields are
all hidden.

BUG=658764
Committed: https://crrev.com/fae6b587d9fc7a0f04dad715173aaadcc31968de
Cr-Commit-Position: refs/heads/master@{#429793}

[estark, 2016-11-01 22:04:51]

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

[commit-bot: I haz the power, 2016-11-01 22:05:39]

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-01 23:02:49]

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

[commit-bot: I haz the power, 2016-11-01 23:02:50]

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

[estark, 2016-11-02 23:46:59]

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

[estark, 2016-11-02 23:47:00]

dcheng, could you please take a look at VisiblePasswordObserver in
content_password_manager_driver.cc? Hoping to get some feedback on the approach
there before sending on to OWNERs.

The VisiblePasswordObserver keeps track of which frames are showing visible
password inputs, and notifies the WebContents when there are no frames that are
showing them. My thinking is that on normal navigations, Blink will send
notifications that password fields are going away... unless the renderer
crashes, in which case we need to listen for RenderFrameDeleted to know that
password fields have gone away. Does that seem reasonable?

[dcheng, 2016-11-03 05:33:01]

On 2016/11/02 23:47:00, estark wrote:
> dcheng, could you please take a look at VisiblePasswordObserver in
> content_password_manager_driver.cc? Hoping to get some feedback on the
approach
> there before sending on to OWNERs.
> 
> The VisiblePasswordObserver keeps track of which frames are showing visible
> password inputs, and notifies the WebContents when there are no frames that
are
> showing them. My thinking is that on normal navigations, Blink will send
> notifications that password fields are going away... unless the renderer
> crashes, in which case we need to listen for RenderFrameDeleted to know that
> password fields have gone away. Does that seem reasonable?

This seems reasonable to me. In theory, we could even suppress the notifications
from Blink on navigations (it does seem kind of redundant, since we get a
separate parallel IPC that the frame navigated), but I'm not sure it's worth the
complexity in Blink.

Out of curiosity, do we have numbers that illustrate it's important to be able
to show and hide the Not Secure banner?

[dcheng, 2016-11-03 05:33:34]

(Also publishing drafts, oops)

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:50:
public base::SupportsUserData::Data {
I'd recommend using WebContentsUserData, which helps abstract out some of this
stuff.

[vabr (Chromium), 2016-11-03 14:57:44]

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

[vabr (Chromium), 2016-11-03 14:57:44]

Hi estark@,

I know you did not ask for password manager review yet, but below I have a
question, and depending on your answer, the code might change quite a bit, so
better asking early. :)

Cheers,
Vaclav

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:40:
// RenderFrameHosts in a WebContents. There is one
Could this WebContentsObserver be simply merged with
ContentPasswordManagerDriverFactory?

The code structure right now is that ContentPasswordManagerDriverFactory
observes a WebContent and records all frames within it, keeping one
ContentPasswordManagerDriver per frame. It seems natural to enhance
ContentPasswordManagerDriverFactory to also keep information about the number of
password forms in each frame.

The current CL duplicates this for every frame within one WebContents.

[estark, 2016-11-03 15:13:35]

vabr: thanks for taking an early look! Response inline.

dcheng:

> This seems reasonable to me. In theory, we could even suppress the
notifications
> from Blink on navigations (it does seem kind of redundant, since we get a
> separate parallel IPC that the frame navigated), but I'm not sure it's worth
the
> complexity in Blink.

That seems like a reasonable possibility to explore, but I agree about the
complexity and, per my answer below, I think it's not worth doing quite yet
until we're sure we're going to launch this dynamic-hiding behavior.

> Out of curiosity, do we have numbers that illustrate it's important to be able
> to show and hide the Not Secure banner?

No, not yet, but we hope to. As part of going through launch review we'll be
gathering data on how the "Not secure" chip behaves on the top N sites. And also
we want to have the dynamic-hiding in particular worked out for UI review to
play with; it's largely a question of whether it feels weird on, e.g.,
nytimes.com for the "Not secure" chip to show up when you open the login form
but not to disappear when you close the login form.

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:40:
// RenderFrameHosts in a WebContents. There is one
On 2016/11/03 14:57:44, vabr (Chromium) wrote:
> Could this WebContentsObserver be simply merged with
> ContentPasswordManagerDriverFactory?
> 
> The code structure right now is that ContentPasswordManagerDriverFactory
> observes a WebContent and records all frames within it, keeping one
> ContentPasswordManagerDriver per frame. It seems natural to enhance
> ContentPasswordManagerDriverFactory to also keep information about the number
of
> password forms in each frame.
> 
> The current CL duplicates this for every frame within one WebContents.

I did think about putting this logic in ContentPasswordManagerDriverFactory, but
I thought it would be a bit odd. I figured that counting password fields has
nothing to do with ContentPasswordManagerDriverFactory's job (which is to
produce ContentPasswordManagerDrivers) and I would be sticking it there simply
because its lifetime was the lifetime that I wanted for my object. So to me it
felt like a bit of an abuse of ContentPasswordManagerDriverFactory to put this
logic there. However, if you'd prefer it there, I'm happy to do that instead.

(Note that the VisiblePasswordObserver is not duplicated for every frame within
a WebContents; there only exists one per WebContents and each frame gets or
creates it as necessary.)

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:50:
public base::SupportsUserData::Data {
On 2016/11/03 05:33:34, dcheng wrote:
> I'd recommend using WebContentsUserData, which helps abstract out some of this
> stuff.

Noted, will do once I work out with vabr (above) where this code should live.

[vabr (Chromium), 2016-11-03 15:19:59]

Thanks for the quick answer!

My comment below reduced to just a suggestion to consider changing the new
code's location, so I'll also have a deeper review, to hopefully spare you one
more day of waiting for it. Will update this soon.

Cheers,
Vaclav

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:40:
// RenderFrameHosts in a WebContents. There is one
On 2016/11/03 15:13:35, estark wrote:
> On 2016/11/03 14:57:44, vabr (Chromium) wrote:
> > Could this WebContentsObserver be simply merged with
> > ContentPasswordManagerDriverFactory?
> > 
> > The code structure right now is that ContentPasswordManagerDriverFactory
> > observes a WebContent and records all frames within it, keeping one
> > ContentPasswordManagerDriver per frame. It seems natural to enhance
> > ContentPasswordManagerDriverFactory to also keep information about the
number
> of
> > password forms in each frame.
> > 
> > The current CL duplicates this for every frame within one WebContents.
> 
> I did think about putting this logic in ContentPasswordManagerDriverFactory,
but
> I thought it would be a bit odd. I figured that counting password fields has
> nothing to do with ContentPasswordManagerDriverFactory's job (which is to
> produce ContentPasswordManagerDrivers) and I would be sticking it there simply
> because its lifetime was the lifetime that I wanted for my object. So to me it
> felt like a bit of an abuse of ContentPasswordManagerDriverFactory to put this
> logic there. However, if you'd prefer it there, I'm happy to do that instead.
> 
> (Note that the VisiblePasswordObserver is not duplicated for every frame
within
> a WebContents; there only exists one per WebContents and each frame gets or
> creates it as necessary.)


Good point about keeping the factory focused on just creating an owning the
frames. And also you are right about the duplication not happening, I missed
that.

Still, it seems a bit confusing to have this crammed into the same file as the
driver. What about putting it in its own file?

[vabr (Chromium), 2016-11-03 15:29:42]

This is great, LGTM!

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:54:
VisiblePasswordObserver* observer =
In addition to moving this out to its own .h/.cc file, please also move the
method definitions out of line (i.e., declarations in .h, definitions in .cc).

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:101:
if (!content::IsOriginSecure(web_contents_->GetVisibleURL())) {
Just a question: the status of the UI indication depends also on the URL. Yet,
this code does not listen to changes in there (i.e., in navigations). I suppose
that is OK, because each such change triggers one of the RenderFrameHas...
notifications anyway?

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:219:
std::unique_ptr<ContentPasswordManagerDriver> subframe_driver(
optional: Could you do
auto subframe_driver = base::MakeUnique(subframe, ...);
?

(Also below.)

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:241:
// when there is a password field both the main frame and a subframe.
typo?
both the main -> both in the main

[estark, 2016-11-03 18:51:42]

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

[commit-bot: I haz the power, 2016-11-03 18:53:03]

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

[estark, 2016-11-03 18:54:13]

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:54:
VisiblePasswordObserver* observer =
On 2016/11/03 15:29:41, vabr (Chromium) wrote:
> In addition to moving this out to its own .h/.cc file, please also move the
> method definitions out of line (i.e., declarations in .h, definitions in .cc).

Done.

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:101:
if (!content::IsOriginSecure(web_contents_->GetVisibleURL())) {
On 2016/11/03 15:29:41, vabr (Chromium) wrote:
> Just a question: the status of the UI indication depends also on the URL. Yet,
> this code does not listen to changes in there (i.e., in navigations). I
suppose
> that is OK, because each such change triggers one of the RenderFrameHas...
> notifications anyway?

That's right. On navigation, we get messages from the renderer process that its
password fields are hidden as the Document detaches. It's only if the renderer
process has crashed that we wouldn't get the expected notifications on
navigation. I'll add a comment somewhere explaining this.

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver_unittest.cc
(right):

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:219:
std::unique_ptr<ContentPasswordManagerDriver> subframe_driver(
On 2016/11/03 15:29:41, vabr (Chromium) wrote:
> optional: Could you do
> auto subframe_driver = base::MakeUnique(subframe, ...);
> ?
> 
> (Also below.)

Done.

https://codereview.chromium.org/2467773002/diff/40001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver_unittest.cc:241:
// when there is a password field both the main frame and a subframe.
On 2016/11/03 15:29:42, vabr (Chromium) wrote:
> typo?
> both the main -> both in the main

Done.

[estark, 2016-11-03 18:54:52]

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

[estark, 2016-11-03 18:54:52]

jam, can you please review the content/ half of this? Thanks!

[commit-bot: I haz the power, 2016-11-03 19:54:52]

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

[commit-bot: I haz the power, 2016-11-03 19:54:53]

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_...)

[jam, 2016-11-04 00:44:40]

lgtm

[estark, 2016-11-04 00:46:32]

Thanks everyone!

dcheng: I'm going to go ahead and land this because it'll be useful for UI
review to have this in Canary ASAP, but please let me know if you have any more
comments -- happy to address in a follow-up CL.

[estark, 2016-11-04 00:47:19]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-04 00:47:19]

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/2467773002/#ps80001
(title: "tweak SSLManager comment")

[commit-bot: I haz the power, 2016-11-04 00:48:33]

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-04 05:20:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-04 05:24:26]

Description was changed from

==========
Notify SSLManager when all password fields on a page are gone

For HTTP-bad phase 1 (https://crbug.com/647754), a "Not secure" warning
in the omnibox should show up when a password field is visible on an
HTTP page, and disappear when all password fields are gone.

This CL adds a WebContentsObserver to ContentPasswordManagerDriver which
keeps track of frames in the current page that have visible password
fields. When all frames' password fields have been hidden, the observer
notifies the WebContents, which notifies the SSLManager, which removes
the relevant flag from the SSLStatus.

This is based on top of https://codereview.chromium.org/2468833002/,
which sends a Mojo IPC from Blink whenever a frame's password fields are
all hidden.

BUG=658764
==========

to

==========
Notify SSLManager when all password fields on a page are gone

For HTTP-bad phase 1 (https://crbug.com/647754), a "Not secure" warning
in the omnibox should show up when a password field is visible on an
HTTP page, and disappear when all password fields are gone.

This CL adds a WebContentsObserver to ContentPasswordManagerDriver which
keeps track of frames in the current page that have visible password
fields. When all frames' password fields have been hidden, the observer
notifies the WebContents, which notifies the SSLManager, which removes
the relevant flag from the SSLStatus.

This is based on top of https://codereview.chromium.org/2468833002/,
which sends a Mojo IPC from Blink whenever a frame's password fields are
all hidden.

BUG=658764

Committed: https://crrev.com/fae6b587d9fc7a0f04dad715173aaadcc31968de
Cr-Commit-Position: refs/heads/master@{#429793}
==========

[commit-bot: I haz the power, 2016-11-04 05:24:28]

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