Can't upstream CC after iframe form submission as AutofillManager gets destroyed on iframe navigation |
|||
Issue descriptionLifetime 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.
,
Oct 5
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?
,
Oct 5
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.
,
Oct 5
on Desktop navigation inside a frame doesn't kill the frame. Why does the iOS implementation diverge here?
,
Oct 5
On navigation (even within a frame), a new JS context is created. So we need to recreate all the messaging objects.
,
Oct 5
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?
,
Oct 5
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?
,
Oct 5
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?
,
Oct 5
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.
,
Oct 5
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.
,
Oct 5
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.
,
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
,
Oct 10
,
Oct 31
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/.
,
Oct 31
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).
,
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
,
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 |
|||
Comment 1 by olivierrobin@chromium.org
, Oct 5