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

Reporter

Description

•

8 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.

Marco Bonardo [:mak]

Assignee

Comment 1

•

8 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?

Mark Hammond [:markh] [:mhammond]

Updated

•

8 years ago

Flags: needinfo?(kcambridge)

Lina Butler [:lina]

Reporter

Comment 2

•

8 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)

Lina Butler [:lina]

Reporter

Comment 3

•

8 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)

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

Mark Hammond [:markh] [:mhammond]

Updated

•

8 years ago

Flags: needinfo?(mak77)

Marco Bonardo [:mak]

Assignee

Comment 5

•

8 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)

Marco Bonardo [:mak]

Assignee

Comment 6

•

8 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.

Marco Bonardo [:mak]

Assignee

Comment 7

•

8 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

Marco Bonardo [:mak]

Assignee

Updated

•

8 years ago

Flags: needinfo?(kcambridge)

Comment hidden (mozreview-request)

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

Marco Bonardo [:mak]

Assignee

Updated

•

8 years ago

Priority: -- → P1

Lina Butler [:lina]

Reporter

Comment 9

•

8 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+

Lina Butler [:lina]

Reporter

Updated

•

8 years ago

Attachment #8786094 - Attachment is obsolete: true

Pulsebot

Comment 10

•

8 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

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 11

•

8 years ago

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)

Marco Bonardo [:mak]

Assignee

Comment 12

•

8 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 on attachment 8786679 [details]
Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering.

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

Pulsebot

Comment 14

•

8 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

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 15

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/91d164ff4285

Status: NEW → RESOLVED

Closed: 8 years ago

status-firefox51: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla51

Lina Butler [:lina]

Reporter

Updated

•

8 years ago

Depends on: 1302960

Bug 1293365 - Check the entire bookmarks tree in test_bookmark_order. 8 years ago Lina Butler [:lina] 58 bytes, text/x-review-board-request		Details
Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering. 8 years ago Marco Bonardo [:mak] 58 bytes, text/x-review-board-request	lina : review+	Details