629720 - Client HMAC errors cause CrossWeave to fail with recent error handling changes

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

We now treat *all* errors during record application as failures, and set _syncError to true. That means old CrossWeave profiles with 'bad' client records (from v4 days) will start failing tests.

  Short term: don't set _syncError.

  Medium term: fix said CrossWeave profiles.

  Long term: make clients engine more robust against bad client records.

Patch for the first coming soon.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 1. v1

This makes CrossWeave complete.

Left line commented, because we'll be reinstating it shortly...

Comment 2

[Philipp von Weitershausen [:philikon]]

I've changed my mind about the short term here. I think it's perfectly fine to bubble those up to the service and UI.

Crossweave's profiles are quite a special case with billions of client records, some of which are eons old from when we had slightly busted crypto etc.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 507899 [details] [diff] [review]
Part 1. v1

If we did anything, we shouldn't do this.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> If we did anything, we shouldn't do this.

To elaborate: never setting Service._syncError would mean the user never got to see an error. I think we eventually want to show errors, otherwise the user won't know that Sync wasn't able to save everything. If we wanted to special-case HMAC failures, we should do that in engines.js when we add those records to 'failed'.

But really, I don't think we should special-case them at all. HMAC failures we can't recover from are likely to be real client bugs that we want to know about.

We just need to fix Crossweave's pathological client record handling and the fact that it aborts w/o logs on a sync error.

Comment 5

[Richard Newman [:rnewman]]

WONTFIXing, because it sounds like the final answer is "start a new CrossWeave
profile", and "make CrossWeave clean up after itself".

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> WONTFIXing, because it sounds like the final answer is "start a new CrossWeave
> profile", and "make CrossWeave clean up after itself".

Indeed. And make Sync clean up after itself (bug 565430). Also, all clients records we upload with 4.0b10+ and Sync 1.7+ will have a TTL, so they'll just expire.