Closed Bug 1295510 Opened 4 years ago Closed 2 years ago

Replace all uses of `new String` with POJOs

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

(Whiteboard: [sync-quality] )

Attachments

(1 file, 1 obsolete file)

Using `new String` to hang properties off string objects obscures intent, and causes bugs like bug 1295410. These objects are never equal to one another, and can also mask subtle issues (for example, `BookmarkSpecialIds.guids.includes(new String("menu")) === false`).

If we want an object, let's use an object.
Priority: -- → P3
Whiteboard: [sync-quality]
The only remaining uses I see are https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/services/sync/modules/resource.js#304, https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/services/sync/modules/engines/bookmarks.js#360, and https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/services/sync/modules/engines.js#1012.

For resource.js, we want to replace `ret` with a plain object like `{ body, url, status, success, headers }`, and update all callers—`.get(), `.put(...)`, `.post(...)`, and `.delete()`—that use it. This is going to be the most time-consuming part, I think. We have reasonably good test coverage for this; `./mach xpcshell-test services/sync` should highlight any failures.

For bookmarks.js, let's replace `entry` with an object like `{ guid, hasDupe }`, and update callers like https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/services/sync/modules/engines/bookmarks.js#568. There may be others, too. Our tests for this are sparse, but at least `./mach xpcshell-test services/sync/tests/unit/test_bookmark_duping.js` should pass.

In engines.js, we can just replace `new String` with `new Error`.
Mentor: kit
Keywords: good-first-bug
Hey! Can I take this bug up? Also, since I am a beginner, can you help me to get started?
Flags: needinfo?(kit)
Hi, welcome and thanks for your interest! Check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction for how to get started. You'll want to clone and build Firefox first, using the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build. `mach bootstrap` will ask you what kind of build you want; you can choose "Firefox for Desktop Artifact Mode" for faster builds.

In comment 1, I linked to all the places we use `new String` in `services/sync`.

First is `services/sync/modules/resource.js`. You'll want to replace `let ret = new String(data)` and all the lines that set properties on `ret` (`ret.url`, `ret.status`, and so on) with a plain object like `{ data, status, url: channel.URI.spec }`, and replace all existing uses of `ret` with `ret.data`. This might be tricky, but I think most code uses `ret.obj` (http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/services/sync/modules/resource.js#315-329) instead of treating the response as a string, so the conversion might be easier than I thought.

Next, http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/services/sync/modules/engines/bookmarks.js#361-364. Please replace that with an object like `let entry = { guid, hasDupe: guidMap[parentName][key] != null }`, and convert everything that does a lookup in the GUID map. I think that's just `_mapDupe` at this point.

Finally, http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/services/sync/modules/engines.js#1013-1017. This should be `new Error` instead, it doesn't need to be `new String`.

Once you've made the changes, please make sure the tests pass by running `./mach xpcshell-test services/sync`. If there are any failures, fix up the issues and rerun the failing tests. Once everything looks good, you can attach a patch to this bug. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch covers how to do this. :-)

Thanks again for contributing!
Flags: needinfo?(kit)
I've started working on the bug, though was facing some problems with the setup. I'll try to send the patch as soon as possible.
Hey can i work on this bug? It should be an easy fix.
Sure!
Kit: Just now Started working on this bug, I hope, will submit patch very soon.
Attached file patch (obsolete) —
Attachment #8944598 - Flags: feedback?(kit)
Hi, I would love to work on this bug.
(In reply to taha.zulfiqar from comment #9)
> Hi, I would love to work on this bug.

That sounds great - please review the comments above and let us know if you need any help to get a patch up.
For some reason, the given command './mach xpcshell-test services/sync' always causes me to prompt. Any suggestions?


I do believe I have the right logic down for the patch though. I created a function that generates POJO. And had it called where ret was.
Flags: needinfo?(kit)
(In reply to taha.zulfiqar from comment #11)
> For some reason, the given command './mach xpcshell-test services/sync'
> always causes me to prompt. Any suggestions?

Hi there! Could you post the message you're seeing when you run `./mach xpcshell-test services/sync`, please?
Flags: needinfo?(kit)
Sorry, it's running now! I'll keep you updated if I find any inconveniences!
Flags: needinfo?(kit)
Untangling the tests is really gnarly, so I'll just take this now. Taha, can I interest you in another bug on https://bugzil.la/sw:%22[good%20first%20bug]%22&limit=0 or http://www.joshmatthews.net/bugsahoy/? :-)
Assignee: nobody → kit
Mentor: kit
Status: NEW → ASSIGNED
Flags: needinfo?(kit)
Keywords: good-first-bug
Comment on attachment 8944598 [details]
patch

Thanks for starting on this, Dylan, and sorry for the delay in getting back to you! I've carried your work forward, and left your name on the commit, so you'll still get cred for contributing to Firefox. :-)
Attachment #8944598 - Attachment is obsolete: true
Attachment #8944598 - Flags: feedback?(kit)
For future reference, or for any folks interested, I used this trick to make response into a poison pill that threw when misused:

const ret = new Proxy({
  data,
  url: response.url,
  status: response.status,
  success: response.ok,
  headers: {},
  
  valueOf() {
    throw new TypeError("Can't coerce response to value");
  },
  toString() {
    throw new TypeError("Can't coerce response to string");
  },
}, {
  get(target, prop) {
    if (prop == "then" || /* Checked by JS runtime when returning from an async function. */
        prop == "action" || /* Used by Log.jsm. */
        prop == "toJSON" ||
        prop == "result" || /* We do `throw response;` in various places; this allows those. */
        prop == "code" ||
        prop == "appIsShuttingDown") {
      return undefined;
    }
    if (typeof prop == "symbol") {
      return target[prop];
    }
    if (!(prop in target)) {
      throw new TypeError(`Can't access nonexistent property ${prop} of response`);
    }
    return target[prop];
  },
});
Comment on attachment 8959736 [details]
Bug 1295510 - Replace all uses of `new String` with POJOs in Sync.

https://reviewboard.mozilla.org/r/228582/#review234428

This looks great thank you!
Attachment #8959736 - Flags: review?(eoger) → review+
Can you make sure to check how bad the fallout to about:sync is with this patch? I suspect we might be able to avoid breaking the API quite as badly without much work/complexity (but it's also possible this is fine)
It looks like our test server adds one more layer of stringification for records in a batch GET. I *think* this is different from using JSON as fake encryption that we talked about on IRC, but maybe not?

With the latest revision, the xpcshell tests, About Sync, and TPS (at least, `test_sync.js`) work properly, and don't need any changes. Thom, could you take a look, please?
Comment on attachment 8959736 [details]
Bug 1295510 - Replace all uses of `new String` with POJOs in Sync.

https://reviewboard.mozilla.org/r/228582/#review234796

This looks basically fine. (and I'm glad we were able to avoid breaking things unnecessarially, thanks!)

::: services/sync/tests/unit/head_http_server.js:123
(Diff revision 2)
>        var body;
>  
>        switch (request.method) {
>          case "GET":
>            if (self.payload) {
> -            body = self.get();
> +            body = JSON.stringify(self.get());

I'm not exactly sure why this change (and the one for get()) is made, it doesn't seem related or necessary.

I'm not actually opposed to it if there's a benefit, but it seems like a bit of a wash, to be honest.
Attachment #8959736 - Flags: review?(tchiovoloni) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17a2f8a77488
Replace all uses of `new String` with POJOs in Sync. r=eoger,tcsc
https://hg.mozilla.org/mozilla-central/rev/17a2f8a77488
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.