1453994 - Unable to export bookmarks - built-in root had wrong parent

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Mark Banner (:standard8)]

I just received a places.sqlite from a long-time user that couldn't export their bookmarks (at least to HTML).

On investigation, I discovered the roots as:

ID     Parent  guid
1      0       root________
2      1       menu________
3      1       toolbar_____
4      1       tags________
674    1       <this is the stub folder for the left-pane>
2447   1       mobile______
13254  2       unfiled_____

My guess is that at some stage Other Bookmarks has been deleted, but then it got recreated. However, when it got recreated, it has been created under the Bookmarks Menu folder, rather than the root.

I'm assuming it wasn't simply moved, as I'd have expected the ID to have been '5' or something similar at that stage.

The wrong parent is enough to break HTML export.

Comment 1

[jscher2000]

For context, the user's Browser Console shows:

root is undefined BookmarkHTMLUtils.jsm:954

940 function BookmarkExporter(aBookmarksTree) {
941   // Create a map of the roots.
942   let rootsMap = new Map();
943   for (let child of aBookmarksTree.children) {
944     if (child.root)
945       rootsMap.set(child.root, child);
946   }
...
952   for (let key of [ "toolbarFolder", "unfiledBookmarksFolder" ]) {
953     let root = rootsMap.get(key);
954     if (root.children && root.children.length > 0) {

So if unfiled is missing from rootsMap, you can see why trying to get undefined.children halts the export.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1453994 - Add a places maintenance task to ensure built-in bookmark folders have the correct parents.

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

Comment 3

[Mark Banner (:standard8)]

Hmm, just remembered after pushing this. Did we want telemetry for this case, or are we more interested in the missing roots?

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

In theory the sync ping already has telemetry for both of those cases (at least, when validation runs it does, which is for users on nightly/beta with <= 1000 bookmarks), but writing a query in ReDash to get these numbers out is a bit tricky https://searchfox.org/mozilla-central/source/services/sync/modules/bookmark_validator.js#109-110

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8968340 [details]
Bug 1453994 - Add a places maintenance task to ensure built-in bookmark folders have the correct parents.

https://reviewboard.mozilla.org/r/237012/#review242950

Should we do something to help Sync here? Like bumping the syncChangeCounter. I'd suggest to ask Kit or Thom, to be sure what's the best path.

::: toolkit/components/places/PlacesDBUtils.jsm:703
(Diff revision 1)
>      // due to an endianness issue.  We try to fix broken roots here.
>      let db = await PlacesUtils.promiseDBConnection();
>      let rows = await db.execute(
>        `SELECT id FROM moz_bookmarks WHERE id = :root`,
>        { root: PlacesUtils.placesRootId });
> -    if (rows.length === 0) {
> +

I think we should check that PlacesUtils.placesRootId doesn't throw (because bookmarks.placesRoot may throw) first, instead of running this query, if it throws the main root doesn't exist, and we should create it (second part here). Then fetch placesRootId again.

At that point once we are sure the main root is ok, we can reparent the others.
In the reparenting query you may add a WHERE parent <> :places_root so in the best case we don't write anything.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8968340 [details]
Bug 1453994 - Add a places maintenance task to ensure built-in bookmark folders have the correct parents.

https://reviewboard.mozilla.org/r/237012/#review242950

I believe this is already covered by a temp trigger: https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/components/places/PlacesDBUtils.jsm#731-748

> I think we should check that PlacesUtils.placesRootId doesn't throw (because bookmarks.placesRoot may throw) first, instead of running this query, if it throws the main root doesn't exist, and we should create it (second part here). Then fetch placesRootId again.
> 
> At that point once we are sure the main root is ok, we can reparent the others.
> In the reparenting query you may add a WHERE parent <> :places_root so in the best case we don't write anything.

TBH, I'm a bit suspect of the existing code anyway, as I think it might be hitting the wrong parent if it creates a missing root. I'll try and extend the tests a bit to check.

Comment 7

[Marco Bonardo [:mak]]

the fact is the initial SELECT is pointless, placesRootId will throw if we don't find a root.

Comment 8

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #7)
> the fact is the initial SELECT is pointless, placesRootId will throw if we
> don't find a root.

Now I've looked a bit more, I agree, that whole section is broken.

Given that we're planning on checking root validity on database open in bug 478035, I think I'll get that bug done first, and then come back to this one to implement the parent id checks in PlacesDBUtils, but in a nicer way.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8968340 [details]
Bug 1453994 - Add a places maintenance task to ensure built-in bookmark folders have the correct parents.

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

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8968340 [details]
Bug 1453994 - Add a places maintenance task to ensure built-in bookmark folders have the correct parents.

https://reviewboard.mozilla.org/r/237012/#review244148

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af9e7a50afc5
Add a places maintenance task to ensure built-in bookmark folders have the correct parents. r=mak

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/af9e7a50afc5