Closed Bug 1395703 Opened 5 years ago Closed 5 years ago

android.database.sqlite.SQLiteException: no such column: modifiedBySync

Categories

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

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Grisha, Assigned: Grisha)

References

Details

Attachments

(1 file)

Oops. Regression from Bug 1392802.

I/FxAccounts( 3612): fennec_grisha :: GlobalSession :: Running next stage syncBookmarks (org.mozilla.gecko.sync.stage.BookmarksServerSyncStage@a87571a8)...
D/dalvikvm( 3612): GC_CONCURRENT freed 2031K, 17% free 28310K/34055K, paused 2ms+1ms, total 15ms
D/dalvikvm( 3612): WAIT_FOR_CONCURRENT_GC blocked 13ms
D/dalvikvm( 3612): GC_CONCURRENT freed 2013K, 17% free 28320K/34055K, paused 0ms+1ms, total 7ms
D/dalvikvm( 3612): WAIT_FOR_CONCURRENT_GC blocked 6ms
D/dalvikvm( 3612): GC_CONCURRENT freed 2008K, 17% free 28353K/34055K, paused 0ms+0ms, total 10ms
D/dalvikvm( 3612): WAIT_FOR_CONCURRENT_GC blocked 9ms
E/SQLiteLog( 3612): (1) no such column: modifiedBySync
E/FxAccounts( 3612): fennec_grisha :: SessionHelper :: Store failed for ufCI06wcVADO
E/FxAccounts( 3612): android.database.sqlite.SQLiteException: no such column: modifiedBySync (code 1): , while compiling: UPDATE bookmarks SET position= ?,tags= ?,guid= ?,modifiedBySync= ?,title= ?,created= ?,description= ?,keyword= ?,parent= ?,type= ?,url= ?,modified= ?,localVersion = localVersion + 1 WHERE _id IN (502)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteConnection.nativePrepareStatement(Native Method)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:882)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:493)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:588)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:58)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:31)
E/FxAccounts( 3612): 	at android.database.sqlite.SQLiteDatabase.compileStatement(SQLiteDatabase.java:992)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.DBUtils.updateArraysWithOnConflict(DBUtils.java:405)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.DBUtils.updateArrays(DBUtils.java:280)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.BrowserProvider.updateAndIncrementLocalVersion(BrowserProvider.java:3157)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.BrowserProvider.updateBookmarks(BrowserProvider.java:1752)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.BrowserProvider.updateBookmarkByGuidAssertingLocalVersion(BrowserProvider.java:2711)
E/FxAccounts( 3612): 	at org.mozilla.gecko.db.BrowserProvider.call(BrowserProvider.java:2563)
E/FxAccounts( 3612): 	at android.content.ContentProvider$Transport.call(ContentProvider.java:256)
E/FxAccounts( 3612): 	at android.content.ContentResolver.call(ContentResolver.java:1029)
E/FxAccounts( 3612): 	at org.mozilla.gecko.sync.repositories.android.BookmarksDataAccessor.updateAssertingLocalVersion(BookmarksDataAccessor.java:130)
E/FxAccounts( 3612): 	at org.mozilla.gecko.sync.repositories.android.BookmarksSessionHelper.replaceWithAssertion(BookmarksSessionHelper.java:617)
E/FxAccounts( 3612): 	at org.mozilla.gecko.sync.repositories.android.BookmarksSessionHelper.doReplaceRecord(BookmarksSessionHelper.java:442)
E/FxAccounts( 3612): 	at org.mozilla.gecko.sync.repositories.android.SessionHelper$1.run(SessionHelper.java:317)
E/FxAccounts( 3612): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
E/FxAccounts( 3612): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
E/FxAccounts( 3612): 	at java.lang.Thread.run(Thread.java:856)
W/FxAccounts( 3612): fennec_grisha :: SynchronizerSession :: First RecordsChannel onFlowStoreFailed. Logging local store error.
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Priority: -- → P1
While processing bookmarks, we sometimes need to mark them for re-upload as we're inserting new ones or updating existing ones. For example, we might set or update a dateAdded field.

We perform insertions "in-bulk", and so we might be inserting some bookmarks which need to be re-uploaded, and some bookmarks which don't. We compile an array of ContentValue objects, and make a single call to our ContentProvider. This means we can't use a URI param to indicate our intent, and so a non-column field in ContentValues objects - modifiedFromSync - is set for those bookmarks which need special treatment during insertion.

Bug 1392802 added a similar mechanism for updating bookmarks. However, updates are done differently - currently, we perform a single call to our ContentProvider for each bookmark. Which means we _can_ use a URI field as a signaling mechanism, which is what that patch did. However, in haste I forgot to take into consideration existing signaling mechanism, which lead to insertion failures such as in Comment 0.

And so we're left with an even clumsier interface to our data store, with two ways to signal the same thing in different circumstances... A quick solution is to just make sure 'modifiedFromSync' field never makes its way to contentprovider on updates; a more refined fix would probably modify update logic to use a ContentValues field for consistency... Either way, there's going to be something ugly, somewhere in the code.

I anticipate a lot of this code changing sometime soon in order to support better transactionality of bookmark syncing, and smarter merging, and so I'm inclined to just to the simple thing for now.
Comment on attachment 8903349 [details]
Bug 1395703 - Make sure modifiedBySync CV field isn't passed to ContentProvider on updates

https://reviewboard.mozilla.org/r/175146/#review180192

::: commit-message-4984d:1
(Diff revision 1)
> +Bug 1395703 - Make sure modifiedFromSync CV field isn't passed to ContentProvider on updates r=rnewman

modifiedBySync, throughout the commit message.
Attachment #8903349 - Flags: review?(rnewman) → review+
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b18cad4e3274
Make sure modifiedBySync CV field isn't passed to ContentProvider on updates r=rnewman
https://hg.mozilla.org/mozilla-central/rev/b18cad4e3274
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.