Count visible password fields on a page

Count visible password fields on a page

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 counts password fields in a frame, incrementing the counter when
a PasswordInputType creates a layout object and decrementing it on the
layout object's willBeDestroyed() method. A Mojo IPC is sent when the
counter is decremented to 0.

A follow-up CL will keep track of the visibility of password fields per
RenderFrameHost and remove the omnibox warning when there are no visible
password fields anywhere in the frame tree.

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

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

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

[commit-bot: I haz the power, 2016-11-01 04:52:04]

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-01 17:53:47]

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

[estark, 2016-11-01 17:53:48]

dcheng, PTAL?

(There's a follow-up CL at https://codereview.chromium.org/2467773002/ to give
you an idea of where I'm going with this, but it's not ready for review yet.)

[estark, 2016-11-01 17:54:42]

Oh, by the way, I stuck the counter on LocalFrame, but I'm not sure whether
that's a good place to stick it or not. Maybe it belongs on Document or
somewhere else entirely?

[dcheng, 2016-11-01 18:12:42]

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:281: uint64_t m_passwordCount;
Yeah, let's put this on Document: this *should* get set to zero when it's
detached... but this way we're sure to avoid consistency issues.

Btw, let's just make this an unsigned: renderers can't alloc more than 2GB
anyway, so getting 2^63-1 password elements seems unlikely.

[dcheng, 2016-11-01 18:13:43]

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:131:
document.frame()->incrementPasswordCount();
Also, for symmetry, can we increment / decrement this inside the custom layout
object?

[estark, 2016-11-01 18:30:15]

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

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

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-01 18:32:26]

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:281: uint64_t m_passwordCount;
On 2016/11/01 18:12:42, dcheng wrote:
> Yeah, let's put this on Document: this *should* get set to zero when it's
> detached... but this way we're sure to avoid consistency issues.

Done.

> 
> Btw, let's just make this an unsigned: renderers can't alloc more than 2GB
> anyway, so getting 2^63-1 password elements seems unlikely.

Done.

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2468833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:131:
document.frame()->incrementPasswordCount();
On 2016/11/01 18:13:43, dcheng wrote:
> Also, for symmetry, can we increment / decrement this inside the custom layout
> object?

Done. (I moved the Mojo IPC in to the layout object too so we just do everything
there.)

Also, to avoid wasting a few IPCs and for slightly more symmetry, I tweaked this
to only send an IPC when we go from 0 to 1 visible password fields, since that's
all the browser process cares about.

[dcheng, 2016-11-01 18:51:36]

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:51:
PasswordLayoutObject(HTMLInputElement* element)
Nit: explicit

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:63: if
(document.passwordCount() > 1 || !document.frame()) {
Is it possible to just DCHECK(document.frame()) here? Creating a layout object
implies that this Document should be attached and active, which implies it
should have a frame.

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:28: if
(!m_mockSensitiveInputVisibilityService || true) {
Hmmm, is this just for testing?

[estark, 2016-11-01 18:59:47]

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp (right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:51:
PasswordLayoutObject(HTMLInputElement* element)
On 2016/11/01 18:51:36, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp:63: if
(document.passwordCount() > 1 || !document.frame()) {
On 2016/11/01 18:51:36, dcheng wrote:
> Is it possible to just DCHECK(document.frame()) here? Creating a layout object
> implies that this Document should be attached and active, which implies it
> should have a frame.

Done.

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:28: if
(!m_mockSensitiveInputVisibilityService || true) {
On 2016/11/01 18:51:36, dcheng wrote:
> Hmmm, is this just for testing?

Omg, that's embarrassing, sorry! Yes, that was for testing. I was debugging this
last night, came in this morning and forgot where I was and just saw that the
tests pass and missed that I had added this. The weird thing is that the tests
fail if I remove this and only create the m_mockSensitiveInputVisibilityService
once: that is,
MockSensitiveInputVisibilityService::AllPasswordFieldsInInsecureContextInvisible
doesn't get called unless I recreate the service on every getInterface() call.
Does that make any sense to you?

[estark, 2016-11-01 19:03:21]

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:28: if
(!m_mockSensitiveInputVisibilityService || true) {
On 2016/11/01 18:59:47, estark wrote:
> On 2016/11/01 18:51:36, dcheng wrote:
> > Hmmm, is this just for testing?
> 
> Omg, that's embarrassing, sorry! Yes, that was for testing. I was debugging
this
> last night, came in this morning and forgot where I was and just saw that the
> tests pass and missed that I had added this. The weird thing is that the tests
> fail if I remove this and only create the
m_mockSensitiveInputVisibilityService
> once: that is,
>
MockSensitiveInputVisibilityService::AllPasswordFieldsInInsecureContextInvisible
> doesn't get called unless I recreate the service on every getInterface() call.
> Does that make any sense to you?

Oh, actually, looking at this again, I guess that does make sense, as |handle|
has to be bound (? not sure if I'm using the right Mojo words) on each call to
getInterface.

I feel like I'm not understanding something Mojo-y here but I'm not sure what it
is. Is there a way that getInterface() should work such that it doesn't create a
new MockSensitiveInputVisibilityService on every call?

[vabr (Chromium), 2016-11-02 11:28:59]

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

[vabr (Chromium), 2016-11-02 11:28:59]

(components/password_manager/content/browser/content_password_manager_driver.*
LGTM so far.)

[dcheng, 2016-11-02 21:13:57]

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:28: if
(!m_mockSensitiveInputVisibilityService || true) {
On 2016/11/01 19:03:21, estark wrote:
> On 2016/11/01 18:59:47, estark wrote:
> > On 2016/11/01 18:51:36, dcheng wrote:
> > > Hmmm, is this just for testing?
> > 
> > Omg, that's embarrassing, sorry! Yes, that was for testing. I was debugging
> this
> > last night, came in this morning and forgot where I was and just saw that
the
> > tests pass and missed that I had added this. The weird thing is that the
tests
> > fail if I remove this and only create the
> m_mockSensitiveInputVisibilityService
> > once: that is,
> >
>
MockSensitiveInputVisibilityService::AllPasswordFieldsInInsecureContextInvisible
> > doesn't get called unless I recreate the service on every getInterface()
call.
> > Does that make any sense to you?
> 
> Oh, actually, looking at this again, I guess that does make sense, as |handle|
> has to be bound (? not sure if I'm using the right Mojo words) on each call to
> getInterface.
> 
> I feel like I'm not understanding something Mojo-y here but I'm not sure what
it
> is. Is there a way that getInterface() should work such that it doesn't create
a
> new MockSensitiveInputVisibilityService on every call?

For code that wants to reuse the same service impl object for multiple
connections, BindingSet can help manage the connections. Does that help here?

[estark, 2016-11-02 21:29:48]

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

[commit-bot: I haz the power, 2016-11-02 21:30:48]

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-02 21:31:05]

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:28: if
(!m_mockSensitiveInputVisibilityService || true) {
On 2016/11/02 21:13:57, dcheng wrote:
> On 2016/11/01 19:03:21, estark wrote:
> > On 2016/11/01 18:59:47, estark wrote:
> > > On 2016/11/01 18:51:36, dcheng wrote:
> > > > Hmmm, is this just for testing?
> > > 
> > > Omg, that's embarrassing, sorry! Yes, that was for testing. I was
debugging
> > this
> > > last night, came in this morning and forgot where I was and just saw that
> the
> > > tests pass and missed that I had added this. The weird thing is that the
> tests
> > > fail if I remove this and only create the
> > m_mockSensitiveInputVisibilityService
> > > once: that is,
> > >
> >
>
MockSensitiveInputVisibilityService::AllPasswordFieldsInInsecureContextInvisible
> > > doesn't get called unless I recreate the service on every getInterface()
> call.
> > > Does that make any sense to you?
> > 
> > Oh, actually, looking at this again, I guess that does make sense, as
|handle|
> > has to be bound (? not sure if I'm using the right Mojo words) on each call
to
> > getInterface.
> > 
> > I feel like I'm not understanding something Mojo-y here but I'm not sure
what
> it
> > is. Is there a way that getInterface() should work such that it doesn't
create
> a
> > new MockSensitiveInputVisibilityService on every call?
> 
> For code that wants to reuse the same service impl object for multiple
> connections, BindingSet can help manage the connections. Does that help here?

Ah, yes, I think so. The tests pass at least... let me know if what I did makes
any sense.

[dcheng, 2016-11-02 21:48:18]

lgtm

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:46: uint32_t
numPasswordFieldsInvisibleCalls() const {
Nit: #include <stdint.h> for uint32_t (or just write unsigned)

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:78:
m_mockSensitiveInputVisibilityService;
Optional nit: I think it's possible to make the service a direct member rather
than holding it in a unique_ptr.

[estark, 2016-11-02 21:54:01]

Thanks dcheng and vabr!

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
(right):

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:46: uint32_t
numPasswordFieldsInvisibleCalls() const {
On 2016/11/02 21:48:18, dcheng wrote:
> Nit: #include <stdint.h> for uint32_t (or just write unsigned)

Done.

https://codereview.chromium.org/2468833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp:78:
m_mockSensitiveInputVisibilityService;
On 2016/11/02 21:48:18, dcheng wrote:
> Optional nit: I think it's possible to make the service a direct member rather
> than holding it in a unique_ptr.

Done.

[estark, 2016-11-02 21:54:06]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-02 21:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2468833002/#ps100001
(title: "dcheng comments")

[commit-bot: I haz the power, 2016-11-02 21:54:39]

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-02 23:26:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-02 23:29:30]

Description was changed from

==========
Count visible password fields on a page

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 counts password fields in a frame, incrementing the counter when
a PasswordInputType creates a layout object and decrementing it on the
layout object's willBeDestroyed() method. A Mojo IPC is sent when the
counter is decremented to 0.

A follow-up CL will keep track of the visibility of password fields per
RenderFrameHost and remove the omnibox warning when there are no visible
password fields anywhere in the frame tree.

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

to

==========
Count visible password fields on a page

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 counts password fields in a frame, incrementing the counter when
a PasswordInputType creates a layout object and decrementing it on the
layout object's willBeDestroyed() method. A Mojo IPC is sent when the
counter is decremented to 0.

A follow-up CL will keep track of the visibility of password fields per
RenderFrameHost and remove the omnibox warning when there are no visible
password fields anywhere in the frame tree.

BUG=658764

Committed: https://crrev.com/556439aac8b23fd346feb501d33a1488aa4f7f7d
Cr-Commit-Position: refs/heads/master@{#429450}
==========

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

Patchset 6 (id:??) landed as
https://crrev.com/556439aac8b23fd346feb501d33a1488aa4f7f7d
Cr-Commit-Position: refs/heads/master@{#429450}

[tkent, 2016-11-07 03:56:13]

tkent@chromium.org changed reviewers:
+ eae@chromium.org

[tkent, 2016-11-07 03:57:37]

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

[tkent, 2016-11-07 03:57:38]

+eae

Adding new LayoutObject subclass just for counting looks overkill to me.  Can't
we decrease the counter in Node::detachLayoutTree?

[eae, 2016-11-07 04:43:11]

On 2016/11/07 03:57:38, tkent wrote:
> +eae
> 
> Adding new LayoutObject subclass just for counting looks overkill to me. 
Can't
> we decrease the counter in Node::detachLayoutTree?

That sounds like it would work.

[estark, 2016-11-07 06:35:27]

On 2016/11/07 04:43:11, eae wrote:
> On 2016/11/07 03:57:38, tkent wrote:
> > +eae
> > 
> > Adding new LayoutObject subclass just for counting looks overkill to me. 
> Can't
> > we decrease the counter in Node::detachLayoutTree?
> 
> That sounds like it would work.

Ok, thanks for the suggestion! I'll give that a try this week.

[estark, 2016-11-08 00:45:19]

On 2016/11/07 06:35:27, estark wrote:
> On 2016/11/07 04:43:11, eae wrote:
> > On 2016/11/07 03:57:38, tkent wrote:
> > > +eae
> > > 
> > > Adding new LayoutObject subclass just for counting looks overkill to me. 
> > Can't
> > > we decrease the counter in Node::detachLayoutTree?
> > 
> > That sounds like it would work.
> 
> Ok, thanks for the suggestion! I'll give that a try this week.

I played around with this suggestion a little bit today and I'm not sure it
gives the behavior that I want. If I increment/decrement the counter in
attachLayoutTree/detachLayoutTree, then the counter gets incremented even if the
password field is set to display:none. I'd like the counter to only increment
when the field is visible.

[tkent, 2016-11-08 01:30:01]

On 2016/11/08 at 00:45:19, estark wrote:
> I played around with this suggestion a little bit today and I'm not sure it
gives the behavior that I want. If I increment/decrement the counter in
attachLayoutTree/detachLayoutTree, then the counter gets incremented even if the
password field is set to display:none. I'd like the counter to only increment
when the field is visible.

Did you try:
 - HTMLInputElement::attachLayoutTree: Count up if layoutObject exists *after*
calling HTMLTextFormControlElement::attachLayoutTree()
 - HTMLInputElement::detachLayoutTree: Count down if layoutObject exists *before
calling HTMLTextFormControlElement::detachLayoutTree()

[estark, 2016-11-08 01:36:43]

On 2016/11/08 01:30:01, tkent wrote:
> On 2016/11/08 at 00:45:19, estark wrote:
> > I played around with this suggestion a little bit today and I'm not sure it
> gives the behavior that I want. If I increment/decrement the counter in
> attachLayoutTree/detachLayoutTree, then the counter gets incremented even if
the
> password field is set to display:none. I'd like the counter to only increment
> when the field is visible.
> 
> Did you try:
>  - HTMLInputElement::attachLayoutTree: Count up if layoutObject exists *after*
> calling HTMLTextFormControlElement::attachLayoutTree()
>  - HTMLInputElement::detachLayoutTree: Count down if layoutObject exists
*before
> calling HTMLTextFormControlElement::detachLayoutTree()

Aha, I think that works, thanks. Will send a CL.