787273 - War on singletons part 2: electric boogaloo

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #785225 +++

Singletons are like a recurring bad dream. Kill all of the singletons!

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: Remove Records singleton, v1

See commit message.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Remove magic authentication from Resource, v1

This was a tricky patch. But, I think I got it.

I can't wait to use the unified client library so all these one-offs of Resource go away.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 3: Remove Identity from SyncStorageRequest, v1

Gave SyncStorageRequest the same treatment as Resource.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 4: Remove Identity singleton, v1

Another one bites the dust.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 5: Remove CollectionKeys singleton, v1

Pretty self-explanatory.

Comment 6

[Gregory Szorc [:gps]]

AFAICT the only singletons we have left now are Status and Service. I think I'm content with this for now.

Please commence review.

Comment 7

[Gregory Szorc [:gps]]

Attachment: Part 6: Remove Weave export from service.js, v1

Read the commit.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 7: Expose Status under service, v1

Not a full elimination of Status. Just funneling more things through Service to work towards our goal of not having Service be dependent on singletons.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 657091 [details] [diff] [review]
Part 2: Remove magic authentication from Resource, v1

Review of attachment 657091 [details] [diff] [review]:
-----------------------------------------------------------------

Two possible comments here, linked.

One is that getResource is looong. I think "Service.resource(url)", or even "Service.res(url)" would suffice.

The second, considering that length:

  let res = Service.getResource(url);
  let res = new Resource(url, Service);

which is to say: why not have a second argument to Resource be an AuthenticatorFactory, and/or have Service act as an Authenticator? This feels a little more general than using the Service as a ResourceFactory, and actually more closely mirrors Android, let alone your own approach to Collection.

Not a formal objection, but think about it.

::: services/sync/modules/record.js
@@ +520,5 @@
>    Resource.call(this, uri);
> +
> +  // This is a bit hacky, but gets the job done.
> +  let res = service.getResource(uri);
> +  this.authenticator = res.authenticator;

See Overview comment, but do you see how s/service/authenticatorFactory can help here?

::: services/sync/modules/resource.js
@@ +21,5 @@
>  
>  /*
>   * AsyncResource represents a remote network resource, identified by a URI.
>   * Create an instance like so:
>   * 

Trailing whitespace from history.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 657097 [details] [diff] [review]
Part 3: Remove Identity from SyncStorageRequest, v1

Review of attachment 657097 [details] [diff] [review]:
-----------------------------------------------------------------

r+ modulo naming.

::: services/sync/modules/service.js
@@ +483,5 @@
>  
>    /**
> +   * Obtain a SyncStorageRequest instance with authentication credentials.
> +   */
> +  getRESTRequest: function getRESTRequest(url) {

getStorageRequest.

Comment 11

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #9)
> One is that getResource is looong. I think "Service.resource(url)", or even
> "Service.res(url)" would suffice.
> 
> The second, considering that length:
> 
>   let res = Service.getResource(url);
>   let res = new Resource(url, Service);
> 
> which is to say: why not have a second argument to Resource be an
> AuthenticatorFactory, and/or have Service act as an Authenticator? This
> feels a little more general than using the Service as a ResourceFactory, and
> actually more closely mirrors Android, let alone your own approach to
> Collection.

I agree with you. If I did it again, I'd do it this way. But considering Resource is deprecated, 0fg.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 657155 [details] [diff] [review]
Part 4: Remove Identity singleton, v1

Review of attachment 657155 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/main.js
@@ +11,5 @@
>    "notifications.js":     ["Notifications", "Notification", "NotificationButton"],
>    "service.js":           ["Service"],
>    "status.js":            ["Status"],
>    "util.js":              ['Utils', 'Svc']
>  };

Yay for this shrinking!

::: services/sync/modules/service.js
@@ +311,5 @@
> +    // Status is instantiated before us and is the first to grab an instance of
> +    // the IdentityManager. We use that instance because IdentityManager really
> +    // needs to be a singleton. Ideally, the longer-lived object would spawn
> +    // this service instance.
> +    this.identity = Status._authManager;

Oh Christ.

Also, thank you for commenting this.

Let's go one step further. If the instantiation order changes, things will screw up, so let's check for falsyness here and do something sane.

::: services/sync/tests/unit/test_identity_manager.js
@@ +9,3 @@
>  function run_test() {
>    initTestLogging("Trace");
> +  Log4Moz.repository.getLogger("Sync.identity").level =

Over-enthusiastic sed…

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 659308 [details] [diff] [review]
Part 5: Remove CollectionKeys singleton, v1

Review of attachment 659308 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with points addressed.

::: services/sync/modules/engines.js
@@ +826,5 @@
>        handled.push(item.id);
>  
>        try {
>          try {
> +          item.decrypt(self.service.collectionKeys.keyForCollection(self.name));

This is ugly, and worse it relies on a Sufficiently Smart Compiler® to make it efficient.

Let's take this opportunity and cache the key outside of the recordHandler assignment:

    let key = this.service.collectionKeys.keyForCollection(this.name);
    newitems.recordHandler = function(item) {
      ..

@@ +1329,2 @@
>        canDecrypt = true;
> +    }.bind(this);

Similarly. There is no reason for us to be doing four attribute lookups and a method call within a loop.

::: services/sync/modules/service.js
@@ +275,5 @@
>  
>    handleFetchedKeys: function handleFetchedKeys(syncKey, cryptoKeys, skipReset) {
>      // Don't want to wipe if we're just starting up!
>      // This is largely relevant because we don't persist
> +    // collectionKeys yet: Bug 610913.

We just WONTFIXed that bug, and "collectionKeys" here doesn't make sense. Please rewrite this comment.

::: services/sync/tests/unit/test_corrupt_keys.js
@@ +64,1 @@
>                                           Svc.Crypto.generateRandomKey()];

Alignment.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 659316 [details] [diff] [review]
Part 6: Remove Weave export from service.js, v1

Review of attachment 659316 [details] [diff] [review]:
-----------------------------------------------------------------

Love it.

Comment 15

[Richard Newman [:rnewman]]

ZOMG all reviews done!!1!

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/37a2942f624d
https://hg.mozilla.org/services/services-central/rev/b57329caf8e8
https://hg.mozilla.org/services/services-central/rev/c6a07b5b3142
https://hg.mozilla.org/services/services-central/rev/6117227b889d
https://hg.mozilla.org/services/services-central/rev/5a9e2b0e6ba1
https://hg.mozilla.org/services/services-central/rev/c258bd2d95e4
https://hg.mozilla.org/services/services-central/rev/81a2fef74b0f

Comment 17

[Gregory Szorc [:gps]]

Attachment: Part 8: Fix TPS, v1

Landing this busted TPS. The underlying problem is part 6. TPS was relying on Weave being exported from service.js. I refactored TPS to go through main.js.

I hate adding additional Weave. accesses. This is only temporary. I have a patch in my queue that kills Weave.

Comment 18

[Gregory Szorc [:gps]]

Part 8: https://hg.mozilla.org/services/services-central/rev/1a7e69990b60

I just noticed mochitests are broken too. I guess we're getting a part 9!

Comment 19

[Gregory Szorc [:gps]]

Attachment: Part 9: Fix browser preferences, v1

This /should/ fix things. I haven't tested locally because I'm waiting on a build to finish. I'll hold off pushing until I can verify locally.

Comment 20

[Gregory Szorc [:gps]]

Part 9: https://hg.mozilla.org/services/services-central/rev/ec93b64f28bb

I snuck in a slight change from the r+'d patch. But, it was just updating a reference to point to Identity in the proper location.

Comment 21

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/37a2942f624d
https://hg.mozilla.org/mozilla-central/rev/b57329caf8e8
https://hg.mozilla.org/mozilla-central/rev/c6a07b5b3142
https://hg.mozilla.org/mozilla-central/rev/6117227b889d
https://hg.mozilla.org/mozilla-central/rev/5a9e2b0e6ba1
https://hg.mozilla.org/mozilla-central/rev/c258bd2d95e4
https://hg.mozilla.org/mozilla-central/rev/81a2fef74b0f
https://hg.mozilla.org/mozilla-central/rev/1a7e69990b60
https://hg.mozilla.org/mozilla-central/rev/ec93b64f28bb