Bug 1295510 - Replace all uses of `new String` with POJOs in Sync. r=eoger,tcsc
authorDylan Turner <dylan.turner@cgs.act.edu.au>, Kit Cambridge <kit@yakshaving.ninja>
Fri, 16 Mar 2018 17:38:17 -0700
changeset 408842 17a2f8a774888c9e5cc8857de0a78664b1ef63cb
parent 408841 3fff2bcc9e0ef0b460eacb741459ef1a93974085
child 408843 b9cb68851e9ba6122f86fe5ae601ed78f17ac43e
push id61359
push userkcambridge@mozilla.com
push dateMon, 19 Mar 2018 21:42:35 +0000
treeherderautoland@17a2f8a77488 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerseoger, tcsc
bugs1295510
milestone61.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 1295510 - Replace all uses of `new String` with POJOs in Sync. r=eoger,tcsc MozReview-Commit-ID: K9jni7AbPsu
services/sync/modules/bookmark_repair.js
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/modules/record.js
services/sync/modules/resource.js
services/sync/modules/service.js
services/sync/tests/unit/head_http_server.js
services/sync/tests/unit/test_collection_getBatched.js
services/sync/tests/unit/test_resource.js
services/sync/tests/unit/test_resource_header.js
--- a/services/sync/modules/bookmark_repair.js
+++ b/services/sync/modules/bookmark_repair.js
@@ -654,17 +654,17 @@ class BookmarkRepairResponder extends Co
     // Throwing here means we abort the repair, which isn't ideal for transient
     // errors (eg, no network, 500 service outage etc), but we don't currently
     // have a sane/safe way to try again later (we'd need to implement a kind
     // of timeout, otherwise we might end up retrying forever and never remove
     // our request command.) Bug 1347805.
     if (!itemsResponse.success) {
       throw new Error(`request for server IDs failed: ${itemsResponse.status}`);
     }
-    let existRemotely = new Set(JSON.parse(itemsResponse));
+    let existRemotely = new Set(itemsResponse.obj);
     // We need to be careful about handing the requested items:
     // * If the item exists locally but isn't in the tree of items we sync
     //   (eg, it might be a left-pane item or similar, we write a tombstone.
     // * If the item exists locally as a folder, we upload the folder and any
     //   children which don't exist on the server. (Note that we assume the
     //   parents *do* exist)
     // Bug 1343101 covers additional issues we might repair in the future.
     for (let { recordId: id, syncable } of repairable) {
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -1003,19 +1003,17 @@ SyncEngine.prototype = {
 
       // Put the new data back into meta/global and mark for upload
       engines[this.name] = engineData;
       metaGlobal.payload.engines = engines;
       metaGlobal.changed = true;
     } else if (engineData.version > this.version) {
       // Don't sync this engine if the server has newer data
 
-      // Changes below need to be processed in bug 1295510 that's why eslint is ignored
-      // eslint-disable-next-line no-new-wrappers
-      let error = new String("New data: " + [engineData.version, this.version]);
+      let error = new Error("New data: " + [engineData.version, this.version]);
       error.failureCode = VERSION_OUT_OF_DATE;
       throw error;
     } else {
       // Changes to syncID mean we'll need to upload everything
       let assignedSyncID = await this.ensureCurrentSyncID(engineData.syncID);
       if (assignedSyncID != engineData.syncID) {
         engineData.syncID = assignedSyncID;
         metaGlobal.changed = true;
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -515,21 +515,20 @@ BookmarksEngine.prototype = {
           continue;
       }
 
       let parentName = parent.title || "";
       if (guidMap[parentName] == null)
         guidMap[parentName] = {};
 
       // If the entry already exists, remember that there are explicit dupes.
-
-      // Changes below need to be processed in bug 1295510 that's why eslint is ignored
-      // eslint-disable-next-line no-new-wrappers
-      let entry = new String(guid);
-      entry.hasDupe = guidMap[parentName][key] != null;
+      let entry = {
+        guid,
+        hasDupe: guidMap[parentName][key] != null,
+      };
 
       // Remember this item's GUID for its parent-name/key pair.
       guidMap[parentName][key] = entry;
       this._log.trace("Mapped: " + [parentName, key, entry, entry.hasDupe]);
     }
 
     return guidMap;
   },
@@ -576,24 +575,24 @@ BookmarksEngine.prototype = {
     if (!parent) {
       this._log.trace("No parent => no dupe.");
       return undefined;
     }
 
     let dupe = parent[key];
 
     if (dupe) {
-      this._log.trace("Mapped dupe: " + dupe);
+      this._log.trace("Mapped dupe", dupe);
       return dupe;
     }
 
     if (altKey) {
       dupe = parent[altKey];
       if (dupe) {
-        this._log.trace("Mapped dupe using altKey " + altKey + ": " + dupe);
+        this._log.trace("Mapped dupe using altKey " + altKey, dupe);
         return dupe;
       }
     }
 
     this._log.trace("No dupe found for key " + key + "/" + altKey + ".");
     return undefined;
   },
 
@@ -712,20 +711,18 @@ BookmarksEngine.prototype = {
                     " (already duped: " + item.hasDupe + ").");
 
     // Don't bother finding a dupe if the incoming item has duplicates.
     if (item.hasDupe) {
       this._log.trace(item.id + " already a dupe: not finding one.");
       return null;
     }
     let mapped = await this._mapDupe(item);
-    this._log.debug(item.id + " mapped to " + mapped);
-    // We must return a string, not an object, and the entries in the GUIDMap
-    // are created via "new String()" making them an object.
-    return mapped ? mapped.toString() : mapped;
+    this._log.debug(item.id + " mapped to", mapped);
+    return mapped ? mapped.guid : null;
   },
 
   // Called when _findDupe returns a dupe item and the engine has decided to
   // switch the existing item to the new incoming item.
   async _switchItemToDupe(localDupeGUID, incomingItem) {
     let newChanges = await PlacesSyncUtils.bookmarks.dedupe(localDupeGUID,
                                                             incomingItem.id,
                                                             incomingItem.parentid);
--- a/services/sync/modules/record.js
+++ b/services/sync/modules/record.js
@@ -41,17 +41,17 @@ WBORecord.prototype = {
   // Set thine 'response' field.
   async fetch(resource) {
     if (!(resource instanceof Resource)) {
       throw new Error("First argument must be a Resource instance.");
     }
 
     let r = await resource.get();
     if (r.success) {
-      this.deserialize(r); // Warning! Muffles exceptions!
+      this.deserialize(r.obj); // Warning! Muffles exceptions!
     }
     this.response = r;
     return this;
   },
 
   upload(resource) {
     if (!(resource instanceof Resource)) {
       throw new Error("First argument must be a Resource instance.");
@@ -67,18 +67,20 @@ WBORecord.prototype = {
       let url = CommonUtils.makeURI(base + this.collection + "/" + this.id);
       url.QueryInterface(Ci.nsIURL);
       return url;
     }
     return null;
   },
 
   deserialize: function deserialize(json) {
-    this.data = json.constructor.toString() == String ? JSON.parse(json) : json;
-
+    if (!json || typeof json !== "object") {
+      throw new TypeError("Can't deserialize record from: " + json);
+    }
+    this.data = json;
     try {
       // The payload is likely to be JSON, but if not, keep it as a string
       this.payload = JSON.parse(this.payload);
     } catch (ex) {}
   },
 
   toJSON: function toJSON() {
     // Copy fields from data to be stringified, making sure payload is a string
@@ -233,17 +235,17 @@ RecordManager.prototype = {
       this.response = {};
       this.response = await this.service.resource(url).get();
 
       // Don't parse and save the record on failure
       if (!this.response.success)
         return null;
 
       let record = new this._recordType(url);
-      record.deserialize(this.response);
+      record.deserialize(this.response.obj);
 
       return this.set(url, record);
     } catch (ex) {
       if (Async.isShutdownException(ex)) {
         throw ex;
       }
       this._log.debug("Failed to import record", ex);
       return null;
--- a/services/sync/modules/resource.js
+++ b/services/sync/modules/resource.js
@@ -186,39 +186,38 @@ Resource.prototype = {
     return this._processResponse(response, method);
   },
 
   async _processResponse(response, method) {
     const data = await response.text();
     this._logResponse(response, method, data);
     this._processResponseHeaders(response);
 
-    // Changes below need to be processed in bug 1295510 that's why eslint is ignored
-    // eslint-disable-next-line no-new-wrappers
-    const ret   = new String(data);
-    ret.url     = response.url;
-    ret.status  = response.status;
-    ret.success = response.ok;
-    ret.headers = {};
+    const ret = {
+      data,
+      url: response.url,
+      status: response.status,
+      success: response.ok,
+      headers: {},
+    };
     for (const [k, v] of response.headers) {
       ret.headers[k] = v;
     }
 
     // Make a lazy getter to convert the json response into an object.
     // Note that this can cause a parse error to be thrown far away from the
     // actual fetch, so be warned!
     XPCOMUtils.defineLazyGetter(ret, "obj", () => {
       try {
-        return JSON.parse(ret);
+        return JSON.parse(ret.data);
       } catch (ex) {
         this._log.warn("Got exception parsing response body", ex);
         // Stringify to avoid possibly printing non-printable characters.
-        this._log.debug("Parse fail: Response body starts: \"" +
-                        JSON.stringify((ret + "").slice(0, 100)) +
-                        "\".");
+        this._log.debug("Parse fail: Response body starts",
+                        (ret.data + "").slice(0, 100));
         throw ex;
       }
     });
 
     return ret;
   },
 
   _logResponse(response, method, data) {
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -746,17 +746,17 @@ Sync11Service.prototype = {
     }
     this._log.info("Got status " + uploadRes.status + " uploading keys.");
     let serverModified = uploadRes.obj; // Modified timestamp according to server.
     this._log.debug("Server reports crypto modified: " + serverModified);
 
     // Now verify that info/collections shows them!
     this._log.debug("Verifying server collection records.");
     let info = await this._fetchInfo();
-    this._log.debug("info/collections is: " + info);
+    this._log.debug("info/collections is: " + info.data);
 
     if (info.status != 200) {
       this._log.warn("Non-200 info/collections response. Aborting.");
       throw new Error("Unable to upload symmetric keys.");
     }
 
     info = info.obj;
     if (!(CRYPTO_COLLECTION in info)) {
--- a/services/sync/tests/unit/head_http_server.js
+++ b/services/sync/tests/unit/head_http_server.js
@@ -85,17 +85,17 @@ function ServerWBO(id, initialPayload, m
 }
 ServerWBO.prototype = {
 
   get data() {
     return JSON.parse(this.payload);
   },
 
   get() {
-    return JSON.stringify(this, ["id", "modified", "payload"]);
+    return { id: this.id, modified: this.modified, payload: this.payload };
   },
 
   put(input) {
     input = JSON.parse(input);
     this.payload = input.payload;
     this.modified = new_timestamp();
     this.sortindex = input.sortindex || 0;
   },
@@ -115,17 +115,17 @@ ServerWBO.prototype = {
     return function(request, response) {
       var statusCode = 200;
       var status = "OK";
       var body;
 
       switch (request.method) {
         case "GET":
           if (self.payload) {
-            body = self.get();
+            body = JSON.stringify(self.get());
           } else {
             statusCode = 404;
             status = "Not Found";
             body = "Not Found";
           }
           break;
 
         case "PUT":
--- a/services/sync/tests/unit/test_collection_getBatched.js
+++ b/services/sync/tests/unit/test_collection_getBatched.js
@@ -2,17 +2,17 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 ChromeUtils.import("resource://services-sync/record.js");
 ChromeUtils.import("resource://services-sync/service.js");
 
 function recordRange(lim, offset, total) {
   let res = [];
   for (let i = offset; i < Math.min(lim + offset, total); ++i) {
-    res.push(JSON.stringify({ id: String(i), payload: "test:" + i }));
+    res.push({ id: String(i), payload: "test:" + i });
   }
   return res;
 }
 
 function get_test_collection_info({ totalRecords, batchSize, lastModified,
                                     throwAfter = Infinity,
                                     interruptedAfter = Infinity }) {
   let coll = new Collection("http://example.com/test/", WBORecord, Service);
--- a/services/sync/tests/unit/test_resource.js
+++ b/services/sync/tests/unit/test_resource.js
@@ -172,17 +172,17 @@ add_task(async function test_proxy_auth_
   });
 
   PACSystemSettings.PACURI = server.baseURI + "/pac2";
   installFakePAC();
   let res = new Resource(server.baseURI + "/open");
   let result = await res.get();
   Assert.ok(pacFetched);
   Assert.ok(fetched);
-  Assert.equal("This path exists", result);
+  Assert.equal("This path exists", result.data);
   pacFetched = fetched = false;
   uninstallFakePAC();
   await promiseStopServer(server);
 });
 
 add_task(async function test_new_channel() {
   _("Ensure a redirect to a new channel is handled properly.");
 
@@ -251,41 +251,41 @@ add_test(function test_members() {
 
   run_next_test();
 });
 
 add_task(async function test_get() {
   _("GET a non-password-protected resource");
   let res = new Resource(server.baseURI + "/open");
   let content = await res.get();
-  Assert.equal(content, "This path exists");
+  Assert.equal(content.data, "This path exists");
   Assert.equal(content.status, 200);
   Assert.ok(content.success);
 
   // Observe logging messages.
   let resLogger = res._log;
   let dbg    = resLogger.debug;
   let debugMessages = [];
-  resLogger.debug = function(msg) {
-    debugMessages.push(msg);
+  resLogger.debug = function(msg, extra) {
+    debugMessages.push(`${msg}: ${JSON.stringify(extra)}`);
     dbg.call(this, msg);
   };
 
   // Since we didn't receive proper JSON data, accessing content.obj
   // will result in a SyntaxError from JSON.parse
   let didThrow = false;
   try {
     content.obj;
   } catch (ex) {
     didThrow = true;
   }
   Assert.ok(didThrow);
   Assert.equal(debugMessages.length, 1);
   Assert.equal(debugMessages[0],
-               "Parse fail: Response body starts: \"\"This path exists\"\".");
+               "Parse fail: Response body starts: \"This path exists\"");
   resLogger.debug = dbg;
 });
 
 add_test(function test_basicauth() {
   _("Test that the BasicAuthenticator doesn't screw up header case.");
   let res1 = new Resource(server.baseURI + "/foo");
   res1.setHeader("Authorization", "Basic foobar");
   Assert.equal(res1._headers.authorization, "Basic foobar");
@@ -293,95 +293,95 @@ add_test(function test_basicauth() {
 
   run_next_test();
 });
 
 add_task(async function test_get_protected_fail() {
   _("GET a password protected resource (test that it'll fail w/o pass, no throw)");
   let res2 = new Resource(server.baseURI + "/protected");
   let content = await res2.get();
-  Assert.equal(content, "This path exists and is protected - failed");
+  Assert.equal(content.data, "This path exists and is protected - failed");
   Assert.equal(content.status, 401);
   Assert.ok(!content.success);
 });
 
 add_task(async function test_get_protected_success() {
   _("GET a password protected resource");
   let identityConfig = makeIdentityConfig();
   let browseridManager = new BrowserIDManager();
   configureFxAccountIdentity(browseridManager, identityConfig);
   let auth = browseridManager.getResourceAuthenticator();
   let res3 = new Resource(server.baseURI + "/protected");
   res3.authenticator = auth;
   Assert.equal(res3.authenticator, auth);
   let content = await res3.get();
-  Assert.equal(content, "This path exists and is protected");
+  Assert.equal(content.data, "This path exists and is protected");
   Assert.equal(content.status, 200);
   Assert.ok(content.success);
 });
 
 add_task(async function test_get_404() {
   _("GET a non-existent resource (test that it'll fail, but not throw)");
   let res4 = new Resource(server.baseURI + "/404");
   let content = await res4.get();
-  Assert.equal(content, "File not found");
+  Assert.equal(content.data, "File not found");
   Assert.equal(content.status, 404);
   Assert.ok(!content.success);
 
   // Check some headers of the 404 response
   Assert.equal(content.headers.connection, "close");
   Assert.equal(content.headers.server, "httpd.js");
   Assert.equal(content.headers["content-length"], 14);
 });
 
 add_task(async function test_put_string() {
   _("PUT to a resource (string)");
   let res_upload = new Resource(server.baseURI + "/upload");
   let content = await res_upload.put(JSON.stringify(sample_data));
-  Assert.equal(content, "Valid data upload via PUT");
+  Assert.equal(content.data, "Valid data upload via PUT");
   Assert.equal(content.status, 200);
 });
 
 add_task(async function test_put_object() {
   _("PUT to a resource (object)");
   let res_upload = new Resource(server.baseURI + "/upload");
   let content = await res_upload.put(sample_data);
-  Assert.equal(content, "Valid data upload via PUT");
+  Assert.equal(content.data, "Valid data upload via PUT");
   Assert.equal(content.status, 200);
 });
 
 add_task(async function test_post_string() {
   _("POST to a resource (string)");
   let res_upload = new Resource(server.baseURI + "/upload");
   let content = await res_upload.post(JSON.stringify(sample_data));
-  Assert.equal(content, "Valid data upload via POST");
+  Assert.equal(content.data, "Valid data upload via POST");
   Assert.equal(content.status, 200);
 });
 
 add_task(async function test_post_object() {
   _("POST to a resource (object)");
   let res_upload = new Resource(server.baseURI + "/upload");
   let content = await res_upload.post(sample_data);
-  Assert.equal(content, "Valid data upload via POST");
+  Assert.equal(content.data, "Valid data upload via POST");
   Assert.equal(content.status, 200);
 });
 
 add_task(async function test_delete() {
   _("DELETE a resource");
   let res6 = new Resource(server.baseURI + "/delete");
   let content = await res6.delete();
-  Assert.equal(content, "This resource has been deleted");
+  Assert.equal(content.data, "This resource has been deleted");
   Assert.equal(content.status, 200);
 });
 
 add_task(async function test_json_body() {
   _("JSON conversion of response body");
   let res7 = new Resource(server.baseURI + "/json");
   let content = await res7.get();
-  Assert.equal(content, JSON.stringify(sample_data));
+  Assert.equal(content.data, JSON.stringify(sample_data));
   Assert.equal(content.status, 200);
   Assert.equal(JSON.stringify(content.obj), JSON.stringify(sample_data));
 });
 
 add_task(async function test_weave_timestamp() {
   _("X-Weave-Timestamp header updates Resource.serverTime");
   // Before having received any response containing the
   // X-Weave-Timestamp header, Resource.serverTime is null.
@@ -390,69 +390,69 @@ add_task(async function test_weave_times
   await res8.get();
   Assert.equal(Resource.serverTime, TIMESTAMP);
 });
 
 add_task(async function test_get_no_headers() {
   _("GET: no special request headers");
   let res_headers = new Resource(server.baseURI + "/headers");
   let content = await res_headers.get();
-  Assert.equal(content, "{}");
+  Assert.equal(content.data, "{}");
 });
 
 add_task(async function test_put_default_content_type() {
   _("PUT: Content-Type defaults to text/plain");
   let res_headers = new Resource(server.baseURI + "/headers");
   let content = await res_headers.put("data");
-  Assert.equal(content, JSON.stringify({"content-type": "text/plain"}));
+  Assert.equal(content.data, JSON.stringify({"content-type": "text/plain"}));
 });
 
 add_task(async function test_post_default_content_type() {
   _("POST: Content-Type defaults to text/plain");
   let res_headers = new Resource(server.baseURI + "/headers");
   let content = await res_headers.post("data");
-  Assert.equal(content, JSON.stringify({"content-type": "text/plain"}));
+  Assert.equal(content.data, JSON.stringify({"content-type": "text/plain"}));
 });
 
 add_task(async function test_setHeader() {
   _("setHeader(): setting simple header");
   let res_headers = new Resource(server.baseURI + "/headers");
   res_headers.setHeader("X-What-Is-Weave", "awesome");
   Assert.equal(res_headers.headers["x-what-is-weave"], "awesome");
   let content = await res_headers.get();
-  Assert.equal(content, JSON.stringify({"x-what-is-weave": "awesome"}));
+  Assert.equal(content.data, JSON.stringify({"x-what-is-weave": "awesome"}));
 });
 
 add_task(async function test_setHeader_overwrite() {
   _("setHeader(): setting multiple headers, overwriting existing header");
   let res_headers = new Resource(server.baseURI + "/headers");
   res_headers.setHeader("X-WHAT-is-Weave", "more awesomer");
   res_headers.setHeader("X-Another-Header", "hello world");
   Assert.equal(res_headers.headers["x-what-is-weave"], "more awesomer");
   Assert.equal(res_headers.headers["x-another-header"], "hello world");
   let content = await res_headers.get();
-  Assert.equal(content, JSON.stringify({"x-another-header": "hello world",
-                                        "x-what-is-weave": "more awesomer"}));
+  Assert.equal(content.data, JSON.stringify({"x-another-header": "hello world",
+                                             "x-what-is-weave": "more awesomer"}));
 });
 
 add_task(async function test_put_override_content_type() {
   _("PUT: override default Content-Type");
   let res_headers = new Resource(server.baseURI + "/headers");
   res_headers.setHeader("Content-Type", "application/foobar");
   Assert.equal(res_headers.headers["content-type"], "application/foobar");
   let content = await res_headers.put("data");
-  Assert.equal(content, JSON.stringify({"content-type": "application/foobar"}));
+  Assert.equal(content.data, JSON.stringify({"content-type": "application/foobar"}));
 });
 
 add_task(async function test_post_override_content_type() {
   _("POST: override default Content-Type");
   let res_headers = new Resource(server.baseURI + "/headers");
   res_headers.setHeader("Content-Type", "application/foobar");
   let content = await res_headers.post("data");
-  Assert.equal(content, JSON.stringify({"content-type": "application/foobar"}));
+  Assert.equal(content.data, JSON.stringify({"content-type": "application/foobar"}));
 });
 
 add_task(async function test_weave_backoff() {
   _("X-Weave-Backoff header notifies observer");
   let backoffInterval;
   function onBackoff(subject, data) {
     backoffInterval = subject;
   }
--- a/services/sync/tests/unit/test_resource_header.js
+++ b/services/sync/tests/unit/test_resource_header.js
@@ -44,16 +44,16 @@ add_task(async function test_headers_cop
   triggerRedirect();
 
   _("Issuing request.");
   let resource = new Resource(TEST_URL);
   resource.setHeader("Authorization", "Basic foobar");
   resource.setHeader("X-Foo", "foofoo");
 
   let result = await resource.get(TEST_URL);
-  _("Result: " + result);
+  _("Result: " + result.data);
 
-  Assert.equal(result, BODY);
+  Assert.equal(result.data, BODY);
   Assert.equal(auth, "Basic foobar");
   Assert.equal(foo, "foofoo");
 
   await promiseStopServer(httpServer);
 });