[Sync] - Upload the callstacks for errors so that the line number of error is in callstack.

For legacy types(that are not in sync api) we uploaded the call stacks after the error has happened.

To solve this we introduce a new method called CreateAndUploadError and eventually this would be the only method than can create a non default SyncError. (Non legacy types have to be changed before we make this constructor private). When the error is created we upload from there as well.

This gives us the following benefits:
1. We have the line number that caused the error in call stack.
2. Once this is fully implemented we dont have the possibility of double uploading of the same error.

BUG=
TEST=


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

[lipalani1, 2012-04-05 00:00:17]

Please review.
I have changed AssociatorInterface because of which all the classes that
inherited from it had to be changed.(these are legacy types. newer types dont
inherit from this)

Zea - Changes to the interface, Non Frontend DTC, frontend DTC and associated
tests, passwords, themes, sessions.

Tim - Bookmarks

Drew - typed urls.

[Andrew T Wilson (Slow), 2012-04-05 00:33:19]

LGTM with some nits.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:269:
&pending_updated_urls_,
I don't think this is valid indentation style - either wrap all arguments
(including the first) or have all arguments align with open paren. I could be
wrong though - if this is valid style, feel free to leave it.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_model_associator.cc (right):

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:144: "Could not get the
typed_url entries.",
Yeah, pretty sure this type of indentation doesn't match the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:316: if (error.IsSet()) {
Hmmm. Seems like we're no longer reporting this location as the source of the
error as we were currently. This means that if we hit an error in
UpdateFromSyncDB() we won't know the source of the callsite. Maybe we should
create and upload errors from both places, but return this error to the caller
for display in about:sync?

What do you think?

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:483:
"WriteToHistoryBackend failed",
Can we change this to "UpdateURL() failed"? I would ideally like to log
information about the URL itself, but I suspect that'd be a privacy violation.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:498:
"WriteToHistoryBackend failed",
Would be nice to say something like "AddVisits() failed" for diagnosing errors
via about:sync.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:508:
"WriteToHistoryBackend failed",
Ditto here: "RemoveVisits failed"

[Nicolas Zea, 2012-04-05 19:09:03]

How are you planning on using this with newer datatypes?

[Nicolas Zea, 2012-04-05 22:08:58]

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_controller.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_controller.cc:25:
ChromeReportUnrecoverableError();
This probably needs to be removed now. We'll be uploading an error when a
SyncError is made, and then calling RecordUnrecoverableError either form
OnSingleDatatype or from the DTC's Associate method.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_error_handler.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_error_handler.h:26: virtual SyncError
CreateAndUploadError(
Add comment about what this does.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:13: class SyncError;
Remove this and add #include (no longer just using a pointer)

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc:169:
EXPECT_CALL(*model_associator_, DisassociateModels()).
curly braces for multiline if

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_change_processor.cc:230: // Needing to
reassociate would have set the error already so clear it.
remove comment.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:542: if (error.IsSet())
if (!UpdateAssociations...) {
  DCHECK(error.IsSet());
  return error;
}

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:561:
UpdateSyncModelDataFromClient(&error);
same here

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:566: return error;
DCHECK(!error.IsSet());
return SyncError();

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:940: bool
SessionModelAssociator::UpdateSyncModelDataFromClient(
undo newline?

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.h:136: // |error| gets set if
any association error occurred.
remove comment (here and DisassociateModels)

[lipalani1, 2012-04-05 22:51:03]

I have addressed both Drew and Nicolas's comments. In retrospect I should have
sent seperate emails. If you have difficulty untangling feel free to ping me and
I will seperate it out.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_change_processor.cc (right):

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_change_processor.cc:269:
&pending_updated_urls_,
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> I don't think this is valid indentation style - either wrap all arguments
> (including the first) or have all arguments align with open paren. I could be
> wrong though - if this is valid style, feel free to leave it.

Done.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
File chrome/browser/sync/glue/typed_url_model_associator.cc (right):

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:144: "Could not get the
typed_url entries.",
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> Yeah, pretty sure this type of indentation doesn't match the style guide:
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:316: if (error.IsSet()) {
When we upload we have no problems because this line would be in call stack.

Are you worried about about:sync not displaying this error? One possibility
would be to get the call stack by going to about:crash. But it is not reliable
as it is possible that the user has disabled crash reporting.

Creating 2 errors and uploading seems wasteful and TPMs are already concerned
about the number of errors.

The only thing I could think of is having a method to change the error message
from here. Let me know what you think. 
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> Hmmm. Seems like we're no longer reporting this location as the source of the
> error as we were currently. This means that if we hit an error in
> UpdateFromSyncDB() we won't know the source of the callsite. Maybe we should
> create and upload errors from both places, but return this error to the caller
> for display in about:sync?
> 
> What do you think?

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:483:
"WriteToHistoryBackend failed",
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> Can we change this to "UpdateURL() failed"? I would ideally like to log
> information about the URL itself, but I suspect that'd be a privacy violation.

Done.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:498:
"WriteToHistoryBackend failed",
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> Would be nice to say something like "AddVisits() failed" for diagnosing errors
> via about:sync.

Done.

http://codereview.chromium.org/9978017/diff/2001/chrome/browser/sync/glue/typ...
chrome/browser/sync/glue/typed_url_model_associator.cc:508:
"WriteToHistoryBackend failed",
On 2012/04/05 00:33:19, Andrew T Wilson wrote:
> Ditto here: "RemoveVisits failed"

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_controller.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_controller.cc:25:
ChromeReportUnrecoverableError();
This is needed in the interim. We will remove it in the next patch (Good eye
btw!) Eventually this method will take SyncError as its only param and do only
UMA reporting. Here is the reason:
we have 4 cases:
Legacy model association: If model association failed then we return
ASSOCIATION_FAILED and for that we will not call RecordUnrecoverableError. We
will only call if it is an unrecoverable error. Look at StartFailed in
NonFrontendDTC and frontendDTC. No UMA will also be reported because of this
change but we will fix that in the next CL. The next CL would simply make this
method take SyncError as a param and will only record the UMA.
Legacy ChangeProcessor: They all simply call OnSingleDatatypeUnrecoverableError
presently which will keep uploading. We will change this method to take in a
SyncError in the next patch and the datatypes would have to call
CreateAndUploadError before calling OnSingleDatatypeUnreocverableError. 
New model associators (mergedataandstartsyncing): We will temporarily not upload
any errors to breakpad(look at StartFailed). But we will fix it in the next cl
when we make them take the interface we spoke about.
New Change processors(part of syncable service) : They presently call
OnSingleDatatypeunrecoverableError which will upload. In the future we will make
them create the error before calling that method.

In Short the new StartFailed would look like the following:
if (error.IsSet()) RecordUnrecoverableError(error);
...(the same code as we have today)

let me know if you have concerns.
On 2012/04/05 22:08:58, nzea wrote:
> This probably needs to be removed now. We'll be uploading an error when a
> SyncError is made, and then calling RecordUnrecoverableError either form
> OnSingleDatatype or from the DTC's Associate method.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_error_handler.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_error_handler.h:26: virtual SyncError
CreateAndUploadError(
On 2012/04/05 22:08:58, nzea wrote:
> Add comment about what this does.

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:13: class SyncError;
On 2012/04/05 22:08:58, nzea wrote:
> Remove this and add #include (no longer just using a pointer)

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc:169:
EXPECT_CALL(*model_associator_, DisassociateModels()).
On 2012/04/05 22:08:58, nzea wrote:
> curly braces for multiline if

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_change_processor.cc:230: // Needing to
reassociate would have set the error already so clear it.
On 2012/04/05 22:08:58, nzea wrote:
> remove comment.

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:542: if (error.IsSet())
On 2012/04/05 22:08:58, nzea wrote:
> if (!UpdateAssociations...) {
>   DCHECK(error.IsSet());
>   return error;
> }

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:561:
UpdateSyncModelDataFromClient(&error);
On 2012/04/05 22:08:58, nzea wrote:
> same here

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:566: return error;
On 2012/04/05 22:08:58, nzea wrote:
> DCHECK(!error.IsSet());
> return SyncError();

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:940: bool
SessionModelAssociator::UpdateSyncModelDataFromClient(
On 2012/04/05 22:08:58, nzea wrote:
> undo newline?

Done.

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.h:136: // |error| gets set if
any association error occurred.
On 2012/04/05 22:08:58, nzea wrote:
> remove comment (here and DisassociateModels)

Done.

[Nicolas Zea, 2012-04-06 19:49:47]

LGTM with some nits and a question. Also, keep in mind
SyncModelHasUserCreatedNodes will need to be updated as well (but that can be in
a later patch).

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_controller.cc (right):

http://codereview.chromium.org/9978017/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_controller.cc:25:
ChromeReportUnrecoverableError();
On 2012/04/05 22:51:03, lipalani1 wrote:
> This is needed in the interim. We will remove it in the next patch (Good eye
> btw!) Eventually this method will take SyncError as its only param and do only
> UMA reporting. Here is the reason:
> we have 4 cases:
> Legacy model association: If model association failed then we return
> ASSOCIATION_FAILED and for that we will not call RecordUnrecoverableError. We
> will only call if it is an unrecoverable error. Look at StartFailed in
> NonFrontendDTC and frontendDTC. No UMA will also be reported because of this
> change but we will fix that in the next CL. The next CL would simply make this
> method take SyncError as a param and will only record the UMA.
> Legacy ChangeProcessor: They all simply call
OnSingleDatatypeUnrecoverableError
> presently which will keep uploading. We will change this method to take in a
> SyncError in the next patch and the datatypes would have to call
> CreateAndUploadError before calling OnSingleDatatypeUnreocverableError. 
> New model associators (mergedataandstartsyncing): We will temporarily not
upload
> any errors to breakpad(look at StartFailed). But we will fix it in the next cl
> when we make them take the interface we spoke about.
> New Change processors(part of syncable service) : They presently call
> OnSingleDatatypeunrecoverableError which will upload. In the future we will
make
> them create the error before calling that method.
> 
> In Short the new StartFailed would look like the following:
> if (error.IsSet()) RecordUnrecoverableError(error);
> ...(the same code as we have today)
> 
> let me know if you have concerns.
> On 2012/04/05 22:08:58, nzea wrote:
> > This probably needs to be removed now. We'll be uploading an error when a
> > SyncError is made, and then calling RecordUnrecoverableError either form
> > OnSingleDatatype or from the DTC's Associate method.
> 

Sounds good. Could you add a TODO mentioning that this needs to be removed (here
and in the OnSingleDatatypeUnrecoverable).

http://codereview.chromium.org/9978017/diff/9004/chrome/browser/sync/glue/boo...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

http://codereview.chromium.org/9978017/diff/9004/chrome/browser/sync/glue/boo...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:211: "error",
fix indent for these two lines

http://codereview.chromium.org/9978017/diff/9004/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_controller.cc (right):

http://codereview.chromium.org/9978017/diff/9004/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_controller.cc:13: return (result ==
UNRECOVERABLE_ERROR);
why is this changing btw?

[commit-bot: I haz the power, 2012-04-10 05:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9978017/19034

[commit-bot: I haz the power, 2012-04-10 07:28:19]

Change committed as 131544