1404631 - Firefox 56: "Unable to process the backup file" without any details on what's wrong

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Konstantin Boyandin]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

Tried to load backup created by Firefox 55.* from built-in Bookmarks manager.


Actual results:

"Unable to process the backup file" displayed, nothing else happens.


Expected results:

Bookmark file should be normally imported.

When I create backup from Firefox 56, it's imported without issues.

Comment 1

[Marco Bonardo [:mak]]

Hi, would you allow me to have access to the broken backup for debugging purposes? A private mail to my bugzilla mail address would be fine.

Comment 2

[Marco Bonardo [:mak]]

you can also send it to my mozilla mail if you trust that more, mak AT mozilla.com

Comment 3

[Konstantin Boyandin]

(In reply to Marco Bonardo [::mak] from comment #2)
> you can also send it to my mozilla mail if you trust that more, mak AT
> mozilla.com

I have sent email with a sample bookmarks backup file attached.

Comment 4

[Marco Bonardo [:mak]]

Investigating.

Comment 5

[Marco Bonardo [:mak]]

Interesting, in the browser console I see this error: Error: Bookmarks.jsm: insertTree: Invalid value for property 'lastModified': 0

We should not be writing 0 here. It's possible there is a corruption in the database where either the lastModified or dateAdded field are zero or NULL, something that is unexpected.

Comment 6

[Konstantin Boyandin]

(In reply to Marco Bonardo [::mak] from comment #5)
> Interesting, in the browser console I see this error: Error: Bookmarks.jsm:
> insertTree: Invalid value for property 'lastModified': 0
> 
> We should not be writing 0 here. It's possible there is a corruption in the
> database where either the lastModified or dateAdded field are zero or NULL,
> something that is unexpected.

The original problem reported in this bug was happening on two separate Firefox installations (both working on Ubuntu, 14.04 and 16.04). If I understand correctly, even if database contained unexpected value, the result shouldn't be an exception, causing the process to abort.

Comment 7

[Marco Bonardo [:mak]]

Yes, we'll add a fixup procedure so that import/restore will try to do its best to insert "invalid" entries or skip that, so that most entries can still be added.

Comment 8

[Marco Bonardo [:mak]]

Attachment: Bug 1404631 - Bookmarks.insertTree should have an option to fixup input and skip broken entries.

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

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8922250 [details]
Bug 1404631 - Bookmarks.insertTree should have an option to fixup input and skip broken entries.

https://reviewboard.mozilla.org/r/193282/#review198724

Looking good. I found one small potential intermittent issue with the test, but I think that's probably an easy fix.

::: toolkit/components/places/tests/bookmarks/test_insertTree_fixupOrSkipInvalidEntries.js:12
(Diff revision 1)
> +  await Assert.throws(() => insertTree({guid, children: []}),
> +                      /Should have a non-zero number of children to insert./);
> +  await Assert.throws(() => insertTree({guid: "invalid", children: [{}]}),
> +                      /The parent guid is not valid/);
> +
> +  let now = new Date();

I just tried this on my Mac and got:

  FAIL Found a valid dateAdded - false == true
/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_insertTree_fixupOrSkipInvalidEntries.js:onItemAdded:25
resource://gre/modules/Bookmarks.jsm:notify:1206
resource://gre/modules/Bookmarks.jsm:insertTree/<:510

It looks like it takes a couple of seconds to run through, so `now` is 1509046068872000 but `dateAdded` recorded by the observer is 1509046068874000.

I think you probably want to move `now` into the `onItemAdded` but also calculate it separately for the dates in the tree.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #9)
> I think you probably want to move `now` into the `onItemAdded` but also
> calculate it separately for the dates in the tree.

I'll just replace the is_time_ordered with a >=, because I can't predict the time used by Bookmarks.jsm to fixup. And experience teaches me it's better to not trust timestamps guessing in tests :)

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8922250 [details]
Bug 1404631 - Bookmarks.insertTree should have an option to fixup input and skip broken entries.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4e6ab850229
Bookmarks.insertTree should have an option to fixup input and skip broken entries. r=standard8

Comment 13

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

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