Password manager should autofill a dynamic form added and processed before the page was loaded.

Password manager should autofill a dynamic form added and processed before the page was loaded.

Currently AutofillAgent::DidAssociateFormControlsDynamically ignores the notification if the frame isn't loaded. That makes no sense because the page may never be loaded due to some slow resource. Even if it will, password autofill shouldn't wait for the page load event. It's needed primarily for the successful submission detection.

BUG=713645
Review-Url: https://codereview.chromium.org/2849653003
Cr-Commit-Position: refs/heads/master@{#468136}
Committed: https://chromium.googlesource.com/chromium/src/+/31ba8c6c5838fd6e65f3e341cfb5af0736186dc3

[vasilii, 2017-04-28 13:39:22]

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

[commit-bot: I haz the power, 2017-04-28 13:39:39]

Dry run: CQ is trying da patch.

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

[vasilii, 2017-04-28 13:41:29]

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

[vasilii, 2017-04-28 13:41:30]

Hi Vaclav,

please review.

[commit-bot: I haz the power, 2017-04-28 15:15:00]

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

[commit-bot: I haz the power, 2017-04-28 15:15:01]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-04-28 19:17:34]

LGTM, thanks for the investigation and fix!

Vaclav

https://codereview.chromium.org/2849653003/diff/1/chrome/test/data/password/i...
File chrome/test/data/password/infinite_password_form.html (right):

https://codereview.chromium.org/2849653003/diff/1/chrome/test/data/password/i...
chrome/test/data/password/infinite_password_form.html:24: <img src="/slow?100">
Nice trick!

https://codereview.chromium.org/2849653003/diff/1/components/autofill/content...
File components/autofill/content/renderer/autofill_agent.cc (left):

https://codereview.chromium.org/2849653003/diff/1/components/autofill/content...
components/autofill/content/renderer/autofill_agent.cc:742: // Frame is only
processed if it has finished loading, otherwise you can end
This comment comes from https://codereview.chromium.org/277043002, with some
discussion at
https://codereview.chromium.org/277043002/diff/20001/components/autofill/cont....
Your findings about the "infinitely long loading" confirm that isherman's
concerns were real.

The partially parsed forms were not seen by users, just by some tests which no
longer exist (and logs from which are long gone). I suppose this could happen if
someone inserted a form first into DOM and then modified it, but that seems
uncommon. The bug which is going to be fixed by this is likely more frequent.

[vasilii, 2017-04-28 21:26:23]

https://codereview.chromium.org/2849653003/diff/1/chrome/test/data/password/i...
File chrome/test/data/password/infinite_password_form.html (right):

https://codereview.chromium.org/2849653003/diff/1/chrome/test/data/password/i...
chrome/test/data/password/infinite_password_form.html:24: <img src="/slow?100">
On 2017/04/28 19:17:33, vabr (Chromium) wrote:
> Nice trick!

Acknowledged.

https://codereview.chromium.org/2849653003/diff/1/components/autofill/content...
File components/autofill/content/renderer/autofill_agent.cc (left):

https://codereview.chromium.org/2849653003/diff/1/components/autofill/content...
components/autofill/content/renderer/autofill_agent.cc:742: // Frame is only
processed if it has finished loading, otherwise you can end
On 2017/04/28 19:17:33, vabr (Chromium) wrote:
> This comment comes from https://codereview.chromium.org/277043002, with some
> discussion at
>
https://codereview.chromium.org/277043002/diff/20001/components/autofill/cont....
> Your findings about the "infinitely long loading" confirm that isherman's
> concerns were real.
> 
> The partially parsed forms were not seen by users, just by some tests which no
> longer exist (and logs from which are long gone). I suppose this could happen
if
> someone inserted a form first into DOM and then modified it, but that seems
> uncommon. The bug which is going to be fixed by this is likely more frequent.

The reply of gcasto@ is wrong today. DidAssociateFormControlsDynamically is
called only after DidFinishDocumentLoad(). Moreover, it's called at least 0.3
seconds after DidFinishDocumentLoad(). That is, most likely after
DidFinishLoad().
I'm not sure what "partially parsed form" really means. The code adds a form
with username and inserts the password input 1 sec later? But it still can
happen after the frame loaded.
I think there was a misunderstanding about frame->IsLoading(). It means there is
a video, image etc. still loading. Nothing to do with the DOM structure. $(
document ).ready() corresponds to DidFinishDocumentLoad(). If I don't use
setTimeout then the form is handled in DidFinishDocumentLoad() and the test
passes with the existing code.

[vasilii, 2017-04-28 21:26:29]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2017-04-28 21:27:17]

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-04-28 21:35:13]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493414789323980, "parent_rev":
"180f23bbfdd91ade1b232094a17e48a4e590f38d", "commit_rev":
"31ba8c6c5838fd6e65f3e341cfb5af0736186dc3"}

[commit-bot: I haz the power, 2017-04-28 21:35:22]

Description was changed from

==========
Password manager should autofill a dynamic form added and processed before the
page was loaded.

Currently AutofillAgent::DidAssociateFormControlsDynamically ignores the
notification if the frame isn't loaded. That makes no sense because the page may
never be loaded due to some slow resource. Even if it will, password autofill
shouldn't wait for the page load event. It's needed primarily for the successful
submission detection.

BUG=713645
==========

to

==========
Password manager should autofill a dynamic form added and processed before the
page was loaded.

Currently AutofillAgent::DidAssociateFormControlsDynamically ignores the
notification if the frame isn't loaded. That makes no sense because the page may
never be loaded due to some slow resource. Even if it will, password autofill
shouldn't wait for the page load event. It's needed primarily for the successful
submission detection.

BUG=713645

Review-Url: https://codereview.chromium.org/2849653003
Cr-Commit-Position: refs/heads/master@{#468136}
Committed:
https://chromium.googlesource.com/chromium/src/+/31ba8c6c5838fd6e65f3e341cfb5...
==========

[commit-bot: I haz the power, 2017-04-28 21:35:27]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/31ba8c6c5838fd6e65f3e341cfb5...