Bug 1295510 - Replace all uses of `new String` with POJOs in Sync. r=eoger,tcsc draft
authorDylan Turner <dylan.turner@cgs.act.edu.au>, Kit Cambridge <kit@yakshaving.ninja>
Fri, 16 Mar 2018 17:38:17 -0700
changeset 769626 aa7164ad089bffbff4b9be3c0b056d83b82eb635
parent 769595 c8dbb4ed05f38f40ef3607a6e36545bd95b8a287
push id103193
push userbmo:kit@mozilla.com
push dateMon, 19 Mar 2018 21:39:37 +0000
reviewerseoger, tcsc
bugs1295510
milestone61.0a1
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);
 });