1332702 - Ignore keyword insertion failures when importing bookmarks

Status: NEW

(Firefox :: Bookmarks & History, defect, P3)

Description

[Stefan [:StefanG_QA]]

Attachment: about sync shows "Unable to process backup file" when the file is processed correctly.png

Mozilla/5.0(Macintosh; Intel Mac OS X 10.11; rv:53.0)Gecko/20100101 Firefox/53.0

Precondition:
Download AboutSync addon: https://addons.mozilla.org/en-US/firefox/addon/about-sync/

Step
1. Launch firefox with an existing sync account
2. Go to Menu Bar > Bookmarks > Show all bookmarks
3. Click on Backup from "Import and Backup" drop down list
4. Save Bookmarks as Json file
5. Add, Edit, Delete bookmarks from the account profile, trigger sync and wait for finish
6. In About Sync, verify that there are no bookmarks missing from the server
7. Close and reopen Firefox with the same profile
8. Go to Menu Bar > Bookmarks > Show all bookmarks
9. Click on "Import and Backup" and select Restore -> Choose file
10. Select backed up file from step 4 to restore
11. Click Ok
12. Trigger Sync and wait for it to finish
13. In About Sync, verify that there are no bookmarks missing from the server

AR: About:Sync shows wrong message "Unable to process backup file" while the bookmarks are imported correctly (see the attached screenshot)

ER: The backup file is imported correctly, About:Sync show successful message

Comment 1

[Kanchan Kumari QA]

I encountered this issue too one time on Windows 10. 
When I tried second time to restore same bookmarks json file, it got imported without any issue.
Note: Backup json file is from some other old profile.

Comment 2

[Lina Butler [:lina]]

It's not clear to me what About Sync has to do with this. From the screenshot, it looks like that's the Library UI, and About Sync isn't involved at all? The code that shows the error is at http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/browser/components/places/content/places.js#515-521, but we don't log why `BookmarkJSONUtils.importFromFile` threw.

Mak, do you think it's worth adding a `Cu.reportError` call here? It's also interesting that Stefan and Kanchan's bookmarks were imported successfully, despite the error, and that importing again makes the error go away.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> involved at all? The code that shows the error is at
> http://searchfox.org/mozilla-central/rev/
> 30fcf167af036aeddf322de44a2fadd370acfd2f/browser/components/places/content/
> places.js#515-521, but we don't log why `BookmarkJSONUtils.importFromFile`
> threw.
> 
> Mak, do you think it's worth adding a `Cu.reportError` call here?

That method uses importFromFile that already has a reportError:
Cu.reportError("Failed to restore bookmarks from " + aFilePath + ": " + ex);

It would be useful to know what it printed to the console.

> It's also
> interesting that Stefan and Kanchan's bookmarks were imported successfully,
> despite the error, and that importing again makes the error go away.

It depends on what we mean by imported successfully, it's possible the process worked 99%, but missed something that may not be easily detectable through the ui. For example, it may have failed to create a keyword? OR maybe something was just busy and the first time it threw? Many things may go wrong without being clearly visible in the UI.

Comment 4

[Marco Bonardo [:mak]]

stefan, could you please report the Browser console output?

Comment 5

[Stefan [:StefanG_QA]]

Attachment: Importing Bookmarks 'Failed' browser console outoput.txt

Please find the requested Browser Console output as an attached txt file

Comment 6

[Marco Bonardo [:mak]]

UNIQUE constraint failed: moz_keywords.keyword  BookmarkJSONUtils.jsm:111

Comment 7

[Marco Bonardo [:mak]]

Looks like the restore tries to recreate an already existing keyword.
I think we may want to ignore keyword insertion errors, regardless "Unable to process backup file" is not a good error to show to the user for this case. We should probably differentiate a complete failure (invalid file) from a partial failure ("We were unable to import some of the items").
This bug could cover figuring out a better error message.

That said, there is probably a race condition here. We remove all the bookmarks to start restoring, keywords observe bookmarks removal and start their removal. The restore process may begin before the keywords work is done, and then effectively we could miss restoring a keyword because the insertion fails and there's a pending removal.
We could:
1. find a way to wait for pending keywords work, but that sounds hackish.
2. explicitly remove all the keywords before starting removing bookmarks. The problem is that once keywords are decoupled from bookmarks (bug 648398), this may end up removing too much.
3. in the keywords bookmarks observer abort a keyword deletion if in the meanwhile someone tries to insert the same keyword OR if there are pending observer operations yield on those before doing the insertion.

Solution 3 looks like the best one, so far.

Comment 9

[Mark Banner (:standard8)]

Stefan, are you still able to reproduce this?

If you can, would you be able to share the bookmarks file?

I think Marco's comments in comment 7 aren't quite right, because he was assuming we're not waiting for the keywords removal to happen, when in fact we are.

Comment 10

[Mark Banner (:standard8)]

The main issue we should look at is ignore keyword insertion failures for existing keywords whilst restoring the bookmark.