In bug 1274108, I noticed that using `PlacesUtils.bookmarks.reorder` to reorder children caused services/sync/tests/unit/test_bookmark_order.js to fail at http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/services/sync/tests/unit/test_bookmark_order.js#126-130. Closer examination showed the bookmarks tree ended up with holes in the indices after calling `reorder`. This tripped up `PlacesUtils.bookmarks.getIdForItemAt`, causing the test to fail. At the point that test runs, Sync indirectly calls `PlacesUtils.bookmarks.reorder("f30_aaaaaaaa", ["10_aaaaaaaaa", "31_aaaaaaaaa"])` on the following tree: { "guid": "root________", "title": "", "index": 0, "dateAdded": 1470681072252000, "lastModified": 1470681078279000, "type": "text/x-moz-place-container", "root": "placesRoot", "children": [{ "guid": "menu________", "title": "Bookmarks Menu", "index": 0, "dateAdded": 1470681072252000, "lastModified": 1470681072605000, "type": "text/x-moz-place-container", "root": "bookmarksMenuFolder" }, { "guid": "toolbar_____", "title": "Bookmarks Toolbar", "index": 1, "dateAdded": 1470681072252000, "lastModified": 1470681072721000, "type": "text/x-moz-place-container", "root": "toolbarFolder" }, { "guid": "unfiled_____", "title": "Other Bookmarks", "index": 3, "dateAdded": 1470681072252000, "lastModified": 1470681078279000, "type": "text/x-moz-place-container", "root": "unfiledBookmarksFolder", "children": [{ "guid": "10_aaaaaaaaa", "title": "10_aaaaaaaaa", "index": 0, "dateAdded": 1470681072962000, "lastModified": 1470681072962000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "f30_aaaaaaaa", "title": "f30_aaaaaaaa", "index": 1, "dateAdded": 1470681074399000, "lastModified": 1470681074742000, "type": "text/x-moz-place-container", "children": [{ "guid": "31_aaaaaaaaa", "title": "31_aaaaaaaaa", "index": 0, "dateAdded": 1470681073986000, "lastModified": 1470681074959000, "type": "text/x-moz-place", "uri": "http://uri/" }] }, { "guid": "f40_aaaaaaaa", "title": "f40_aaaaaaaa", "index": 2, "dateAdded": 1470681076144000, "lastModified": 1470681078279000, "type": "text/x-moz-place-container", "children": [{ "guid": "41_aaaaaaaaa", "title": "41_aaaaaaaaa", "index": 0, "dateAdded": 1470681075304000, "lastModified": 1470681076694000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "42_aaaaaaaaa", "title": "42_aaaaaaaaa", "index": 1, "dateAdded": 1470681075756000, "lastModified": 1470681076951000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "20_aaaaaaaaa", "title": "20_aaaaaaaaa", "index": 2, "dateAdded": 1470681073539000, "lastModified": 1470681078279000, "type": "text/x-moz-place", "uri": "http://uri/" }] }] }, { "guid": "Rg0A9DRVRohv", "title": "mobile", "index": 4, "dateAdded": 1470681072730000, "lastModified": 1470681072812000, "annos": [{ "name": "mobile/bookmarksRoot", "flags": 0, "expires": 4, "value": 1 }, { "name": "places/excludeFromBackup", "flags": 0, "expires": 4, "value": 1 }], "type": "text/x-moz-place-container" }] } ...Producing this tree: { "guid": "root________", "title": "", "index": 0, "dateAdded": 1470421326071000, "lastModified": 1470421327544000, "type": "text/x-moz-place-container", "root": "placesRoot", "children": [{ "guid": "menu________", "title": "Bookmarks Menu", "index": 0, "dateAdded": 1470421326071000, "lastModified": 1470421326196000, "type": "text/x-moz-place-container", "root": "bookmarksMenuFolder" }, { "guid": "toolbar_____", "title": "Bookmarks Toolbar", "index": 1, "dateAdded": 1470421326071000, "lastModified": 1470421326222000, "type": "text/x-moz-place-container", "root": "toolbarFolder" }, { "guid": "unfiled_____", "title": "Other Bookmarks", "index": 3, "dateAdded": 1470421326071000, "lastModified": 1470421327544000, "type": "text/x-moz-place-container", "root": "unfiledBookmarksFolder", "children": [{ "guid": "f30_aaaaaaaa", "title": "f30_aaaaaaaa", "index": 0, "dateAdded": 1470421326595000, "lastModified": 1470421327544000, "type": "text/x-moz-place-container", "children": [{ "guid": "10_aaaaaaaaa", "title": "10_aaaaaaaaa", "index": 0, "dateAdded": 1470421326271000, "lastModified": 1470421327544000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "31_aaaaaaaaa", "title": "31_aaaaaaaaa", "index": 1, "dateAdded": 1470421326513000, "lastModified": 1470421326708000, "type": "text/x-moz-place", "uri": "http://uri/" }] }, { "guid": "f40_aaaaaaaa", "title": "f40_aaaaaaaa", "index": 1, "dateAdded": 1470421326981000, "lastModified": 1470421327433000, "type": "text/x-moz-place-container", "children": [{ "guid": "41_aaaaaaaaa", "title": "41_aaaaaaaaa", "index": 0, "dateAdded": 1470421326797000, "lastModified": 1470421327087000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "20_aaaaaaaaa", "title": "20_aaaaaaaaa", "index": 2, "dateAdded": 1470421326407000, "lastModified": 1470421327433000, "type": "text/x-moz-place", "uri": "http://uri/" }, { "guid": "42_aaaaaaaaa", "title": "42_aaaaaaaaa", "index": 3, "dateAdded": 1470421326897000, "lastModified": 1470421327144000, "type": "text/x-moz-place", "uri": "http://uri/" }] }] }, { "guid": "ulT4L3Q4iD0Z", "title": "mobile", "index": 4, "dateAdded": 1470421326224000, "lastModified": 1470421326242000, "annos": [{ "name": "mobile/bookmarksRoot", "flags": 0, "expires": 4, "value": 1 }, { "name": "places/excludeFromBackup", "flags": 0, "expires": 4, "value": 1 }], "type": "text/x-moz-place-container" }] } Even though we didn't reorder the children of `f40_aaaaaaaa`, they still skip an index (0, 2, 3). Note also that the roots are missing index 2: toolbar is 1, and unfiled is 3. I'm not sure if I'm using `reorder` incorrectly, there's a race, or something else is going on.

(In reply to Kit Cambridge [:kitcambridge] from comment #0) > At the point that test runs, Sync indirectly calls > `PlacesUtils.bookmarks.reorder("f30_aaaaaaaa", ["10_aaaaaaaaa", > "31_aaaaaaaaa"])` on the following tree: I don't understand, in the first tree, 10_aaaaaaaaa seems to be child of unfiled, while 31_aaaaaaaaa is child of f30_aaaaaaaa. reorder is supposed to be used for direct (1 level) children of a single folder. But in the second tree, both are children of f30_aaaaaaaaa... looks like there is some other operation in the middle, or how could it move?

I did some more investigating today, and extended test_bookmark_order.js to check the full tree. That's the only test that fails if I remove the custom query and uncomment http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/toolkit/components/places/PlacesSyncUtils.jsm#137-140. FTR, this test checks reordering when we insert missing parents. Up through "Moving 20 behind 42 in f40 -> update 50", the trees are identical: [{ "guid": "menu________", "index": 0 }, { "guid": "toolbar_____", "index": 1 }, { "guid": "unfiled_____", "index": 3, "children": [{ "guid": "10_aaaaaaaaa", "index": 0 }, { "guid": "f30_aaaaaaaa", "index": 1, "children": [{ "guid": "31_aaaaaaaaa", "index": 0 }] }, { "guid": "f40_aaaaaaaa", "index": 2, "children": [{ "guid": "41_aaaaaaaaa", "index": 0 }, { "guid": "42_aaaaaaaaa", "index": 1 }, { "guid": "20_aaaaaaaaa", "index": 2 }] }] }] (It looks like the child of the root at index 2 is "tags________", so that's OK). The next test is "Moving 10 in front of 31 in f30 -> update 10, f30". Here, we move "10_aaaaaaaaa" into "f30_aaaaaaaa", so we expect a tree that looks like this: [{ "guid": "menu________", "index": 0 }, { "guid": "toolbar_____", "index": 1 }, { "guid": "unfiled_____", "index": 3, "children": [{ "guid": "f30_aaaaaaaa", "index": 0, "children": [{ "guid": "10_aaaaaaaaa", "index": 0 }, { "guid": "31_aaaaaaaaa", "index": 1 }] }, { "guid": "f40_aaaaaaaa", "index": 1, "children": [{ "guid": "41_aaaaaaaaa", "index": 0 }, { "guid": "42_aaaaaaaaa", "index": 1 }, { "guid": "20_aaaaaaaaa", "index": 2 }] }] }] ...Which is what we get with `PlacesSyncUtils.bookmarks.order("f30_aaaaaaaa", ["10_aaaaaaaaa", "31_aaaaaaaaa"])`. With `PlacesUtils.bookmarks.reorder("f30_aaaaaaaa", ["10_aaaaaaaaa", "31_aaaaaaaaa"])`, though, we end up with one like this: [{ "guid": "menu________", "index": 0 }, { "guid": "toolbar_____", "index": 1 }, { "guid": "unfiled_____", "index": 3, "children": [{ "guid": "f30_aaaaaaaa", "index": 0, "children": [{ "guid": "10_aaaaaaaaa", "index": 0 }, { "guid": "31_aaaaaaaaa", "index": 1 }] }, { "guid": "f40_aaaaaaaa", "index": 1, "children": [{ "guid": "41_aaaaaaaaa", "index": 0 }, { "guid": "20_aaaaaaaaa", "index": 2 }, { "guid": "42_aaaaaaaaa", "index": 3 }] }] }] So, the children of f30_aaaaaaaa are correct...but what happened to f40_aaaaaaaa? Why was 42_aaaaaaaaa even moved, when we only reordered the children of f30_aaaaaaaa? And what happened to index 1?

Here's the log between the start of "Moving 10 in front of 31 in f30 -> update 10, f30" and the end: https://gist.github.com/kitcambridge/93e401587d802ed90df6d45693c45a8a The only mention of f40_aaaaaaaa is at the end, when the test fails because we somehow reordered f40_aaaaaaaa's children, despite never mentioning it in any other statement. ¯\_(ツ)_/¯

(In reply to Kit Cambridge [:kitcambridge] from comment #2) > The next test is "Moving 10 in front of 31 in f30 -> update 10, f30". Here, > we move "10_aaaaaaaaa" into "f30_aaaaaaaa", so we expect a tree that looks > like this: While I look into reorder code, I need some more information about this move operation cause the bug seems more related to that. How is this move done, which bookmarks API calls is actually making it? Do we wait fo that move operation to be complete before trying to reorder? Why after the move we need to reorder, doesn't the update API already do that?

fwiw, the update query in Bookmarks.jsm::reorderChildren seems to be missing a WHERE condition, or the parenthesis is misplaced. This is only used in tests so far, so I'd not be surprised if it's just wrong :( I'll see if I can write a bookmarks unit test that hits your bug and in any case re check this query.

As expected the bug is the lack of a WHERE condition on the UPDATE statement. Due to that it's trying to reorder EVERY bookmark, not just the ones into the specified folder.

Comment on attachment 8786679 [details] Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering. https://reviewboard.mozilla.org/r/75592/#review73600 Awesome, thanks!

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/fx-team/rev/af5df7bd1d26 PlacesUtils.bookmarks.reorder can introduce holes when reordering. r=kitcambridge

Backed out for failing modified xpcshell test test_bookmarks_reorder.js: https://hg.mozilla.org/integration/fx-team/rev/9d715b38e5f094c63df4fa560fd00b6ff6852f9b Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=af5df7bd1d265bb95c76b69fa4fbcc60a84094f9 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=11382558&repo=fx-team > TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js | move_and_reorder - [move_and_reorder : 152] 1 == 2

bah, thanks and osrry for the noise. The usual last minute test change that can't go wrong :/

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/91d164ff4285 PlacesUtils.bookmarks.reorder can introduce holes when reordering. r=kitcambridge