Issue 165171: Incorrect SyncDataType parsing for throttled types causes chrome to crash (originally titled “When Gmail is down, Chrome Sync crashes Chrome”)
Status: Fixed Owner: [email protected] Closed: Mar 2013 Type: Bug, Pri-1, Stability-Crash, OS-All
Reported by [email protected], Dec 10, 2012
Gmail is down; everyone’s Chrome is now crashing in the sync thread.
Comment #2 — paulir…@chromium.org, Dec 10, 2012
Full crash log from a user posted (a Chrome_SyncThread crash originating in syncer::SyncerProtoUtil::PostClientToServerMessage, with the underlying error std::__throw_out_of_range).
Comment #9 — [email protected], Dec 10, 2012
“I think this affected everything, but we should get a fix ASAP.” Labeled ReleaseBlock-Stable, Mstone-24.
Comment #21 — [email protected], Dec 10, 2012
So I think I figured out a problem, at least for the stack for 13a4c173601caec6.
syncer::anonymous namespace::ConvertErrorPBToLocalType() calls GetModelTypeFromSpecificsFieldNumber, which tries to convert a protobuf field number to an enum value. If the field number is unknown, it returns UNSPECIFIED == 0. But we use an EnumSet template instantiation which is defined only from FIRST_REAL_MODEL_TYPE (non-zero), i.e. enums in the set are converted to offsets from FIRST_REAL_MODEL_TYPE and stored in a bitset. So if UNSPECIFIED is passed to the set, it calculates a negative offset and tries to store that in a bitset, resulting in the out_of_range error.
So a few problems:
- That code path has NOTREACHED(), so we don’t have unit test coverage for this.
- We should use a separate function for “untrusted” input and handle the case appropriately.
- Why is the server sending down throttled types that are unknown to the client? Possibly because these are new types that aren’t known to the client yet (esp. stable ones).
Hopefully, we can figure out a server-side fix for #3 so we can stop triggering this code path on the client, and then we can fix the client after that.
Comment #22 — [email protected], Dec 10, 2012
We still don’t know (3). It’s interesting that the crash stopped happening - something decided to change the set of throttled datatypes sent down to clients specifically when the outage occurred.
Comment #23 — [email protected], Dec 10, 2012
We’ve confirmed the server is (for some reason) throttling all types in certain cases, including ones the stable client does not yet know about. So we should be able to at least do something server-side to prevent this from occurring. Fred’s point that the client isn’t handling the responses correctly though still stands.
Comment #26 — [email protected], Dec 10, 2012
Yes, in fact this crash would not happen if the sync server itself was unreachable. It’s due to a backend service that sync servers depend on becoming overwhelmed, and sync servers responding to that by telling all clients to throttle all data types (including data types that the client may not understand yet).
Comment #27 — [email protected], Dec 10, 2012
Renaming this bug as it really has nothing to do with GMail specifically.
To clarify:
- Chrome Sync Server relies on a backend infrastructure component to enforce quotas on per-datatype sync traffic.
- That quota service experienced traffic problems today due to a faulty load balancing configuration change.
- That change was to a core piece of infrastructure that many services at Google depend on. This means other services may have been affected at the same time, leading to the confounding original title of this bug.
- Because of the quota service failure, Chrome Sync Servers reacted too conservatively by telling clients to throttle “all” data types, without accounting for the fact that not all client versions support all data types.
The crash is due to faulty logic responsible for handling “throttled” data types on the client when the data types are unrecognized.
Summary changed to: Incorrect SyncDataType parsing for throttled types causes chrome to crash (was: When Gmail is down, Chrome Sync crashes Chrome)
Fix landed — [email protected], Dec 10, 2012
Revision r172232 by [email protected]: “[Sync] Handle invalid specifics field numbers gracefully. Change GetModelTypeFromSpecificsFieldNumber() to not NOTREACHED() on an unknown field number. Instead, have callers compare the return value to UNSPECIFIED and handle that case.” (BUG=165171)
Comment #35 — [email protected], Dec 11, 2012
Fix merged to branch 1312 (r172389) for release in Chrome M24.
Comment #37 — [email protected], Dec 13, 2012
Follow-up revision r172816: “[Sync] Add tests for invalid specifics field number handling. This is a follow up to r172232. Change NOTREACHED() to DLOG(WARNING), since the server sending down unknown/invalid field numbers is a valid event. Add tests for the code that uses GetModelTypeFromSpecificsFieldNumber(). Clean up some code in sync/engine/ a bit.”
Comment #38 — dhar…@google.com, Dec 13, 2012
Removing release blocker as we have a possible fix merged in M24.
Comment #42 — [email protected], Mar 21, 2013
This was resolved a while ago. Status changed to Fixed.