New issue
Advanced search Search tips

Issue 892299 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Can't upstream CC after iframe form submission as AutofillManager gets destroyed on iframe navigation

Project Member Reported by mahmadi@chromium.org, Oct 4

Issue description

Lifetime of CreditCardSaveManager is indirectly tied to that of AutofillManager's. With chrome://flags/#web-frame-messaging enabled, An AutofillManager instance will exist per frame. However, navigation inside that frame (resulted form submission) causes the frame to become unavailable and therefore AutofillManager gets deleted. This prevents operations such as CC upstream from completion as they rely on the AutofillManager to stay around. This likely has other downsides such as sending votes after form submission.
 
According to AutofillManager documentation, 

https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofill_manager.h?q=autofill_manag&sq=package:chromium&g=0&l=67
// Manages saving and restoring the user's personal information entered into web
// forms. One per frame; owned by the AutofillDriver.

it seems that deleting the autofill manager when the frame is deleted.

This also seems to be the expected behavior on other platforms
https://cs.chromium.org/chromium/src/components/autofill/content/browser/content_autofill_driver_factory.cc?type=cs&g=0&l=133

So in my opinion, having asynchronous jobs that expect the autofill manager to live after the webFrame is destroyed is a bug.
I filed crbug.com/892612 to fix this.

For the short term solution, we can retain the last autofillManager that triggered a form submission.


AutofillManager's life is indeed tied to the frame. I think the issue here is that the frame gets destroyed on navigation in iOS and it doesn't in other platforms. wdyt?
It is a possiblity.
In that case, the frame would be kept in a cache.
But relying on this cache to make sure request are complete seems wrong to me.
on Desktop navigation inside a frame doesn't kill the frame. Why does the iOS implementation diverge here? 
On navigation (even within a frame), a new JS context is created.
So we need to recreate all the messaging objects.

Sorry if the answer is obvious, but can't WebFrame objects stay around after navigation while JS messaging is being reestablished? In other words, why should the frame get destroyed on 'unload' event?
You mean reuse the WebFrame object and change the key and ID etc...?
The answer is not obvious.

- The main reason is that the webframe is identified by an id. This ID has to be created in the frame (in JS) and not in the browser, because there is no secure channel from the browser to the frame before we setup the encryption keys.
So there is no way to securely (re)inject an ID in the JS frame.
This means that on any navigation, as the JS context changes, we have to create a new frame ID.
In the browser, we could reuse the WebFrame object, and just change it's key. But as the clients can only find the frame using it's key it would be a small gain (and a source of big mistakes). In our case, this would mean some security issues:
* Imagine that after the credit card is uploaded, the uploader send a confirmation to the webpage with the card number. As there was a navigation, the credit card number is sent to the wrong site.

- Note that on each navigation, we call autofillManager->Reset().
We could expect that this reset stops the upload queries.
There is a comment in the implementation
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofill_manager.cc?dr=CSs&g=0&l=1096
that states that the query is not reset precisely because there is navigation.
But this looks a workaround.

- also, if you navigate from a page with 10 frames to a page with 1 frame, which one do you reuse?


Thank you for the explanation. I'm fine with the AutofillManager getting destroyed after navigation. It's a lot cleaner and safer than resetting a subset of its state. We just have to make sure the behavior is the same on all platforms. We don't need more divergence in iOS :) Currently on Desktop, AutofillManager gets destroyed on main frame navigation only.

I think we should have payments::PaymentsClient, FormDataImporter, and AutofillDownloadManager live someplace else, AutofillController maybe?


If AutofillManager is destroyed on main frame navigation, does it mean that the same issues appears on desktop when the form is in the main frame?

I agree on the fix.
We need to move at least some parts of these class (the part that does the upload) to a frame independent owner (why not AutofillController).

I filed crbug.com/892612 for this fix, as it is different from the iOS specific issue.
As Oliver said, we have technical reasons why web::WebFrame has to be destroyed on navigation. There is no reliable way to distinguish cases when the frame was removed and when frame has simply changed document object. 

In /ios/web we trying to follow /content API contracts where possible and reasonable.
Thanks for filing the bug Olivier.
In my previous comment I meant AutofillManager gets *reset* on main frame navigation; but it should get *destroyed* instead as part of the future refactoring. 
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 10

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

commit 802862fd0577c749be374e2095e55bcbb13beb4e
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Wed Oct 10 06:51:22 2018

Retain AutofillDriver on form submission.

Bug: 892299
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I909b17a2ec51158f0ffa440abe2322e19e34bc12
Reviewed-on: https://chromium-review.googlesource.com/c/1264637
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598234}
[modify] https://crrev.com/802862fd0577c749be374e2095e55bcbb13beb4e/components/autofill/ios/browser/autofill_agent.mm
[modify] https://crrev.com/802862fd0577c749be374e2095e55bcbb13beb4e/components/autofill/ios/browser/autofill_driver_ios.mm
[modify] https://crrev.com/802862fd0577c749be374e2095e55bcbb13beb4e/components/autofill/ios/browser/autofill_driver_ios_webframe.h
[modify] https://crrev.com/802862fd0577c749be374e2095e55bcbb13beb4e/components/autofill/ios/browser/autofill_driver_ios_webframe.mm

Status: Fixed (was: Untriaged)
Cc: -eugene...@chromium.org -michaeldo@chromium.org rogerm@google.com
Labels: -M-71 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: mahmadi@chromium.org
Status: Assigned (was: Fixed)
Turns out AutofillManager is destroyed on navigation to a different domain on non-iOS platforms as well. Therefore, credit card save is not offered when submitting credit card forms in https://rsolomakhin.github.io/autofill/ as it redirects to https://example.com/. 
As a followup to #c14, for local credit card save, it *is* offered but clicking [Save] does not actually save the card (despite showing a "Card saved" animation and followup bubble).
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 6

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

commit 68eb3344df9069012d662b394e9991d3095b362e
Author: Jared Saul <jsaul@google.com>
Date: Tue Nov 06 21:49:19 2018

[Autofill] Add metric to log when local save is successful

We're looking to merge this (ideally temporary) metric into M71.
A problem has been discovered with regards to checkout flows that span
multiple domains, and this will help determine its impact.

Bug: 892299
Change-Id: I79c9acba23eb750aecf0171ee92d0de456c2ce0e
Reviewed-on: https://chromium-review.googlesource.com/c/1315953
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#605825}
[modify] https://crrev.com/68eb3344df9069012d662b394e9991d3095b362e/components/autofill/core/browser/autofill_metrics.cc
[modify] https://crrev.com/68eb3344df9069012d662b394e9991d3095b362e/components/autofill/core/browser/autofill_metrics.h
[modify] https://crrev.com/68eb3344df9069012d662b394e9991d3095b362e/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[modify] https://crrev.com/68eb3344df9069012d662b394e9991d3095b362e/components/autofill/core/browser/personal_data_manager.cc
[modify] https://crrev.com/68eb3344df9069012d662b394e9991d3095b362e/tools/metrics/histograms/histograms.xml

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 7

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

commit 1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb
Author: Moe Ahmadi <mahmadi@chromium.org>
Date: Wed Nov 07 19:10:33 2018

[AF] Moves payments::PaymentsClient and FormDataImporter to AutofillClient

This CL moves ownership of payments::PaymentsClient and FormDataImporter to
implementations of AutofillClient in order for these objects to live beyond
the lifespan of AutofillManager. AutofillDriver and thus AutofillManager's
lifespan is tied to the renderer frame which gets destroyed with
cross-domain navigations on form submission and therefore creidt card save
isn't offered. With this change these objects' lifespan will be tied to the
tab.

Bug: 892299
Change-Id: I1651e92ba032d04d047221d91bafbdfcc511296a
Reviewed-on: https://chromium-review.googlesource.com/c/1320215
Commit-Queue: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: John Wu <jzw@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606100}
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/android_webview/browser/aw_autofill_client.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/android_webview/browser/aw_autofill_client.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/chrome/browser/ui/autofill/chrome_autofill_client.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/chrome/browser/ui/autofill/chrome_autofill_client.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/chrome/browser/ui/views/autofill/save_card_bubble_views_browsertest_base.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_assistant_unittest.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_client.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_manager.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_manager.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/autofill_metrics_unittest.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/credit_card_save_manager_unittest.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/local_card_migration_manager_unittest.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/test_autofill_client.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/test_autofill_client.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/test_autofill_manager.cc
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/core/browser/test_autofill_manager.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/components/autofill/ios/browser/autofill_agent.mm
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/chrome/browser/autofill/autofill_controller_unittest.mm
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/chrome/browser/autofill/form_structure_browsertest.mm
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/chrome/browser/ui/autofill/chrome_autofill_client_ios.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/chrome/browser/ui/autofill/chrome_autofill_client_ios.mm
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/chrome/browser/ui/autofill/save_card_infobar_egtest.mm
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/web_view/internal/autofill/web_view_autofill_client_ios.h
[modify] https://crrev.com/1ba572f8b3e4c50e2ff7bafb37267c5d2c4d3edb/ios/web_view/internal/autofill/web_view_autofill_client_ios.mm

Sign in to add a comment