1116446 - Cannot restore bookmarks from backup JSON file.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Tooru Fujisawa [:arai]]

bug 852034 replaces PlacesUtils.restoreBookmarksFromJSONFile with BookmarkJSONUtils.importFromFile,
then bug 854288 removes PlacesUtils.restoreBookmarksFromJSONFile.

it breaks the SeaMonkey's bookmark restoration feature, which still uses PlacesUtils.restoreBookmarksFromJSONFile.
http://dxr.mozilla.org/comm-central/source/suite/common/bookmarks/bookmarksManager.js#436

Need to port some of bug 852034's patch to comm-central.

it's also reproducible on SeaMonkey 2.31.

Comment 1

[Tooru Fujisawa [:arai]]

Also, may need to port some of bug 852041's patch to make _initPlaces asynchronous.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch

currently jsonlz4 is used for bookmark backup format,
so this patch is needed to support restore from in-profile backup.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

This patch fixes Restore menu in bookmark manager.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in nsSuiteGlue.js.

This patch fixes the automated bookmark restoration when database is corrupted.

Tested on Mac OS X 10.9.5 with following steps:
  1. run SeaMonkey with clean profile
  2. restore from JSON file which is exported from other profile (Part 2)

  3. backup to some file, to make backup in bookmarkbakcups folder
  4. remove some bookmark items
  5. restore from today's backup (Part 1)

  6. shutdown SeaMonkey
  7. remove places.sqlite
  8. run SeaMonkey, and confirm bookmark is restored (Part 3)

Comment 5

[therube]

See also, Bug 1061672 - Import json-file from Firefox (or even from SeaMonkey) do not work.

Comment 6

[Philip Chee]

Comment on attachment 8542532 [details] [diff] [review]
Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch

Neil may be a better reviewer.

Comment 7

[[e-mail]]

Comment on attachment 8542532 [details] [diff] [review]
Bug 1116446 Part 1: Support jsonlz4 format for restore in_bookmarksManager_js.patch

Notwithstanding that the old code used match to test a string against a regexp when it should have used test instead, but not only did you compound the error by adding an endsWidth test instead of extending the existing regexp but you also copied Firefox's error of not testing for the . in the extension.

Comment 8

[[e-mail]]

Comment on attachment 8542533 [details] [diff] [review]
Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

>+    Task.spawn(function() {
Aren't generators supposed to use function*()?

>+        yield BookmarkJSONUtils.importFromFile(aFile, true);
This wants a path, not an nsIFile object.

>+    }.bind(this));
Rather than bind a generator to a task, might it be easier to catch the rejection on the returned Promise?

Comment 9

[[e-mail]]

(In reply to comment #7)
> Notwithstanding that the old code used match to test a string against a
> regexp when it should have used test instead, but not only did you compound
> the error by adding an endsWidth test instead of extending the existing
> regexp but you also copied Firefox's error of not testing for the . in the
> extension.

PlacesBackups.getMostRecentBackup almost has the right regexp, but they still slipped up. Oops!

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support jsonlz4 format for restore in bookmarksManager.js

Thank you for reviewing :D
merged two tests, and use .test() method.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

Fixed aFile to aFile.path, and use .catch().

(In reply to [e-mail] from comment #8)
> Comment on attachment 8542533 [details] [diff] [review]
> Part 2: Use BookmarkJSONUtils.importFromFile instead of
> PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.
> 
> >+    Task.spawn(function() {
> Aren't generators supposed to use function*()?
Yeah, it also should be addressed in Firefox's side,
we should remove all legacy generator (bug 1083482).

Comment 12

[Philip Chee]

Comment on attachment 8542598 [details] [diff] [review]
Part 2: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in bookmarksManager.js.

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.

Comment 13

[Philip Chee]

Comment on attachment 8542597 [details] [diff] [review]
Part 1: Support jsonlz4 format for restore in bookmarksManager.js

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.

Comment 14

[Philip Chee]

Comment on attachment 8542535 [details] [diff] [review]
Part 3: Use BookmarkJSONUtils.importFromFile instead of PlacesUtils.restoreBookmarksFromJSONFile in nsSuiteGlue.js.

[Approval Request Comment]
Regression caused by (bug #): bug 852034 bug 854288
User impact if declined: Cannot restore bookmark from backup JSON file
Testing completed (on m-c, etc.): Port of bug 852034 bug 854288
Risk to taking this patch (and alternatives if risky): none bustage fix
String changes made by this patch: None.

Comment 15

[Tooru Fujisawa [:arai]]

Thank you for reviewing and approval :D

Comment 16

[Philip Chee]

Pushed to comm-central:
Part 1 http://hg.mozilla.org/comm-central/rev/2781ce3e4d00
Part 2 http://hg.mozilla.org/comm-central/rev/d49fb2508139
Part 3 http://hg.mozilla.org/comm-central/rev/6b1776a1a2f6

Pushed to comm-aurora:
Part 1 http://hg.mozilla.org/releases/comm-aurora/rev/5b15f41fa855
Part 2 http://hg.mozilla.org/releases/comm-aurora/rev/f611e79f87d6
Part 3 http://hg.mozilla.org/releases/comm-aurora/rev/3e14df96f855

Pushed to comm-beta:
Part 1 http://hg.mozilla.org/releases/comm-beta/rev/ba17d6278eee
Part 2 http://hg.mozilla.org/releases/comm-beta/rev/d4bc15898eeb
Part 3 http://hg.mozilla.org/releases/comm-beta/rev/0c59a68dbe20