Closed Bug 1214193 Opened 9 years ago Closed 9 years ago

Deal with unknown incoming tombstones in history data adapter

Categories

(Firefox OS Graveyard :: Sync, defect, P2)

Product:
Firefox OS Graveyard
Component:
Sync
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
FxOS-S10 (30Oct)

People

(Reporter: mbdejong, Assigned: mbdejong)

References

Details

(Whiteboard: [partner-cherry-pick])

Attachments

(1 file)

[gaia] michielbdejong:1214193-incoming-tombstones > mozilla-b2g:master
46 bytes, text/x-github-pull-request
selee
: review+
ferjm
: review+
selee
: feedback+
Details | Review
It seems to me that asyncStorage.getItem in https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L64 will return null if a tombstone is imported on initial sync. We should check for that case and not try to delete the record with id null from the DataStore.
Assignee: nobody → mbdejong
Summary: Deal with unknown incoming tombstones → Deal with unknown incoming tombstones in history data adapter
Sean, do you agree?
Flags: needinfo?(selee)
The ID got from getItem will be used here to remove a record: https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L156 Agree with you that it should not try to remove a record of 'null' ID.
Flags: needinfo?(selee)
In the bookmarks adapter, a similar bug exists - when an unknown tombstone comes in it does console.warn [1], but still tries to continue. I'll add tests and fixes for both history and bookmarks. [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/bookmarks.js#L149-L151
Target Milestone: --- → FxOS-S9 (16Oct)
Attachment #8674109 - Flags: feedback?(selee)
Needs a unit test, still.
Comment on attachment 8674109 [details] [review] [gaia] michielbdejong:1214193-incoming-tombstones > mozilla-b2g:master Thank you, Michiel!
Attachment #8674109 - Flags: feedback?(selee) → feedback+
Hey Michiel, I think I can review this patch if you needed. :)
Blocks: 1195647
Target Milestone: FxOS-S9 (16Oct) → FxOS-S10 (30Oct)
Priority: -- → P2
Attachment #8674109 - Flags: review?(selee)
Comment on attachment 8674109 [details] [review] [gaia] michielbdejong:1214193-incoming-tombstones > mozilla-b2g:master Thank you! LGTM :)
Attachment #8674109 - Flags: review?(selee) → review+
Rebased on master.
Flags: needinfo?(selee)
LGTM! Thanks :)
Flags: needinfo?(selee)
Updated the console warning message to be more precise about what is happening.
Comment on attachment 8674109 [details] [review] [gaia] michielbdejong:1214193-incoming-tombstones > mozilla-b2g:master Already got r+ from Sean before (changed console warning message after that).
Attachment #8674109 - Flags: review?(ferjmoreno)
Comment on attachment 8674109 [details] [review] [gaia] michielbdejong:1214193-incoming-tombstones > mozilla-b2g:master LGTM. There's a failing test though.
Attachment #8674109 - Flags: review?(ferjmoreno) → review+
Turned out that test was already broken in code, but we weren't seeing it in the test. Fixed and split it into two commits.
Flags: needinfo?(ferjmoreno)
https://github.com/mozilla-b2g/gaia/commit/82d65caac572ac5c9c10de9623ff0cfa3740ce08
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(ferjmoreno)
Resolution: --- → FIXED
Whiteboard: [partner-cherry-pick]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: