Closed Bug 1292359 Opened 3 years ago Closed 3 years ago

java.lang.ClassCastException during history sync

Categories

(Firefox for Android :: Android Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

08-04 15:00:45.900 2371-4837/org.mozilla.fennec_grisha E/FxAccounts: fennec_grisha :: BrowserRepoSession :: Store failed for WknGb4IXKl4x

java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Long
at org.mozilla.gecko.sync.repositories.android.VisitsHelper.getVisitContentValues(VisitsHelper.java:110)
at org.mozilla.gecko.sync.repositories.android.VisitsHelper.getVisitsContentValues(VisitsHelper.java:45)
at org.mozilla.gecko.sync.repositories.android.AndroidBrowserHistoryDataAccessor.update(AndroidBrowserHistoryDataAccessor.java:96)
at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession.replace(AndroidBrowserRepositorySession.java:574)
at org.mozilla.gecko.sync.repositories.android.AndroidBrowserRepositorySession$1.run(AndroidBrowserRepositorySession.java:514)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
at java.lang.Thread.run(Thread.java:818)
The problem here is one of mixed data types. HistoryRecord and everything that deals with it treats visit types as Long. In the local database, visit type is a TINYINT, and when we're reading visits from the database, we cast them to Integer. When we combine mixed data types into one big untyped JSONArray, bad things happen.

Details:
A problem occurs during sync, when an incoming history record has a local equivalent. We "reconcile" the two records and store the result. Two sets of visits are merged. All of incoming record's visit types are long (due to how HistoryRecords and friends work), and local record's visit types are Integers (due to how we populate local record with visits during reconciliation; visit types are read from the database and cast to Integers; see [0], [1] and [2]). Once history records are reconciled, they are inserted - and their visits are "bulk-inserted". To insert them, we need to generate an array of ContentValues, and that is where failure occurs. Method which generates ContentValues array (see [3]) assumes visit types in the JSONArray will be Long, yet they're a mixed bag of both Longs and Integers.

To replicate this scenario, simply visit the same website a few times on a desktop and mobile clients, then sync desktop first, then sync mobile - in logcat you'll see failures to store incoming history records.

Two ways to fix this:
1) ensure that a visit JSONObject in HistoryRecord's visits JSONArray always has a "type" key whose value is an Integer, instead of a Long as it is now. Visit type is really just a tinyint (1,2,3,4,5..), so I don't think it should have been a Long in the first place. This will involve changes to HistoryRecords, any (non-unit) tests around it, and possible code that touches HistoryRecord.visits (field declared as public...).

2) Another solution is to just cast visit types to Long instead of Integer when we read them from the database to create a "recent visits" ContentValues array (in [2]).

Option 1 feels more correct, but a) there is probably some weird reason visit types were treated as Longs in the first place, b) due to how this stuff is written/tested changes require much attention and it's possible to introduce new problems.
Option 2 is very simple and that code is much less entrenched and changes to it are unlikely to cause other problems.

Either option gets us to consistency in how we treat visit types (all as Longs, or all as Integers), so I'm going to pick the most straight forward solution (2) and just fix the newer code in VisitsHelper class.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java#80
[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java#495
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java#81
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java#110
In general, all of the services Fennec code treats all integers as long (lower case long).  The reason is that mostly that simple-json and SharedPreferences treat Integer and Long as distinct, including when comparing in tests; it's frustrating.  So read things as long out of the DB, etc.  (Remember, our timestamps don't fit in int...)
(In reply to Nick Alexander :nalexander from comment #2)
> In general, all of the services Fennec code treats all integers as long
> (lower case long).  The reason is that mostly that simple-json and
> SharedPreferences treat Integer and Long as distinct, including when
> comparing in tests; it's frustrating.  So read things as long out of the DB,
> etc.  (Remember, our timestamps don't fit in int...)

Thanks for clarifying, Nick!
Comment on attachment 8778431 [details]
Bug 1292359 - Treat visit type consistently as a Long while syncing

https://reviewboard.mozilla.org/r/69744/#review66852
Attachment #8778431 - Flags: review?(nalexander) → review+
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc48f4abbe03
Treat visit type consistently as a Long while syncing r=nalexander
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Blocks: 1265516
Buggy behaviour landed in 48; requesting uplift for this to both beta & aurora, if possible.
 
[Feature/regressing bug #]: Bug 1046709
[User impact if declined]: While syncing, we will fail to store incoming updates to history records on Fennec under certain conditions (same website was visited on the local client and from other clients), potentially loosing some history visits.
[Describe test coverage new/current, TreeHerder]: Updated unit tests to ensure modified behaviour; manual testing of the failing scenario after changes.
[Risks and why]: Hopefully low. Change itself is constrained and simple, and the root cause is well understood.
[String/UUID change made/needed]: none
Attachment #8778503 - Flags: approval-mozilla-beta?
Attachment #8778503 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cc48f4abbe03
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8778503 [details] [diff] [review]
visit-types.patch

Review of attachment 8778503 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a sync issue in Fennec. Let's take it in 49 beta and aurora.
Attachment #8778503 - Flags: approval-mozilla-beta?
Attachment #8778503 - Flags: approval-mozilla-beta+
Attachment #8778503 - Flags: approval-mozilla-aurora?
Attachment #8778503 - Flags: approval-mozilla-aurora+
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.