Closed Bug 1297941 Opened 3 years ago Closed 3 years ago

removeFoldersContents may leave an invalid GuidHelper cache

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: markh, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

The attached patch demonstrates an example of removeFolderContents leaving now-deleted entries in the GuidHelper cache. removeFolderContents is used by PlacesUtils.bookmarks.remove() and PlacesUtils.bookmarks.eraseEverything().

The problem appears to be the order of the rows returned by the first query in removeFolderContents - it is expected that all parents will be returned before children - then that list is reversed and observer notifications sent. If the order is wrong (ie, a notification for a parent is sent before the child) the GuidHelper observer will re-add the already deleted parent to the cache.

IOW, the bug can be worked around by removing the line https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/toolkit/components/places/PlacesUtils.jsm#2493

I don't understand why the SQL statement in removeFoldersContents is supposed to guarantee the order - it *usually* seems to, but not in my testcase where I reparented an item.

This could have fairly bad consequences - particularly for Sync, but really for any code that needs to map GUIDs to IDs after a folder is deleted.

Mak, any thoughts?
Flags: needinfo?(mak77)
Blocks: 1297945
It's a very interesting test case!

I suspect the problem is due to the use of the IN construct. While WITH RECURSIVE is supposed to visit the graph in order, the IN clause is not supposed to return entries in the same order they appear in the IN list (cause it's a set!).
The results are instead returned in database insertion order, rather than in found order, and in most cases that is correct (child is inserted after parent), but with your test case that assumption is not true anymore, the child is inserted before its future parent, and then moved into it!

I can try rewriting the query without the IN.
I looked around and "luckily" couldn't find other cases where we care about order and use an IN clause. But I'll keep my eyes open for other possible mis-uses.

Thank you for catching this and for the test!
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Blocks: 1294291
Attachment #8784674 - Attachment is obsolete: true
Comment on attachment 8785235 [details]
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

I honestly think Kit has a better understanding of the SQL here.
Attachment #8785235 - Flags: review?(markh) → review?(kcambridge)
Comment on attachment 8785235 [details]
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

https://reviewboard.mozilla.org/r/74506/#review72428

Interesting! This works because `WITH RECURSIVE` is FIFO, right?
Attachment #8785235 - Flags: review?(kcambridge) → review+
with recursive is by definition... recursive. It can basically visit the bookmarks graph as we prefer, since we start from the roots, it will always visit parents before children (breadth-first or depth-first doesn't matter here).
Now, the only "problem" remaining is that ideally a SQL engine could reorder joins, and then we should pay an additional price for an ORDER BY, but it's very unlikely to do that in this case, cause it starts from a list of ids, so I preferred to just trust that and the written test.
If in the future that should end up being a problem, we can replace JOIN with CROSS JOIN, that it's the same, but tells the optimizer it should not reorder the joins.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/fx-team/rev/087a40f5eaa9
eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache. r=kcambridge
https://hg.mozilla.org/mozilla-central/rev/087a40f5eaa9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8785235 [details]
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

Approval Request Comment
[Feature/regressing bug #]: new bookmarking API
[User impact if declined]: Some bookmarks GUID may be wrongly reported, confusing notification consumers like Sync.
[Describe test coverage new/current, TreeHerder]: unit tests
[Risks and why]: simple sql change, low risk
[String/UUID change made/needed]: none
Attachment #8785235 - Flags: approval-mozilla-aurora?
Comment on attachment 8785235 [details]
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

Fixes a bug in the new bookmarking API, Aurora50+
Attachment #8785235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.