Thanks Vaclav, very clear explanation. Moving the Autofill parsing to during DidFinishLoad seems like an obvious thing to do, to ensure correctness of the visibility check. In terms of timing, does DidFinishLoad come seconds after DidFinishDocumentLoad, or in your testing they happen relatively close to each other?

Thanks for filing.

Autofill re-extracts forms if/when it detects that they've changed. It looks like we also redo some of the extraction on receiving some messages from the browser.

For autofill, and I suspect the same applies to password manager, in the general case:

  * we want to parse and have representation/prediction for all fields, regardless
    of their visibility (which can be dynamic/transient)

  * at suggest/fill/interaction-time, we want to know exactly which fields are visible
    to the user.

  * We want to reparse when the DOM changes with respect to forms/fields.

Have I missed any general/specific requirements?

Based on these, I see only one reason not to consolidate the parsing to always be post render: we can initiate requests to the prediction server earlier.

That said, I think parsing for the purposes of sending server requests doesn't require us to ascertain the field's visibility. We only need to know that when we're about to suggest, preview, or fill.

Look at PasswordManagerBrowserTestWithViewsFeature.SlowPageFill I added some time ago. It ensures that the page is still autofilled even if DidFinishLoad is significantly delayed. Or may be it never happens on slow networks.

I think we should audit the code and don't call anything inappropriate from DidFinishDocumentLoad. And ScopedLayoutPreventer could enforce it by crashing the process.

Thanks all for sharing your knowledge!

From #3 I understand that there can be a significant pause between DidFinishDocumentLoad and DidFinishLoad. So for purposes of fetching stuff from the server (or PasswordStore), it probably makes sense to act in DidFinishDocumentLoad.

However, with filling, there are arguments going both ways:
Vasilii added the test (for  bug 713645 ) to cover situations when DidFinishLoad is delayed due to subresource loading, but the rest of the page is already rendered and the user expects the (visible) form to be filled.

OTOH,  bug 851808  describes how Chrome ends up filling an invisible field and stick to it even later (when it's already clear it's invisible), when the filling is done without visibility information.

Ideally, we should have a better signal which tells us when to check visibility safely. I suspect that this happens long before DidFinishLoad in the case from  bug 713645 , for example.

Hi Blink layout experts!

For parsing web forms in autofill, we'd like to check if fields are visible (WebElement::IsFocusable()). We understand that this is not a good idea before rendering finishes, to avoid premature or even nested layouts.

So far, we use DidFinishLoad as a reliable signal that it's OK to check visibility of elements. However,  bug 713645  indicates that there might be situations when checking visibility should be safe long before DidFinishLoad (e.g, if DidFinishLoad waits for a loading subresource but the page is otherwise rendered).

Is there a more precise way to know that it's safe to call WebElement::IsFocusable() than waiting for DidFinishLoad?

note that DidFinishLoad doesn't relate to rendering at all.

Good to know. The more I'd be interested in knowing what would be a good signal for calling WebElement::IsFocusable() safely.

+szager

Hmm, not sure why Blink>Layout was removed (probably in the deleted comment 9?).

I put it there to get advice from Blink people, because without that I'm stuck.

Blink experts, please see my comment #5 above, thanks!

szager (cc:ed in comment 8) would be the right person to ask.

Thanks, I will reach to szager directly.

szager pointed me to a comment for Element::IsFocusable() [1], which says that Document::UpdateStyleAndLayoutTree() should be called before Element::IsFocusable(), but does not mention any restrictions. This hints that it should be safe to call Element::IsFocusable() as long as Document::UpdateStyleAndLayoutTree() is called.

szager further says that Document::UpdateStyleAndLayoutTree() does not "run the layout algorithm, but it will populate the tree of layout objects and apply styles". This should ensure that bug 595078 is not repeated, but seems enough to get good visibility approximation to avoid bugs like  issue 851808 .

Finally, WebNode::IsFocusable() calls Document::UpdateStyleAndLayoutTreeForNode for the specific node [2], which ends up calling Document::UpdateStyleAndLayoutTree as necessary.

I think that based on this we can just get rid of autofill::form_util::ScopedLayoutPreventer. I will put together a CL to do that.


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/element.h?rcl=689e778b9014fe05e70fd811102bcead5aa22de7&l=614

[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/exported/web_node.cc?l=136&rcl=ba3e91b791530112940c9c55efc561156e9262f9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff6d76db8c032fa23bc8c215a87b53e99304ec5a

commit ff6d76db8c032fa23bc8c215a87b53e99304ec5a
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Jul 17 23:58:02 2018

Remove autofill::form_util::ScopedLayoutPreventer

ScopedLayoutPreventer was used to avoid detecting visibility of form
elements in situations when it was believed to cause pre-mature layout
(see https://crbug.com/595078). Visibility is approximated through
calling IsFocusable on the WebNodes corresponding to forms and their
fields.

Looking at recent documentation, though, the WebNode-based API seems
robust enough to prevent such dangerous calls:
  (1) The contract of Element::IsFocusable() only states the need to
      call Document::UpdateStyleAndLayoutTree() beforehand.
  (2) Document::UpdateStyleAndLayoutTree() does not cause the layout.
  (3) WebNode::IsFocusable() causes calling
      Document::UpdateStyleAndLayoutTree() as necessary before
      delegating to Element::IsFocusable().
See  https://crbug.com/855598#c14  for more context.

As a result, ScopedLayoutPreventer blocked visibility checks
unnecessarily, leading to bugs like  https://crbug.com/851808 .

Therefore this CL removes ScopedLayoutPreventer.

Bug:  855598 , 851808 ,595078
Change-Id: Ic415919be5c40821dfa965eeb16372ae92ba92d5
Reviewed-on: https://chromium-review.googlesource.com/1136531
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575859}
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util.h
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/form_autofill_util_browsertest.cc
[modify] https://crrev.com/ff6d76db8c032fa23bc8c215a87b53e99304ec5a/components/autofill/content/renderer/password_autofill_agent.cc

After r575859 there is no inconsistency of preventing premature layout in autofill-related code. All of that code now relies on the built-in measures in WebNode::IsFocusable. As long as that CL sticks, we can consider this solved.