Explode Autofill sync error messages for debugging

chrome/browser/sync/glue/generic_change_processor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/glue/generic_change_processor.h"
 
 #include "base/location.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/sync/api/sync_change.h"
@@ skipping 110 matching lines @@
     }
     current_sync_data->push_back(SyncData::CreateRemoteData(
         sync_child_node.GetId(), sync_child_node.GetEntitySpecifics()));
     sync_child_id = sync_child_node.GetSuccessorId();
   }
   return SyncError();
 }
 
 namespace {
 
-bool AttemptDelete(const SyncChange& change, sync_api::WriteNode* node) {
+// TODO(isherman): Investigating http://crbug.com/121592
+SyncError AttemptDelete(const SyncChange& change,
+                        syncable::ModelType type,
+                        std::string type_str,
+                        sync_api::WriteNode* node,
+                        DataTypeErrorHandler* error_handler) {
   DCHECK_EQ(change.change_type(), SyncChange::ACTION_DELETE);
   if (change.sync_data().IsLocal()) {
     const std::string& tag = change.sync_data().GetTag();
     if (tag.empty()) {
-      return false;
+      SyncError error(
+          FROM_HERE,
+          "Failed to delete " + type_str + " node. Local data, empty tag.",
+          type);
+      error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                        error.message());
+      return error;
     }
-    if (node->InitByClientTagLookup(
-            change.sync_data().GetDataType(), tag) !=
-            sync_api::BaseNode::INIT_OK) {
-      return false;
+
+    sync_api::BaseNode::InitByLookupResult result =
+        node->InitByClientTagLookup(change.sync_data().GetDataType(), tag);
+    if (result != sync_api::BaseNode::INIT_OK) {
+      SyncError error;
+      switch (result) {
+        case sync_api::BaseNode::INIT_FAILED_ENTRY_NOT_GOOD:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Local data, "
+                          "could not find entry matching the lookup criteria.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_ENTRY_IS_DEL:

[tim (not reviewing), 2012/05/16 17:19:37]

You need to add good old break/return statements in this switch.

[Ilya Sherman, 2012/05/16 21:10:01]

Done.

+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Local data, "
+                          "entry is already deleted.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_DECRYPT_IF_NECESSARY:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Local data, "
+                          "unable to decrypt",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_PRECONDITION:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Local data, "
+                          "a precondition was not met for calling init.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        default:
+          // Should have listed all the possible error cases above.
+          NOTREACHED();
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Local data, "
+                          "unknown error",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+      }
+      return error;
     }
   } else {
-    if (node->InitByIdLookup(change.sync_data().GetRemoteId()) !=
-            sync_api::BaseNode::INIT_OK) {
-      return false;
+    sync_api::BaseNode::InitByLookupResult result =
+        node->InitByIdLookup(change.sync_data().GetRemoteId());
+    if (result != sync_api::BaseNode::INIT_OK) {
+      SyncError error;
+      switch (result) {
+        case sync_api::BaseNode::INIT_FAILED_ENTRY_NOT_GOOD:

[tim (not reviewing), 2012/05/16 17:19:37]

One possible way we could reduce the code explosion - factor this switch into a
method taking an InitByLookupResult and invoke it from each place.  The stack
will still show us what path we took.  

We could also then / later use it below, in the ProcessSyncChanges case.

[tim (not reviewing), 2012/05/16 17:23:30]

Doh, I guess that will neuter the FROM_HERE in the SyncError  unless you pass
the location in. That might be okay, the error string should be enough to
differentiate if we actually had our hands on a real local repro.

[Ilya Sherman, 2012/05/16 21:10:01]

Good call, done.

+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Non-local data, "
+                          "could not find entry matching the lookup criteria.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_ENTRY_IS_DEL:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Non-local data, "
+                          "entry is already deleted.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_DECRYPT_IF_NECESSARY:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Non-local data, "
+                          "unable to decrypt",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        case sync_api::BaseNode::INIT_FAILED_PRECONDITION:
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Non-local data, "
+                          "a precondition was not met for calling init.",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+        default:
+          // Should have listed all the possible error cases above.
+          NOTREACHED();
+          error.Reset(FROM_HERE,
+                      "Failed to delete " + type_str + " node. Non-local data, "
+                          "unknown error",
+                      type);
+          error_handler->OnSingleDatatypeUnrecoverableError(error.location(),
+                                                            error.message());
+      }
+      return error;
     }
   }
   node->Remove();
-  return true;
+  return SyncError();
 }
 
 }  // namespace
 
 SyncError GenericChangeProcessor::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const SyncChangeList& list_of_changes) {
   DCHECK(CalledOnValidThread());
   sync_api::WriteTransaction trans(from_here, share_handle());
 
   for (SyncChangeList::const_iterator iter = list_of_changes.begin();
        iter != list_of_changes.end();
        ++iter) {
     const SyncChange& change = *iter;
     DCHECK_NE(change.sync_data().GetDataType(), syncable::UNSPECIFIED);
     syncable::ModelType type = change.sync_data().GetDataType();
     std::string type_str = syncable::ModelTypeToString(type);
     sync_api::WriteNode sync_node(&trans);
     if (change.change_type() == SyncChange::ACTION_DELETE) {
-      if (!AttemptDelete(change, &sync_node)) {
+      SyncError error = AttemptDelete(change, type, type_str, &sync_node,
+                                      error_handler());
+      if (error.IsSet()) {
         NOTREACHED();
-        SyncError error(FROM_HERE,
-                        "Failed to delete " + type_str + " node.",
-                        type);
-        error_handler()->OnSingleDatatypeUnrecoverableError(error.location(),
-                                                            error.message());
         return error;
       }
     } else if (change.change_type() == SyncChange::ACTION_ADD) {
       // TODO(sync): Handle other types of creation (custom parents, folders,
       // etc.).
       sync_api::ReadNode root_node(&trans);
       if (root_node.InitByTagLookup(
               syncable::ModelTypeToRootTag(change.sync_data().GetDataType())) !=
                   sync_api::BaseNode::INIT_OK) {
         NOTREACHED();
@@ skipping 148 matching lines @@
 void GenericChangeProcessor::StopImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 sync_api::UserShare* GenericChangeProcessor::share_handle() const {
   DCHECK(CalledOnValidThread());
   return share_handle_;
 }
 
 }  // namespace browser_sync