Use an MutationObserver to check when a password form disappears after XHR

Use an MutationObserver to check when a password form disappears after XHR

Right now, we consider a password submitted if the filled form is gone
after an successful XHR or fetch operation. However, if the website
didn't synchronously remove the password form in the event handler, we
will not consider the password submitted.

In that case, we now use a MutationObserver to be notified about future
changes to the form, and if it disappears, we'd still consider the
password submission successful.

BUG=589533
R=dvadym@chromium.org, dglazkov@chromium.org

Review-Url: https://codereview.chromium.org/2865233003
Cr-Commit-Position: refs/heads/master@{#471421}
Committed: https://chromium.googlesource.com/chromium/src/+/fcf91a8eaacdeaedb0de1eba39ff4c24fab34323

[jochen (gone - plz use gerrit), 2017-05-09 13:53:48]

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

[commit-bot: I haz the power, 2017-05-09 13:54:09]

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, 2017-05-09 14:10:17]

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

[commit-bot: I haz the power, 2017-05-09 14:10:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jochen (gone - plz use gerrit), 2017-05-09 14:16:28]

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

[commit-bot: I haz the power, 2017-05-09 14:16:45]

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, 2017-05-09 15:25:44]

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

[commit-bot: I haz the power, 2017-05-09 15:25:44]

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

[jochen (gone - plz use gerrit), 2017-05-10 09:11:23]

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

[commit-bot: I haz the power, 2017-05-10 09:11:42]

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, 2017-05-10 10:10:32]

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

[commit-bot: I haz the power, 2017-05-10 10:10:34]

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

[jochen (gone - plz use gerrit), 2017-05-10 11:29:35]

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

[commit-bot: I haz the power, 2017-05-10 11:30:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-05-10 11:42:53]

Description was changed from

==========
Use an MutationObserver to check when a password form disappears after XHR

Right now, we consider a password submitted if the filled form is gone
after an successful XHR or fetch operation. However, if the website
didn't synchronously remove the password form in the event handler, we
will not consider the password submitted.

In that case, we now use a MutationObserver to be notified about future
changes to the form, and if it disappears, we'd still consider the
password submission successful.

BUG=589533
R=dvadym@chromium.org
==========

to

==========
Use an MutationObserver to check when a password form disappears after XHR

Right now, we consider a password submitted if the filled form is gone
after an successful XHR or fetch operation. However, if the website
didn't synchronously remove the password form in the event handler, we
will not consider the password submitted.

In that case, we now use a MutationObserver to be notified about future
changes to the form, and if it disappears, we'd still consider the
password submission successful.

BUG=589533
R=dvadym@chromium.org, dglazkov@chromium.org
==========

[jochen (gone - plz use gerrit), 2017-05-10 11:45:40]

jochen@chromium.org changed reviewers:
+ dglazkov@chromium.org

[jochen (gone - plz use gerrit), 2017-05-10 11:45:42]

PTAL,

Vadym: everything
Dimitri: blink part

https://codereview.chromium.org/2865233003/diff/60001/chrome/renderer/autofil...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (left):

https://codereview.chromium.org/2865233003/diff/60001/chrome/renderer/autofil...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1678:
password_element_.SetAttribute("style", "display:none;");
These methods don't actually spin Blink's message loop (and neither does
RunUntilIdle()), so I replaced them with equivalent JS calls

https://codereview.chromium.org/2865233003/diff/60001/components/autofill/con...
File components/autofill/content/DEPS (left):

https://codereview.chromium.org/2865233003/diff/60001/components/autofill/con...
components/autofill/content/DEPS:5: "+third_party/WebKit/public/web",
browser/ must not blanket allow WebKit includes, so I cleaned that up

https://codereview.chromium.org/2865233003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/FormElementObserver.cpp (right):

https://codereview.chromium.org/2865233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/FormElementObserver.cpp:139: if
(!it->value->ShouldStopObserving())
To avoid duplicating the logic to check whether we consider a form element
visible or not, I made the callback decide that

[commit-bot: I haz the power, 2017-05-10 12:33:05]

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

[commit-bot: I haz the power, 2017-05-10 12:33:06]

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

[jochen (gone - plz use gerrit), 2017-05-10 15:59:27]

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

[commit-bot: I haz the power, 2017-05-10 16:00:38]

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, 2017-05-10 16:13:40]

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

[commit-bot: I haz the power, 2017-05-10 16:13:41]

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

[jochen (gone - plz use gerrit), 2017-05-10 16:25:57]

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

[commit-bot: I haz the power, 2017-05-10 16:27:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2017-05-10 16:42:52]

Thanks Jochen! It looks great, I was playing around a little bit with patch
locally.

I have some questions, in order to clarify how exactly it works and to
understand whether we can have false positive.

What exactly the moments when ShouldStopObserving() is called? On any form
change? Wouldn't it possible to receive from Blink an event when the form is
removed from DOM? And in that case we could skip checking of visibility. Now
OnSameDocumentNavigationCompleted() is called from ShouldStopObserving and it
checks visibility of the form. And as far as I understand checking visibility
might be expensive. WDYT?

FYI I've landed today CL  https://codereview.chromium.org/2859243004/ with a
metric for password form submission types which we used for successful form
submissions. Could you please add one more type to this metric?

https://codereview.chromium.org/2865233003/diff/60001/components/autofill/con...
File components/autofill/content/DEPS (left):

https://codereview.chromium.org/2865233003/diff/60001/components/autofill/con...
components/autofill/content/DEPS:5: "+third_party/WebKit/public/web",
On 2017/05/10 11:45:41, jochen wrote:
> browser/ must not blanket allow WebKit includes, so I cleaned that up

Thanks

[commit-bot: I haz the power, 2017-05-10 17:51:20]

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

[commit-bot: I haz the power, 2017-05-10 17:51:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jochen (gone - plz use gerrit), 2017-05-11 07:24:55]

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

[commit-bot: I haz the power, 2017-05-11 07:25:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-05-11 07:25:54]

On 2017/05/10 at 16:42:52, dvadym wrote:
> Thanks Jochen! It looks great, I was playing around a little bit with patch
locally.
> 
> I have some questions, in order to clarify how exactly it works and to
understand whether we can have false positive.
> 
> What exactly the moments when ShouldStopObserving() is called? On any form
change? Wouldn't it possible to receive from Blink an event when the form is
removed from DOM? And in that case we could skip checking of visibility. Now
OnSameDocumentNavigationCompleted() is called from ShouldStopObserving and it
checks visibility of the form. And as far as I understand checking visibility
might be expensive. WDYT?

I updated the blink side to actually determine whether the form is visible or
not. We now notify under three conditions:

the element was removed from the DOM

the action attribute changed

either the class or style attribute changed, and the computed style is now
"display: none"

> 
> FYI I've landed today CL  https://codereview.chromium.org/2859243004/ with a
metric for password form submission types which we used for successful form
submissions. Could you please add one more type to this metric?

done

[commit-bot: I haz the power, 2017-05-11 07:43:19]

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

[commit-bot: I haz the power, 2017-05-11 07:43:19]

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

[jochen (gone - plz use gerrit), 2017-05-11 08:46:08]

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

[commit-bot: I haz the power, 2017-05-11 08:46:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-05-11 10:20:14]

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

[jochen (gone - plz use gerrit), 2017-05-11 10:21:25]

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

[jochen (gone - plz use gerrit), 2017-05-11 10:21:26]

+mkwst for components/autofill/content/common/* (IPC review)

[dvadym, 2017-05-11 11:09:56]

Thanks! Password part LGTM

[dglazkov, 2017-05-11 21:09:05]

My concerns are mostly around layering...

https://codereview.chromium.org/2865233003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2865233003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1694: Member<FormElementObserver>
form_element_observer_;
Does it have to be a member of Document? I am primarily worried about adding
things to Document that aren't technically part of core DOM implementation.

It looks like it's instantiated on access (RAII pattern), so maybe it could be
in a separate class? IOW, could we introduce WebFormElementObserver, which wraps
the observer and then let create it in PasswordAutofillAgent, passing
WebDocument to it?

https://codereview.chromium.org/2865233003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/FormElementObserver.h (right):

https://codereview.chromium.org/2865233003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/FormElementObserver.h:1: // Copyright 2017
The Chromium Authors. All rights reserved.
Ideally, FormElementObserver shouldn't live in core/dom, since it's not part of
DOM implementation. In such cases, we typically put things in modules.

[jochen (gone - plz use gerrit), 2017-05-12 11:38:20]

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

[commit-bot: I haz the power, 2017-05-12 11:38:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-05-12 11:38:41]

ptal.

We don't have many modules that depend on web/ (only serviceworker), and it has
it's implementation in Source/web, so I followed that pattern.

[Mike West, 2017-05-12 11:46:28]

mojo LGTM. I only skimmed the rest.

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/modules/password_manager/OWNERS (right):

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/password_manager/OWNERS:1:
jochen@chromium.org
Nit: Perhaps someone from the password manager team wants to help out with the
blink side of their feature?

[jochen (gone - plz use gerrit), 2017-05-12 11:48:32]

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/modules/password_manager/OWNERS (right):

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/password_manager/OWNERS:1:
jochen@chromium.org
On 2017/05/12 at 11:46:28, Mike West wrote:
> Nit: Perhaps someone from the password manager team wants to help out with the
blink side of their feature?

Vadym, any proposals who that could be?

[commit-bot: I haz the power, 2017-05-12 13:30:48]

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

[commit-bot: I haz the power, 2017-05-12 13:30:49]

Dry run: This issue passed the CQ dry run.

[dglazkov, 2017-05-12 19:42:01]

lgtm

[jochen (gone - plz use gerrit), 2017-05-12 19:51:17]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-05-12 19:51:17]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2865233003/#ps140001
(title: "updates")

[commit-bot: I haz the power, 2017-05-12 19:51:58]

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, 2017-05-12 20:04:48]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494618677097300,
"parent_rev": "874c38a6ffddc2794f18a913297cd1cdfbae3526", "commit_rev":
"fcf91a8eaacdeaedb0de1eba39ff4c24fab34323"}

[commit-bot: I haz the power, 2017-05-12 20:05:02]

Description was changed from

==========
Use an MutationObserver to check when a password form disappears after XHR

Right now, we consider a password submitted if the filled form is gone
after an successful XHR or fetch operation. However, if the website
didn't synchronously remove the password form in the event handler, we
will not consider the password submitted.

In that case, we now use a MutationObserver to be notified about future
changes to the form, and if it disappears, we'd still consider the
password submission successful.

BUG=589533
R=dvadym@chromium.org, dglazkov@chromium.org
==========

to

==========
Use an MutationObserver to check when a password form disappears after XHR

Right now, we consider a password submitted if the filled form is gone
after an successful XHR or fetch operation. However, if the website
didn't synchronously remove the password form in the event handler, we
will not consider the password submitted.

In that case, we now use a MutationObserver to be notified about future
changes to the form, and if it disappears, we'd still consider the
password submission successful.

BUG=589533
R=dvadym@chromium.org, dglazkov@chromium.org

Review-Url: https://codereview.chromium.org/2865233003
Cr-Commit-Position: refs/heads/master@{#471421}
Committed:
https://chromium.googlesource.com/chromium/src/+/fcf91a8eaacdeaedb0de1eba39ff...
==========

[commit-bot: I haz the power, 2017-05-12 20:05:04]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/fcf91a8eaacdeaedb0de1eba39ff...

[dvadym, 2017-05-16 10:17:46]

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/modules/password_manager/OWNERS (right):

https://codereview.chromium.org/2865233003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/password_manager/OWNERS:1:
jochen@chromium.org
On 2017/05/12 11:48:32, jochen wrote:
> On 2017/05/12 at 11:46:28, Mike West wrote:
> > Nit: Perhaps someone from the password manager team wants to help out with
the
> blink side of their feature?
> 
> Vadym, any proposals who that could be?

Sure. I think me, vabr and kolos are good candidates. I'll create a CL for
adding us as owners for this directory.