Closed Bug 1453994 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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.
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.
Priority: P2 → P1
Status: NEW → ASSIGNED
Hmm, just remembered after pushing this. Did we want telemetry for this case, or are we more interested in the missing roots?
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 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.
Attachment #8968340 - Flags: review?(mak77)
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.
the fact is the initial SELECT is pointless, placesRootId will throw if we don't find a root.
(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.
See Also: → 1455164
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
Attachment #8968340 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/af9e7a50afc5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1466329
See Also: → 1423418
See Also: → 1472241
You need to log in before you can comment on or make changes to this bug.