Closed Bug 1392716 Opened 3 years ago Closed 3 years ago

Crash in java.lang.IllegalStateException: Expected to modify syncVersion for a guid, but did not at org.mozilla.gecko.db.BrowserProvider.bulkUpdateSyncVersions(BrowserProvider.java)

Categories

(Firefox for Android :: Android Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

Assertions split out from Bug 1392078 to help investigate root cause.
Summary: java.lang.IllegalStateException: Expected to modify syncVersion for a guid, but did not at org.mozilla.gecko.db.BrowserProvider.bulkUpdateSyncVersions(BrowserProvider.java) → Crash in java.lang.IllegalStateException: Expected to modify syncVersion for a guid, but did not at org.mozilla.gecko.db.BrowserProvider.bulkUpdateSyncVersions(BrowserProvider.java)
Depends on: 1388884
Duplicate of this bug: 1393104
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8900534 [details]
Bug 1392716 - Clean up version map while de-duping records

https://reviewboard.mozilla.org/r/171930/#review177148

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/VersioningDelegateHelper.java:182
(Diff revision 1)
>          @Override
> -        public void onRecordStoreReconciled(String guid, Integer newVersion) {
> +        public void onRecordStoreReconciled(String guid, String oldGuid, Integer newVersion) {
>              if (newVersion == null) {
>                  throw new IllegalArgumentException("Received null localVersion after reconciling a versioned record.");
>              }
> +            localVersionsOfGuids.remove(oldGuid);

OK, so what we think happened:

- We download and apply a record, X.
- We download and apply a record, Z.
- Z dupes to X. We rename X to Z. Here we put Z into the map.
- We download and apply Y.
- Y dupes to Z. We rename Z to Y, and put Y into the map.

The DB now contains Y, and the map contains Y and Z. Z doesn't exist to update, so we crash.

This fix is to remove a GUID from the map each time we rename it.

Could you document this?

A deeper bug is that we are duping statelessly: records match against other records earlier in the sync.
Attachment #8900534 - Flags: review?(rnewman) → review+
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aae46de1a7cf
Clean up version map while de-duping records r=rnewman
https://hg.mozilla.org/mozilla-central/rev/aae46de1a7cf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Looking at the crash stats now, in total we had 2 crashes on builds which include patch in Comment 7. So, quite low volume, which matches the "user deleted a bookmark during a sync, and we wiped a tombstone at exactly the wrong time" hypothesis proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1392078#c12. There were no crashes (yet!) since Bug 1388884 landed (which fixes this flaw), so keeping fingers crossed this just goes away entirely.

But, given the very low crash volume, we won't know for sure until 57 hits beta.
Product: Android Background Services → Firefox for Android
Looking at the crash graphs, there remained a low volume of crashes with a matching signature in 57 and 58 (betas) - coming from about a 100 or so installs.

This matches the "user did something at an inopportune moment to cause it" theory.
You need to log in before you can comment on or make changes to this bug.