1297941 - removeFoldersContents may leave an invalid GuidHelper cache

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Demonstrate-invalid-cache-bug.patch

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?

Comment 1

[Marco Bonardo [:mak]]

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!

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

Review commit: https://reviewboard.mozilla.org/r/74506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74506/

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8785235 [details]
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74506/diff/1-2/

Comment 4

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 5

[Lina Butler [:lina]]

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?

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/087a40f5eaa9
eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache. r=kcambridge

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/087a40f5eaa9

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6337a31b7b04e332afe15b2d3647beb7a37b11d0