822200 - Create a module for async JSON backups/restores

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

- Move the current backup/restore code to a module
- Make pseudo-APIs async

The idea is to allow moving to an async bookmarking API abstracting this piece of code with an async API. And while there remove the backup/restore code from PlacesUtils.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Let me simplify the work with a part of the code that I just submitted for feedback :)

Comment 2

[David Teller [:Yoric] - still alive but not very active]

mak, would the following feature be useful to you:
- letting OS.File.writeAtomic accept JSON objects;
- letting OS.File.read return JSON objects depending on options?

Comment 3

[Marco Bonardo [:mak]]

what we need here is to collect bookmarks in background and store them to a json, if the collection process or the json write is interrupted at any time we should just give up. so yes I think writeAtomic accepting json sounds like a must have.

Not sure regarding the read, we surely want to read the json file asynchronously, but don't need very specific options, just to read and parse the contents.

Comment 4

[Marco Bonardo [:mak]]

Just as a note, I want clarify what's the scope of this bug. We currently have a promise based BookmarksHTMLUtils module, we want a similar promise based BookmarksJSONUtils module that can read bookmarks from a json and write bookmarks to a json. Any method exposed from it should be async and promise based.

The current implementation is in PlacesUtils, it's a good reference for what is needed, but the code is quite complicated, much more than what we need. I'm not saying rewrite-from-scratch, but we are not too far.
Some parts of the current implementation are using hacks (like for livemarks it can't use the new livemarks API that is async, so it directly uses annotations), the new code should do that properly.

The first implementation may keep using Places queries as the current implementation and as BookmarksHTMLUtils, though in future we'll change that to read bookmarks asynchronously, so it should be "ready" for that (as soon as the API is totally async we can change the internals at any time). The API may look similar to BookmarkHTMLUtilsm so { restoreFromFile, restoreFromString, backupToFile }

The PlacesUtils implementation will then be converted to have mostly compatibility shims pointing to the new module, and complaining to the console that they are deprecated, so in a future version we may remove them.

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Part 1 - Module

Part I creates a new BookmarksJSONUtils module, similar to BookmarksHTMLUtils, based on the current PlacesUtils code but with promises. 

Btw, I found out that serializeNodeAsJSONToOutputStream in PlacesUtils (used to create the json) is not storing the bookmark's icon. 

Also, I think the backups code in PlacesUtils can be moved to a new BackupUtils module, that handles and uses the new BookmarksJSONUtils to write the backups.

Comment 6

[Marco Bonardo [:mak]]

> Btw, I found out that serializeNodeAsJSONToOutputStream in PlacesUtils (used
> to create the json) is not storing the bookmark's icon. 

yes it's bug 423126, there are various bugs in backups, icons and guids are the most noticeable misses.

> Also, I think the backups code in PlacesUtils can be moved to a new
> BackupUtils module, that handles and uses the new BookmarksJSONUtils to
> write the backups.

Yes, my original intent when I created that subobject was to move it out, so this is definitely something to evaluate, but in a separate follow-up bug. We can probably call the module PlacesBackups.jsm and in future could also take care of other kind of backups (like history).

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 717245 [details] [diff] [review]
Part 1 - Module

Review of attachment 717245 [details] [diff] [review]:
-----------------------------------------------------------------

I did a quick overview, there is some changes to do to the approach and some more code to port from PlacesUtils

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +17,5 @@
> +this.BookmarkJSONUtils = Object.freeze({
> +  /**
> +   * Import bookmarks from a JSON string.
> +   * Note: any item annotated with "places/excludeFromBackup" won't be removed
> +   *       before executing the restore.

move this to a @note

@@ +30,5 @@
> +   * @rejects JavaScript exception.
> +   */
> +  importFromString: function bju_importFromString(aString, aReplace) {
> +    return BookmarkImporter.importFromString(aString, aReplace);
> +  },

Actually the only consumer of this API is Metro (let alone mobile/xul that should be removed), but looks like they just copied the mobile/xul code, while they didn't actually need to complicate the code that much, since the product is not yet released so there's no risk of an old profile with bookmarks but without the root... plus we 
plus it seems to need an importFromURL method to import from chrome://browser/locale/bookmarks.json, so we should add one (see http://mxr.mozilla.org/mozilla-central/source/browser/metro/components/BrowserStartup.js#85)

So I think we should have importFromURL rather than importFromString, and we should file a bug to fix Metro to use that and ignore the case where the root doesn't exist, unless i'm missing something about their setup (we can figure that out with them in such bug).

@@ +149,5 @@
> +            // Insert the data into the db
> +            node.children.forEach(function(child) {
> +              let index = child.index;
> +              let [folders, searches] =
> +                PlacesUtils.importJSONNode(child, container, index, 0);

importJSONNode should be moved into an helper of this importer object

@@ +165,5 @@
> +
> +        // Fixup imported place: uris that contain folders
> +        searchIds.forEach(function(aId) {
> +          let oldURI = PlacesUtils.bookmarks.getBookmarkURI(aId);
> +          let uri = PlacesUtils._fixupQuery(oldURI, folderIdMap);

fixupQuery should be moved to a local module scope helper

@@ +171,5 @@
> +            PlacesUtils.bookmarks.changeBookmarkURI(aId, uri);
> +          }
> +        });
> +
> +        deferred.resolve();

for now we should simulate async, thus any resolve() should be invoked in a mainthread enqueued callback

@@ +220,5 @@
> +  },
> +
> +  _notifyObservers: function bi_notifyObservers(topic) {
> +    Services.obs.notifyObservers(null, topic, "json");
> +  }

this could be moved to a local module scope helper function

@@ +292,5 @@
> +    root.containerOpen = true;
> +
> +    // Serialize to JSON and write to stream.
> +    PlacesUtils.serializeNodeAsJSONToOutputStream(
> +      root, streamProxy, false, false, excludeItems);

serializeNodeAsJSONToOutputStream should also be moved to a local helper here

::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +10,5 @@
> +
> +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +
> +// An object representing the contents of bookmarks.preplaces.json.

the preplaces.html file was a very special naming choosen when we moved from old rdf history to Places, and it stick there for historical reasons, you should not use preplaces naming for json since these backups didn't exist before Places.
You can probably steal json test files from the other tests supporting json
http://mxr.mozilla.org/mozilla-central/search?string=.json&find=places&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
otherwise just name this bookmarks.json

@@ +77,5 @@
> +let gBookmarksFileNew;
> +
> +add_task(function setup() {
> +  // Avoid creating smart bookmarks during the test.
> +  Services.prefs.setIntPref("browser.places.smartBookmarksVersion", -1);

this should not be needed, this is needed if you do the import through browserglue

@@ +79,5 @@
> +add_task(function setup() {
> +  // Avoid creating smart bookmarks during the test.
> +  Services.prefs.setIntPref("browser.places.smartBookmarksVersion", -1);
> +
> +  // File pointer to legacy bookmarks file.

not legacy

@@ +93,5 @@
> +  // This test must be the first one, since it setups the new bookmarks.json.
> +  // Test importing a pre-Places canonical bookmarks file.
> +  // 1. import bookmarks.preplaces.json
> +  // 2. run the test-suite
> +  // Note: we do not empty the db before this import to catch bugs like 380999

I'd prefer if you'd write a new test based on the module structure as you would write a test from scratch, this has too many comments and nitpicks that are only related to the html format...
I'll check the new version in the next iteration

Comment 8

[Andres Hernandez [:andreshm]]

Attachment: Part 1 - Module v2

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 719707 [details] [diff] [review]
Part 1 - Module v2

Review of attachment 719707 [details] [diff] [review]:
-----------------------------------------------------------------

I did not complete this round for lack of time, but there's enough already to start working on

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +34,5 @@
> +
> +  /**
> +   * Restores bookmarks and tags from a JSON file.
> +   * WARNING: This method *removes* any bookmarks in the collection before
> +   * restoring from the file.

this comment clashes with the aReplace parameter...

@@ +79,5 @@
> +   * @rejects JavaScript exception.
> +   */
> +  importFromURL: function BI_importFromURL(aURL, aReplace) {
> +    let deferred = Promise.defer();
> +    ObserverHelper.notify(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN);

why not just providing a notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN) function in the global scope than an object?

@@ +82,5 @@
> +    let deferred = Promise.defer();
> +    ObserverHelper.notify(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN);
> +
> +    try {
> +      let observer = {

the observer object shouldn't be in the try, more generally try should always wrap the smaller code possible, or it may hide bugs

@@ +84,5 @@
> +
> +    try {
> +      let observer = {
> +        onStreamComplete: function(aLoader, aContext, aStatus, aLength,
> +          aResult) {

please align arguments
nit: space after function for anonymous functions

@@ +87,5 @@
> +        onStreamComplete: function(aLoader, aContext, aStatus, aLength,
> +          aResult) {
> +          let converter =
> +            Cc["@mozilla.org/intl/scriptableunicodeconverter"].
> +            createInstance(Ci.nsIScriptableUnicodeConverter);

no need to indent since doesn't go over 80 chars

@@ +88,5 @@
> +          aResult) {
> +          let converter =
> +            Cc["@mozilla.org/intl/scriptableunicodeconverter"].
> +            createInstance(Ci.nsIScriptableUnicodeConverter);
> +          let jsonString = "";

this is not used outside of the try, so can just define it on first use

@@ +90,5 @@
> +            Cc["@mozilla.org/intl/scriptableunicodeconverter"].
> +            createInstance(Ci.nsIScriptableUnicodeConverter);
> +          let jsonString = "";
> +          try {
> +            converter.charset = "UTF-8";

doesn't need to be in the try, just move it after defining converter

@@ +108,5 @@
> +          }
> +        }
> +      };
> +      let uri = Services.io.newURI(aURL, null, null);
> +      let channel = Services.io.newChannelFromURI(uri);

let channel = Services.io.newChannelFromURI(NetUtil.newURI(aURL));

@@ +111,5 @@
> +      let uri = Services.io.newURI(aURL, null, null);
> +      let channel = Services.io.newChannelFromURI(uri);
> +      let streamLoader =
> +        Cc["@mozilla.org/network/stream-loader;1"].
> +        createInstance(Ci.nsIStreamLoader);

may avoid indenting after streamLoader =

@@ +113,5 @@
> +      let streamLoader =
> +        Cc["@mozilla.org/network/stream-loader;1"].
> +        createInstance(Ci.nsIStreamLoader);
> +
> +      streamLoader.init(observer);

just to avoid confusion with nsIObserver I'd rename observer to streamObserver

@@ +126,5 @@
> +
> +  /**
> +   * Import bookmarks from a JSON string.
> +   * @note any item annotated with "places/excludeFromBackup" won't be removed
> +   *       before executing the restore.

this note should be moved to the import methods in BookmarkJSONUtils

@@ +133,5 @@
> +   *        JSON string of serialized bookmark data.
> +   * @param aReplace
> +   *        Boolean if true, replace existing bookmarks, else merge.
> +   */
> +  _importFromJSON: function BI__importFromJSON(aString, aReplace) {

can drop the prefix "_", this is an internal object already

@@ +169,5 @@
> +          query.setFolders([PlacesUtils.placesRootId], 1);
> +          let options = PlacesUtils.history.getNewQueryOptions();
> +          options.expandQueries = false;
> +          let root = PlacesUtils.history.executeQuery(query, options).root;
> +          root.containerOpen = true;

I think this may use PlacesUtils.getFolderContents

@@ +234,5 @@
> +
> +        // Fixup imported place: uris that contain folders
> +        searchIds.forEach(function(aId) {
> +          let oldURI = PlacesUtils.bookmarks.getBookmarkURI(aId);
> +          let uri = URIHelper.fixupQuery(oldURI, folderIdMap);

like ObserverHelper, why a global object instead of just a global function?

@@ +260,5 @@
> +   * @param   aContainer
> +   *          The container the data was dropped or pasted into
> +   * @param   aIndex
> +   *          The index within the container the item was dropped or pasted at
> +   * @returns an array containing of maps of old folder ids to new folder ids,

@return, with no s

@@ +265,5 @@
> +   *          and an array of saved search ids that need to be fixed up.
> +   *          eg: [[[oldFolder1, newFolder1]], [search1]]
> +   */
> +  importJSONNode: function BI_importJSONNode(aData, aContainer, aIndex,
> +    aGrandParentId) {

please align arguments

@@ +431,5 @@
> +  exportToFile: function BE_exportToFile(aLocalFile) {
> +    return Task.spawn(this._writeToFile(aLocalFile));
> +  },
> +
> +  _converterOut: null,

since this uses internal properties to track stuff, would be better to make it an instanceable object like in the HTML importer http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkHTMLUtils.jsm#886

I'd say to do the same for the BookmarkImporter too, it's more future-proof

@@ +491,5 @@
> +    options.expandQueries = false;
> +    let query = PlacesUtils.history.getNewQuery();
> +    query.setFolders([PlacesUtils.placesRootId], 1);
> +    let root = PlacesUtils.history.executeQuery(query, options).root;
> +    root.containerOpen = true;

May use PlacesUtils.getFolderContents

@@ +494,5 @@
> +    let root = PlacesUtils.history.executeQuery(query, options).root;
> +    root.containerOpen = true;
> +
> +    // Serialize to JSON and write to stream.
> +    //PlacesUtils.serializeNodeAsJSONToOutputStream(

debug commented out

@@ +622,5 @@
> +            return false;
> +          }
> +          return true;
> +        });
> +        dump("\n\nA: " + annos.legnth + "\n\n");

debug spew

@@ +692,5 @@
> +      aJSNode.uri = aPlacesNode.uri;
> +    }
> +  },
> +
> +  _asContainer: function BN_asContainer(aNode) {

PlacesUtils.asContainer should do the same...

::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +77,5 @@
> +add_task(function test_import_bookmarks() {
> +  bookmarksFile = do_get_file("bookmarks.json");
> +
> +  yield BookmarkJSONUtils.importFromFile(bookmarksFile, true);
> +  yield promiseAsyncUpdates();

the calls to promiseAsyncUpdates in this test should not be needed, if they are it's likely we are resolving the promise too early or not waiting for some async op

Comment 10

[Andres Hernandez [:andreshm]]

Attachment: Part 1 - Module v3

Applied suggested changes.

Comment 11

[Andres Hernandez [:andreshm]]

Attachment: Part 2 - Update use of importFromFile in browser

Still need to update tests using |PlacesUtils.restoreBookmarksFromJSONFile|.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 722441 [details] [diff] [review]
Part 1 - Module v3

Review of attachment 722441 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +100,5 @@
> +            yield this.importFromJSON(jsonString, aReplace);
> +          }.bind(this)).then(function() {
> +            notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_SUCCESS);
> +            deferred.resolve();
> +          });

this seems to just handle the success case, we should also notify TOPIC_BOOKMARKS_RESTORE_FAILED in the onFailure case... probably I'd invert the thing, so first spawn the Task, and catch exceptions inside it

Task.spawn(function() {
  try {
    let jsonString = converter.convertFromByteArray(aResult, aResult.length);
    yield this.importFromJSON(jsonString, aReplace);
    notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
    deferred.resolve();
  } catch (ex) {
    ...

::: toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ +81,5 @@
> +  testImportedBookmarks();
> +});
> +
> +add_task(function test_export_bookmarks() {
> +  bookmarksExportedFile = Services.dirsvc.get("ProfD", Ci.nsILocalFile);

you should be able to use gProfD - see http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/tests/head_common.js#58

@@ +98,5 @@
> +  yield BookmarkJSONUtils.importFromFile(bookmarksExportedFile, true);
> +  yield BookmarkJSONUtils.exportToFile(bookmarksExportedFile);
> +  yield BookmarkJSONUtils.importFromFile(bookmarksExportedFile, true);
> +  testImportedBookmarks();
> +  yield promiseAsyncUpdates();

this shouldn't be needed

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 724791 [details] [diff] [review]
Part 2 - Update use of importFromFile in browser

Review of attachment 724791 [details] [diff] [review]:
-----------------------------------------------------------------

I think what we should do here, is just to land the new module, so we can start working parallel on 2 fronts:
1. improve the module itself (async json, async internal APIs, cleanup)
2. replace callpoints

Doing both in this bug may slowdown the process, since here for example we should rewrite part of the logic in nsBrowserGlue, and that's just the tip of the iceberg.

I think we should file a new tracking bug for async backups, that depends on this bug, on its dependencies and on new bugs where we replace the sync calls with async calls in the codebase. Once that process is done we can just file a bug to remove the old code and move on.

::: browser/components/nsBrowserGlue.js
@@ +1018,5 @@
>        // get latest JSON backup
>        var bookmarksBackupFile = PlacesUtils.backups.getMostRecent("json");
>        if (bookmarksBackupFile) {
>          // restore from JSON backup
> +        BookmarkJSONUtils.importFromFile(bookmarksBackupFile, true);

the problem here is that we must wait for import completion before proceeding, see

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1044

this._distributionCustomizer.applyBookmarks(); and this.ensurePlacesDefaultQueriesInitialized(); should be invoked after the import process is complete.

::: browser/components/places/content/places.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Components.utils.import("resource:///modules/MigrationUtils.jsm");
> +Components.utils.import("resource://gre/modules/BookmarkJSONUtils.jsm");

just import it before the first (and only) use

@@ +442,3 @@
>      }
>      catch(ex) {
>        this._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));

the error should be handled asynchronously, importFromFile can return errors, should also be tested trying to import a corrupt json file

Comment 14

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Updated patch.

Comment 15

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d607ed53d6a1

Comment 16

[Andres Hernandez [:andreshm]]

All good on try: https://tbpl.mozilla.org/?tree=Try&rev=d607ed53d6a1

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/162aa63b2ccf

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/162aa63b2ccf

Comment 19

[Andrei Vaida [:avaida]]

Setting qe-verify- here since the fix has automation coverage. If there's something manual QA should look at here, please flip the flag.