Explode Autofill sync error messages for debugging

Explode Autofill sync error messages for debugging

BUG=121592
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137572

[Ilya Sherman, 2012-05-11 00:22:56]

This adds logging for each possible source of errors, though I'm not sure how to
upload these error messages to the crash server -- how should I go about doing
that?

[tim (not reviewing), 2012-05-14 22:50:08]

http://codereview.chromium.org/10383127/diff/1/chrome/browser/sync/glue/gener...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

http://codereview.chromium.org/10383127/diff/1/chrome/browser/sync/glue/gener...
chrome/browser/sync/glue/generic_change_processor.cc:147: NOTREACHED();
*extra* unreachable :) One NR() might suffice, but okay either way.

http://codereview.chromium.org/10383127/diff/1/chrome/browser/sync/glue/gener...
chrome/browser/sync/glue/generic_change_processor.cc:204:
error_handler()->OnSingleDatatypeUnrecoverableError(error.location(),
So, unfortunately this is actually the line that will create the
NONCONTINUABLE_EXCEPTION that gets uploaded to crash/.  So what you want to do
is likely have a different loc depending on the error that happened within
AttemptDelete -- so instead of a message string, you may want an enum or extra
bool param.

[Ilya Sherman, 2012-05-15 00:22:15]

https://chromiumcodereview.appspot.com/10383127/diff/1/chrome/browser/sync/gl...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://chromiumcodereview.appspot.com/10383127/diff/1/chrome/browser/sync/gl...
chrome/browser/sync/glue/generic_change_processor.cc:147: NOTREACHED();
On 2012/05/14 22:50:08, timsteele wrote:
> *extra* unreachable :) One NR() might suffice, but okay either way.

Done.

https://chromiumcodereview.appspot.com/10383127/diff/1/chrome/browser/sync/gl...
chrome/browser/sync/glue/generic_change_processor.cc:204:
error_handler()->OnSingleDatatypeUnrecoverableError(error.location(),
On 2012/05/14 22:50:08, timsteele wrote:
> So, unfortunately this is actually the line that will create the
> NONCONTINUABLE_EXCEPTION that gets uploaded to crash/.  So what you want to do
> is likely have a different loc depending on the error that happened within
> AttemptDelete -- so instead of a message string, you may want an enum or extra
> bool param.

Ok, assuming I've understood things correctly, I believe this should be fixed in
the latest patch set.  PTAL :)

[tim (not reviewing), 2012-05-15 16:44:12]

http://codereview.chromium.org/10383127/diff/4/chrome/browser/sync/glue/gener...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

http://codereview.chromium.org/10383127/diff/4/chrome/browser/sync/glue/gener...
chrome/browser/sync/glue/generic_change_processor.cc:202: SyncError error =
AttemptDelete(change, type, type_str, &sync_node);
This still doesn't quite do it :P Think of OnSingleDataTypeUnrecoverableError as
a CHECK statement.  The location parameter is only used for local logging; what
matters is the actual line of code that OnSingleDatatypeUnrecoverableError is
itself called on.

[Ilya Sherman, 2012-05-16 00:43:06]

https://chromiumcodereview.appspot.com/10383127/diff/4/chrome/browser/sync/gl...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://chromiumcodereview.appspot.com/10383127/diff/4/chrome/browser/sync/gl...
chrome/browser/sync/glue/generic_change_processor.cc:202: SyncError error =
AttemptDelete(change, type, type_str, &sync_node);
On 2012/05/15 16:44:12, timsteele wrote:
> This still doesn't quite do it :P Think of OnSingleDataTypeUnrecoverableError
as
> a CHECK statement.  The location parameter is only used for local logging;
what
> matters is the actual line of code that OnSingleDatatypeUnrecoverableError is
> itself called on.

*sigh* ok, new patch set up... lots of repeated code, but if that's what it
takes, so be it :)

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

Now we're talking.  I'm with you on the duplication though, had a comment about
that.

Thanks a lot for seeing this through!

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
chrome/browser/sync/glue/generic_change_processor.cc:162: case
sync_api::BaseNode::INIT_FAILED_ENTRY_IS_DEL:
You need to add good old break/return statements in this switch.

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
chrome/browser/sync/glue/generic_change_processor.cc:201: case
sync_api::BaseNode::INIT_FAILED_ENTRY_NOT_GOOD:
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:29]

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
chrome/browser/sync/glue/generic_change_processor.cc:201: case
sync_api::BaseNode::INIT_FAILED_ENTRY_NOT_GOOD:
On 2012/05/16 17:19:37, timsteele wrote:
> 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.

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]

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
File chrome/browser/sync/glue/generic_change_processor.cc (right):

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
chrome/browser/sync/glue/generic_change_processor.cc:162: case
sync_api::BaseNode::INIT_FAILED_ENTRY_IS_DEL:
On 2012/05/16 17:19:37, timsteele wrote:
> You need to add good old break/return statements in this switch.

Done.

https://chromiumcodereview.appspot.com/10383127/diff/5005/chrome/browser/sync...
chrome/browser/sync/glue/generic_change_processor.cc:201: case
sync_api::BaseNode::INIT_FAILED_ENTRY_NOT_GOOD:
On 2012/05/16 17:19:37, timsteele wrote:
> 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.

Good call, done.

[tim (not reviewing), 2012-05-16 22:19:18]

LGTM!

[commit-bot: I haz the power, 2012-05-16 22:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/10383127/11003

[commit-bot: I haz the power, 2012-05-17 00:09:16]

Change committed as 137572