New issue
Advanced search Search tips

Issue 820072 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Async suggestions retrieval is breaking autofill

Project Member Reported by mahmadi@chromium.org, Mar 8 2018

Issue description

1) Go to amazon.com
2) Sign in and add a new address
3) Address form doesn't autofill

I investigated and the cause seem to be crrev.com/c/921961
password controller, as the first suggestion provider, fails to return YES/NO to checkIfSuggestionsAvailableForForm: call. Therefore autofill agent is never queried.

If I'm understanding correctly, your CL is trying to fix the forms that are added after page load. olivierrobin@ has a CL in flight that might address the issue for password controller as well. crrev.com/c/943321
what do you think about sharing that logic with autofill? 
 
Cc: se...@chromium.org olivierrobin@chromium.org

Comment 2 by se...@chromium.org, Mar 8 2018

Yes Autofill should probably do something similar. I didn't include it in my dynamic forms fix doc because I want to take the time to detail this separately.

The reason being that we may not want to fill new fields as they are added to the form, since the user may not want that.

Comment 3 by se...@chromium.org, Mar 8 2018

Opps - wrongly associated this with dynamic forms filling.

Yes that would be useful. I think on Desktop we currently re-parse the whole page when that happens. But I don't think we catch all cases.
Status: Started (was: Assigned)
I'll check what're the problems here. 

Autofill already does async suggestions (here https://cs.chromium.org/chromium/src/components/autofill/ios/browser/autofill_agent.mm?q=checkIfSuggestionsAvailableForForm&sq=package:chromium&l=543 completion handler is copied in order to respond when suggestions are ready). Async here means - async between Obj-C and C++. As far as I understand the problem, that crrev.com/c/943321 is trying to solve, is to be able to get server-side predictions. 
yes, autofill does query suggestions asynchronously. It's ok if password controller does the same. It just has to make sure it always invokes the callback, otherwise autofill will never be queried for suggestions. see: https://cs.chromium.org/chromium/src/ios/chrome/browser/autofill/form_suggestion_controller.mm?l=250

crrev.com/c/943321 allows autofill to be notified of dynamically added forms. In this case used for getting server predictions. Is it possible that password controller can take advantage of it somehow? The more logic we share, the better I think.
Labels: OS-iOS
The CL to fix it is in code review https://chromium-review.googlesource.com/c/chromium/src/+/955685

mahmadi@ thanks for adding in loop about new dynamic form detection in crrev.com/c/943321 
Thank you for the quick fix!
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 8 2018

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

commit af3972380eb097187f629bf0716141b73ae38b8d
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Thu Mar 08 17:24:57 2018

[IOS Password Manager] Inform that no suggestions when no password forms.

On CL https://chromium-review.googlesource.com/c/chromium/src/+/921961
async retrieval of suggestions in Password Manager was implemented.
The problem that when there are no password forms PasswordController
doesn't call callback that there are no suggestions so in result
Autofill can't provide own suggestions. This CL fixes that.

Bug:  820072 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4a7dce1540b2217b388caadb19fc2e563c79eb26
Reviewed-on: https://chromium-review.googlesource.com/955685
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541821}
[modify] https://crrev.com/af3972380eb097187f629bf0716141b73ae38b8d/ios/chrome/browser/passwords/password_controller.mm
[modify] https://crrev.com/af3972380eb097187f629bf0716141b73ae38b8d/ios/chrome/browser/passwords/password_controller_unittest.mm

Status: Fixed (was: Started)
Labels: Merge-Request-66
Project Member

Comment 11 by sheriffbot@chromium.org, Mar 13 2018

Labels: -Merge-Request-66 Merge-Approved-66 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 15 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84cf416270891fd9d4a74fbce7f7736274413455

commit 84cf416270891fd9d4a74fbce7f7736274413455
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Thu Mar 15 17:50:51 2018

[IOS Password Manager] Inform that no suggestions when no password forms.

On CL https://chromium-review.googlesource.com/c/chromium/src/+/921961
async retrieval of suggestions in Password Manager was implemented.
The problem that when there are no password forms PasswordController
doesn't call callback that there are no suggestions so in result
Autofill can't provide own suggestions. This CL fixes that.

TBR=dvadym@chromium.org

(cherry picked from commit af3972380eb097187f629bf0716141b73ae38b8d)

Bug:  820072 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4a7dce1540b2217b388caadb19fc2e563c79eb26
Reviewed-on: https://chromium-review.googlesource.com/955685
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#541821}
Reviewed-on: https://chromium-review.googlesource.com/964322
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#272}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/84cf416270891fd9d4a74fbce7f7736274413455/ios/chrome/browser/passwords/password_controller.mm
[modify] https://crrev.com/84cf416270891fd9d4a74fbce7f7736274413455/ios/chrome/browser/passwords/password_controller_unittest.mm

Verified on chrome canary version 67.0.3375.0 on iPhone 8 with iOS 11.2.6 and iPad pro with iOS 11.2.6 following steps mentioned in comment #1.  Address form autofill.  Looks good.
Status: Verified (was: Fixed)
Verified on M66.0.3359.52 beta.
New address form is autofilled correctly on amazon.com
Tested on iPhone8, iOS11.2.6

Sign in to add a comment