Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks. r=hsivonen,mak
authorMark Banner <standard8@mozilla.com>
Fri, 01 Jun 2018 17:05:33 +0100
changeset 475567 e0821414fc6ef41bb655f50dd3c67a8d1bf28b45
parent 475566 f8c98ae75bbf5043ab2f6d2f33b534cf58be7790
child 475568 22c2645c283cbff140a25290936e2d3ccb4698bf
push id9374
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:43:20 +0000
treeherdermozilla-beta@160e085dfb0b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershsivonen, mak
bugs1444329
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1444329 - Stop using nsIScriptableUnicodeConverter when importing bookmarks. r=hsivonen,mak This also changes the way an empty file is handled when importing bookmarks - it will now throw with a syntax error. The reason for the change is that using fetch's json converter, can't handle an empty file, and we can't easily adjust the data without reverting to JSON.parse manually. MozReview-Commit-ID: KmVWZA5HIT
toolkit/components/places/BookmarkJSONUtils.jsm
toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js
toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
--- a/toolkit/components/places/BookmarkJSONUtils.jsm
+++ b/toolkit/components/places/BookmarkJSONUtils.jsm
@@ -4,18 +4,18 @@
 
 var EXPORTED_SYMBOLS = [ "BookmarkJSONUtils" ];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
 
-ChromeUtils.defineModuleGetter(this, "NetUtil",
-  "resource://gre/modules/NetUtil.jsm");
+Cu.importGlobalProperties(["fetch"]);
+
 ChromeUtils.defineModuleGetter(this, "PlacesBackups",
   "resource://gre/modules/PlacesBackups.jsm");
 
 /**
  * Generates an hash for the given string.
  *
  * @note The generated hash is returned in base64 form.  Mind the fact base64
  * is case-sensitive if you are going to reuse this code.
@@ -166,62 +166,40 @@ BookmarkImporter.prototype = {
    *
    * @param aSpec
    *        url of the bookmark data.
    *
    * @return {Promise}
    * @resolves When the new bookmarks have been created.
    * @rejects JavaScript exception.
    */
-  importFromURL(spec) {
-    return new Promise((resolve, reject) => {
-      let streamObserver = {
-        onStreamComplete: (aLoader, aContext, aStatus, aLength, aResult) => {
-          let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
-                          createInstance(Ci.nsIScriptableUnicodeConverter);
-          converter.charset = "UTF-8";
-          try {
-            let jsonString = converter.convertFromByteArray(aResult,
-                                                            aResult.length);
-            resolve(this.importFromJSON(jsonString));
-          } catch (ex) {
-            Cu.reportError("Failed to import from URL: " + ex);
-            reject(ex);
-          }
-        }
-      };
+  async importFromURL(spec) {
+    let nodes = await (await fetch(spec)).json();
 
-      let uri = NetUtil.newURI(spec);
-      let channel = NetUtil.newChannel({
-        uri,
-        loadUsingSystemPrincipal: true
-      });
-      let streamLoader = Cc["@mozilla.org/network/stream-loader;1"]
-                           .createInstance(Ci.nsIStreamLoader);
-      streamLoader.init(streamObserver);
-      channel.asyncOpen2(streamLoader);
-    });
+    if (!nodes.children || !nodes.children.length) {
+      return;
+    }
+
+    await this.import(nodes);
   },
 
   /**
    * Import bookmarks from a compressed file.
    *
    * @param aFilePath
    *        OS.File path string of the bookmark data.
    *
    * @return {Promise}
    * @resolves When the new bookmarks have been created.
    * @rejects JavaScript exception.
    */
   importFromCompressedFile: async function BI_importFromCompressedFile(aFilePath) {
       let aResult = await OS.File.read(aFilePath, { compression: "lz4" });
-      let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
-                        createInstance(Ci.nsIScriptableUnicodeConverter);
-      converter.charset = "UTF-8";
-      let jsonString = converter.convertFromByteArray(aResult, aResult.length);
+      let decoder = new TextDecoder();
+      let jsonString = decoder.decode(aResult);
       await this.importFromJSON(jsonString);
   },
 
   /**
    * Import bookmarks from a JSON string.
    *
    * @param {String} aString JSON string of serialized bookmark data.
    * @return {Promise}
@@ -232,19 +210,23 @@ BookmarkImporter.prototype = {
     let nodes =
       PlacesUtils.unwrapNodes(aString, PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER);
 
     if (nodes.length == 0 || !nodes[0].children ||
         nodes[0].children.length == 0) {
       return;
     }
 
-    // Change to nodes[0].children as we don't import the root, and also filter
+    await this.import(nodes[0]);
+  },
+
+  async import(rootNode) {
+    // Change to rootNode.children as we don't import the root, and also filter
     // out any obsolete "tagsFolder" sections.
-    nodes = nodes[0].children.filter(node => !node.root || node.root != "tagsFolder");
+    let nodes = rootNode.children.filter(node => node.root !== "tagsFolder");
 
     // If we're replacing, then erase existing bookmarks first.
     if (this._replace) {
       await PlacesUtils.bookmarks.eraseEverything({ source: this._source });
     }
 
     let folderIdToGuidMap = {};
     let searchGuids = [];
--- a/toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js
+++ b/toolkit/components/places/tests/bookmarks/test_1016953-renaming-uncompressed.js
@@ -22,21 +22,19 @@ add_task(async function test_same_date_s
   await OS.File.move(tempPath, backupFile);
 
   // Force a compressed backup which fallbacks to rename
   await PlacesBackups.create();
   let mostRecentBackupFile = await PlacesBackups.getMostRecentBackup();
   // check to ensure not renamed to jsonlz4
   Assert.equal(mostRecentBackupFile, backupFile);
   // inspect contents and check if valid json
-  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
-                        createInstance(Ci.nsIScriptableUnicodeConverter);
-  converter.charset = "UTF-8";
   let result = await OS.File.read(mostRecentBackupFile);
-  let jsonString = converter.convertFromByteArray(result, result.length);
+  let decoder = new TextDecoder();
+  let jsonString = decoder.decode(result);
   info("Check is valid JSON");
   JSON.parse(jsonString);
 
   // Cleanup
   await OS.File.remove(backupFile);
   await OS.File.remove(tempPath);
   PlacesBackups._backupFiles = null; // To force re-cache of backupFiles
 });
@@ -52,21 +50,19 @@ add_task(async function test_same_date_d
   let filename = "bookmarks-" + PlacesBackups.toISODateString(dateObj) + "_" +
                   count + "_differentHash==.json";
   let backupFile = OS.Path.join(backupFolder, filename);
   await OS.File.move(tempPath, backupFile);
   await PlacesBackups.create(); // Force compressed backup
   let mostRecentBackupFile = await PlacesBackups.getMostRecentBackup();
 
   // Decode lz4 compressed file to json and check if json is valid
-  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
-                        createInstance(Ci.nsIScriptableUnicodeConverter);
-  converter.charset = "UTF-8";
   let result = await OS.File.read(mostRecentBackupFile, { compression: "lz4" });
-  let jsonString = converter.convertFromByteArray(result, result.length);
+  let decoder = new TextDecoder();
+  let jsonString = decoder.decode(result);
   info("Check is valid JSON");
   JSON.parse(jsonString);
 
   // Cleanup
   await OS.File.remove(mostRecentBackupFile);
   await OS.File.remove(tempPath);
   PlacesBackups._backupFiles = null; // To force re-cache of backupFiles
 });
--- a/toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
+++ b/toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
@@ -130,53 +130,50 @@ add_task(async function test_json_restor
   let file = await promiseFile("bookmarks-test_restoreNotification.json");
   await addBookmarks();
 
   await BookmarkJSONUtils.exportToFile(file);
   await PlacesUtils.bookmarks.eraseEverything();
   try {
     await BookmarkJSONUtils.importFromFile(file, { replace: true });
   } catch (e) {
-    do_throw("  Restore should not have failed" + e);
+    do_throw("  Restore should not have failed " + e);
   }
 
   await checkObservers(expectPromises, expectedData);
   await teardown(file);
 });
 
 add_task(async function test_json_restore_empty() {
   let expectedData = {
     data:       NSIOBSERVER_DATA_JSON,
     folderId:   null
   };
-  let expectPromises = registerObservers(true);
+  let expectPromises = registerObservers(false);
 
-  info("JSON restore: empty file should succeed");
+  info("JSON restore: empty file should fail");
   let file = await promiseFile("bookmarks-test_restoreNotification.json");
-  try {
-    await BookmarkJSONUtils.importFromFile(file, { replace: true });
-  } catch (e) {
-    do_throw("  Restore should not have failed" + e);
-  }
+  await Assert.rejects(BookmarkJSONUtils.importFromFile(file, { replace: true }),
+    /SyntaxError/, "Restore should reject for an empty file.");
 
   await checkObservers(expectPromises, expectedData);
   await teardown(file);
 });
 
 add_task(async function test_json_restore_nonexist() {
   let expectedData = {
     data:       NSIOBSERVER_DATA_JSON,
     folderId:   null
   };
   let expectPromises = registerObservers(false);
 
   info("JSON restore: nonexistent file should fail");
   let file = Services.dirsvc.get("ProfD", Ci.nsIFile);
   file.append("this file doesn't exist because nobody created it 1");
-  Assert.rejects(BookmarkJSONUtils.importFromFile(file.path, { replace: true }),
+  await Assert.rejects(BookmarkJSONUtils.importFromFile(file.path, { replace: true }),
     /Cannot restore from nonexisting json file/, "Restore should reject for a non-existent file.");
 
   await checkObservers(expectPromises, expectedData);
   await teardown(file);
 });
 
 add_task(async function test_html_restore_normal() {
   let expectedData = {