New issue
Advanced search Search tips

Issue 914327 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 916137



Sign in to add a comment

Submission success detection is broken on https://www.topcashback.co.uk

Project Member Reported by cfroussios@chromium.org, Dec 12

Issue description

1. Visit https://www.topcashback.co.uk/logon?PageRequested=https%3a%2f%2fwww.topcashback.co.uk%2fHome
2. Enter garbage credentials into the login form
3. Press Login

Expected:
The submission is understood to be unsuccessful, based on the fact that the form was seen again.

Observed:
Submission was seen as successful.


Signature of form: 4290511264542618811
Google Chrome 71.0.3578.80 (Official Build) (64-bit)
 
Description: Show this description
I think this is caused by a race condition due to OOPIF.

The save prompt appears only in multi-process mode, not in single process mode.

In single process mode we observe:
- DidFinishDocumentLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=false) ==> aborts (page empty)
- DidFinishLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=true) ==> aborts (page empty)
- DidFinishDocumentLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=false) ==> This one actually sends 1 parsed form to PasswordFormsParsed
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> sees no forms
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> aborts (empty page)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> aborts (empty page)
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> aborts (empty page)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> aborts (empty page)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> Sends 0 forms to PasswordFormsRendered with did_stop_loading=0
- OnPasswordFormsRendered() does nothing because did_stop_loading = 0
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> Sends 1 form to PasswordFormsRendered with did_stop_loading=1
- OnPasswordFormsRendered() sees the form and decides not to show the save dialog


In multi process mode we observe:
- DidFinishDocumentLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=false) ==> aborts (page empty)
- DidFinishLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=true) ==> aborts (page empty)
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> sees no forms
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> aborts (page empty)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> aborts (page empty)
- DidFinishDocumentLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=false) ==> aborts (page empty)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> aborts (page empty)
- DidFinishLoad() for https://www.youtube.com/
  - SendPasswordForms(only_visible=true) ==> This one sends 0 parsed form to PasswordFormsRendered and did_stop_loading=1
- OnPasswordFormsRendered() sees 0 forms and decides to show the popup
- DidFinishDocumentLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=false) ==> Sends 1 form to PasswordFormsParsed
- DidFinishLoad() for https://www.topcashback.co.uk/
  - SendPasswordForms(only_visible=true) ==> Sends 1 form to PasswordFormsRendered and did_stop_loading=1

So the problem seems to be that the iframe reports that loading has stopped and that it contains 0 forms, but the main frame has not had a chance to report the existence of forms.
How does DidFinishLoad get triggered:

R: multiple callers lead to the following: (for the main frame this is coming from blink::ImageLoader::DispatchPendingLoadEvent())
R: Document::CheckCompleted()
R: Document::CheckCompletedInternal()
R: LocalFrameClientImpl::DispatchDidFinishLoad()
R: WebLocalFrameImpl::DidFinish()
R: RenderFrameImpl::DidFinishLoad() 
R: PasswordAutofillAgent::DidFinishLoad()   [TODO: DidFinishDocumentLoad]
R: PasswordAutofillAgent::SendPasswordForms
B: ChromePasswordManagerClient::PasswordFormsRendered
B: PasswordManager::OnPasswordFormsRendered

PasswordAutofillAgent::SendPasswordForms calls ChromePasswordManagerClient::PasswordFormsRendered with the information whether main_frame->IsLoading() is false.

Problem: is_loading is set to false already but PasswordAutofillAgent::DidFinishLoad() has not been called for the main frame, yet.

Let's see how IsLoading() is determined to be false.
R: PasswordAutofillAgent::SendPasswordForms
R: WebFrame::IsLoading()
R: Frame::IsLoading() returns is_loading_

And this is how it gets set:

R: Document::FinishedParsing()
R: FrameLoader::FinishedParsing()
R: Document::CheckCompleted()
R: FrameLoader::DidFinishNavigation() (this gets called after Document::CheckCompletedInternal())
R: ProgressTracker::ProgressCompleted()
R: Frame::SetIsLoading()

This means: is_loading is set to false for the main frame when the document has been parsed. At this point it has not been rendered nor have all resources been loaded.

What we observe is:

1. Main frame is fully parsed --> is_loading is set to false for the main frame and DidFinishDocumentLoad() is called
2. The iframe is parsed --> DidFinishDocumentLoad is called for the iframe
3. All resources of the iframe have been loaded --> DidFinishLoad() is called for the iframe and the password manager displays the bubble.
4. All resources of the main frame have been loaded --> DidFinishLoad() is called for the mainframe. At this point we are aware of the password form (which should prevent the bubble) but it is visible already.


As of today:
When the main frame has finished parsing but has not loaded all resources, any iframe that has loaded all resources can trigger the password save bubble by claiming that all possible password forms have been observed.

What we can do:

1) In PasswordManager::OnPasswordFormsRendered, replace did_stop_loading with called_by_the_main_frame. This means when the main frame has loaded all resources, we evaluate whether we have observed the login/signup form again. We neither wait for iframes nor do we allow them to trigger this evaluation.

2) Try to get a better signal whether the main form has loaded all of its resources and pass that to this function. Unfortunately, WebFrame does not seem to have such a feature at the moment.

It looks like 1) subsumes 2), which makes this change simpler to implement. In the past we would delay the check for submission until the main frame has finished parsing the dom. With the change we would delay the check for submission until the main frame has also finished loading all resources (which triggers the SendPasswordForms(true) call for the main frame followed by the PasswordManager::OnPasswordFormsRendered() call).


Analysis:

Let's say we have a main frame M and two other frames A and B. The one denoted with * contains the login form.

The following page loaded event orders can happen and "broken" means that we show the save popup because we failed to find the login form after a failed submission:

If main frame contains the password form.
- M* A B - before: ok, after change: ok
- A M* B - before: potentially broken, after change: *ok*

- M A* B - before: broken, after change: broken
- A* M B - before: ok, after change ok
- M B A* - before broken, after change: broken
- A* B M - before: potentially broken, after change: *ok*
- B A* M - before: potentially broken, after change: *ok*
- B M A* - before: broken, after change: broken

This means that the change is an improvement but the submission detection remains broken in case the login form is in an iframe that finishes loading after the main frame.

I see four ways of addressing this:

1) Accept that this is broken.
2) Wait for all frames to have loaded all resources. --> This is problematic in case some (potentially hidden) iframes never finish loading.
3) Wait for all frames to have loaded all resources only if the submitted manager was in an iframe. Otherwise wait for the main frame to have loaded all resources.
4) As 3) but with a timeout that starts once the main frame finishes loading all resource.

It is unclear to me at the moment whether we have a good way of waiting for all frames, so 2-4 would be significantly more complex than 1.
Blockedon: 916137
The actual root cause is now documented in bug 916137.

Sign in to add a comment