1295510 - Replace all uses of `new String` with POJOs

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Lina Butler [:lina]]

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.

Comment 1

[Lina Butler [:lina]]

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`.

Comment 2

[meghasharma4910]

Hey! Can I take this bug up? Also, since I am a beginner, can you help me to get started?

Comment 3

[Lina Butler [:lina]]

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!

Comment 4

[meghasharma4910]

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.

Comment 5

[Parth]

Hey can i work on this bug? It should be an easy fix.

Comment 6

[Lina Butler [:lina]]

Sure!

Comment 7

[Manish [:manishkk]]

Kit: Just now Started working on this bug, I hope, will submit patch very soon.

Comment 8

[dylan.turner]

Attachment: patch

Comment 9

[taha.zulfiqar]

Hi, I would love to work on this bug.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 11

[taha.zulfiqar]

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.

Comment 12

[Lina Butler [:lina]]

(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?

Comment 13

[taha.zulfiqar]

Sorry, it's running now! I'll keep you updated if I find any inconveniences!

Comment 14

[Lina Butler [:lina]]

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/? :-)

Comment 15

[Lina Butler [:lina]]

Attachment: Bug 1295510 - Replace all uses of `new String` with POJOs in Sync.

Review commit: https://reviewboard.mozilla.org/r/228582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228582/

Comment 16

[Lina Butler [:lina]]

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. :-)

Comment 17

[Lina Butler [:lina]]

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 18

[Edouard Oger [:eoger]]

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!

Comment 19

[Thom Chiovoloni [:tcsc] (ex-moco)]

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)

Comment 20

[Lina Butler [:lina]]

Comment on attachment 8959736 [details]
Bug 1295510 - Replace all uses of `new String` with POJOs in Sync.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228582/diff/1-2/

Comment 21

[Lina Butler [:lina]]

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 22

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8959736 [details]
Bug 1295510 - Replace all uses of `new String` with POJOs in Sync.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228582/diff/2-3/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17a2f8a77488
Replace all uses of `new String` with POJOs in Sync. r=eoger,tcsc

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/17a2f8a77488