`PlacesUtils.bookmarks.reorder` can introduce holes when reordering

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: mak)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
(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?
Flags: needinfo?(kcambridge)
(Reporter)

Comment 2

2 years ago
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?
Flags: needinfo?(kcambridge)
(Reporter)

Comment 3

2 years ago
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. ¯\_(ツ)_/¯
Comment hidden (mozreview-request)
Flags: needinfo?(mak77)
(Assignee)

Comment 5

2 years ago
(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?
Flags: needinfo?(mak77) → needinfo?(kcambridge)
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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.
Assignee: nobody → mak77
(Assignee)

Updated

2 years ago
Flags: needinfo?(kcambridge)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8786679 [details]
Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering.

https://reviewboard.mozilla.org/r/75592/#review73600

Awesome, thanks!
Attachment #8786679 - Flags: review?(kcambridge) → review+
(Reporter)

Updated

2 years ago
Attachment #8786094 - Attachment is obsolete: true

Comment 10

2 years ago
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
Flags: needinfo?(mak77)
(Assignee)

Comment 12

2 years ago
bah, thanks and osrry for the noise.
The usual last minute test change that can't go wrong :/
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)

Comment 14

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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91d164ff4285
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Updated

2 years ago
Depends on: 1302960
You need to log in before you can comment on or make changes to this bug.