[Autofill] Autofill fails to fill credit card number when split across fields.

components/autofill/core/browser/credit_card_field.cc

Index: components/autofill/core/browser/credit_card_field.cc
diff --git a/components/autofill/core/browser/credit_card_field.cc b/components/autofill/core/browser/credit_card_field.cc
index 73dc90db9fc32658b470ec80b5d732fc5865e73d..8af884632925dbae464b4c95d9a240a8505606cd 100644
--- a/components/autofill/core/browser/credit_card_field.cc
+++ b/components/autofill/core/browser/credit_card_field.cc
@@ -19,6 +19,10 @@
 
 namespace autofill {
 
+// Credit card numbers are at most 19 digits in length.
+// [Ref: http://en.wikipedia.org/wiki/Bank_card_number]
+static const size_t kMaxValidCardNumberSize = 19;
+
 // static
 FormField* CreditCardField::Parse(AutofillScanner* scanner) {
   if (scanner->IsEnd())
@@ -58,7 +62,7 @@ FormField* CreditCardField::Parse(AutofillScanner* scanner) {
       // for the cardholder's first and last name if they have the labels "cfnm"
       // and "clnm".
       scanner->SaveCursor();
-      const AutofillField* first;
+      AutofillField* first;
       if (ParseField(scanner, base::ASCIIToUTF16("^cfnm"), &first) &&
           ParseField(scanner,
                      base::ASCIIToUTF16("^clnm"),
@@ -91,8 +95,32 @@ FormField* CreditCardField::Parse(AutofillScanner* scanner) {
     }
 
     pattern = base::UTF8ToUTF16(autofill::kCardNumberRe);
-    if (!credit_card_field->number_ &&
-        ParseField(scanner, pattern, &credit_card_field->number_)) {
+    AutofillField* current_number_field;
+    if (ParseField(scanner, pattern, &current_number_field)) {
+      // Avoid autofilling any credit card number field having very low or high
+      // |start_index| on the HTML form.
+      size_t start_index = 0;
+      if (!credit_card_field->numbers_.empty()) {
+        size_t last_number_field_size =
+            credit_card_field->numbers_.back()->credit_card_number_offset() +
+            credit_card_field->numbers_.back()->max_length;
+
+        // In some cases, HTML form may have credit card number split across
+        // multiple input fields and either one or cumulatively having
+        // |max_length| more than |kMaxValidCardNumberSize|, mark these input
+        // form fields as invalid and skip autofilling them.
+        if (last_number_field_size == 0U ||
+            last_number_field_size >= kMaxValidCardNumberSize) {
+          // Mark that the credit card number splits are invalid. But keep
+          // scanning HTML form so that cursor moves beyond related fields.
+          credit_card_field->is_valid_card_number_split_ = false;

[Ilya Sherman, 2014/09/03 01:07:51]

Is it possible to recover from this parsing error?  If not, why not bail out
early here?

[Pritam Nikam, 2014/09/03 12:21:50]

I think, I didn't understand this completely.

Moreover, from little debugging, as i understand Parse-Classify cycle runs in
tandem, and  based on Scanner's cursor position it try to keep on parsing the
input fields on HTML form.
So in my experiments, if I try to break or return early (i.e. before parsing
rest of the fields related to credit card) it does still invoke
CreditCardField::Parse() followed by CreditCardField::ClassifyField() for rest
of the fields.

Also, I assume if number splits are invalid it's better not to autofill
expiration date and card-holder name as well.

Please correct me and guide what shall be the correct work-flow here.

[Ilya Sherman, 2014/09/03 23:25:26]

I don't see how this parsing error is different from any other failure, in which
we just rewind the scanner and return NULL.  Can you explain to me why this case
should be treated in a special way?  If not, please just clear the numbers_ list
and break out of the loop.

+        }
+
+        start_index = last_number_field_size;
+      }
+
+      current_number_field->set_credit_card_number_offset(start_index);
+      credit_card_field->numbers_.push_back(current_number_field);
       continue;
     }
 
@@ -175,7 +203,8 @@ FormField* CreditCardField::Parse(AutofillScanner* scanner) {
   // a strong enough signal that this is a credit card.  It is possible that
   // the number and name were parsed in a separate part of the form.  So if
   // the cvc and date were found independently they are returned.
-  if ((credit_card_field->number_ || credit_card_field->verification_) &&
+  if ((!credit_card_field->numbers_.empty() ||
+       credit_card_field->verification_) &&
       (credit_card_field->expiration_date_ ||
        (credit_card_field->expiration_month_ &&
         credit_card_field->expiration_year_))) {
@@ -190,16 +219,27 @@ CreditCardField::CreditCardField()
     : cardholder_(NULL),
       cardholder_last_(NULL),
       type_(NULL),
-      number_(NULL),
       verification_(NULL),
       expiration_month_(NULL),
       expiration_year_(NULL),
       expiration_date_(NULL),
-      exp_year_type_(CREDIT_CARD_EXP_DATE_4_DIGIT_YEAR) {
+      exp_year_type_(CREDIT_CARD_EXP_DATE_4_DIGIT_YEAR),
+      is_valid_card_number_split_(true) {
+}
+
+CreditCardField::~CreditCardField() {
 }
 
 bool CreditCardField::ClassifyField(ServerFieldTypeMap* map) const {
-  bool ok = AddClassification(number_, CREDIT_CARD_NUMBER, map);
+  // Bail-out autofilling for invalid credit card number splits.
+  if (!is_valid_card_number_split_)
+    return false;
+
+  bool ok = true;
+  for (size_t index = 0; index < numbers_.size(); ++index) {
+    ok = ok && AddClassification(numbers_.at(index), CREDIT_CARD_NUMBER, map);
+  }
+
   ok = ok && AddClassification(type_, CREDIT_CARD_TYPE, map);
   ok = ok &&
        AddClassification(verification_, CREDIT_CARD_VERIFICATION_CODE, map);