Closed Bug 1266640 Opened 4 years ago Closed 4 years ago

Avoid using the persisted clusterURL (mainly by not persisting it!)

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 2 obsolete files)

There are some bugs we are investigating where the user is not ending up on the correct storageURL, even after restarting the browser. Sync expects the value to be stored in preferences, and when it needs the value it reads it from preferences. However, since the move to FxA the cluster URL is obtained from the tokenserver, and this is requested at each startup.

The problem with the current approach is that it is not immediately obvious that Sync is always asking the token server for the cluster URL and it isn't accidentally reusing the clusterURL from a previous run. This makes it difficult to say with certainly that the client is not at fault in the cases where the wrong cluster URL is being used after startup.

So we should just not store it there :) This is another nail in the coffin for the "legacy identity provider", but that's OK - it's already dead, just not yet buried.

The only non-obvious thing in this patch is what Weave.js uses this preference as an additional flag to indicate that Sync really is configured. However, this doesn't seem necessary and may even lead to false-negatives - when a user if first signing in to Sync they aren't going to yet have a cluster URL, but Sync is certainly configured at that point. So Weave.js just checks if the username is stored (and as the function in question states, "It does *not* perform a robust check to see if the client is working" - it's just a clue that it's probably safe to load Sync-proper and find out more.) Requesting feedback from rnewman on just this part - I'll find someone with more time for the real review (but obviously a real review would be welcome - the patch isn't very big)
Attachment #8744163 - Flags: feedback?(rnewman)
Flags: firefox-backlog+
Priority: -- → P2
So yeah, the client is doing the wrong thing here :( Although it fetches the token at startup, the value of the clusterURL isn't requested until Sync itself asks for it - which it only does if it doesn't already have one, or it sees the 401.

I'm working on some additional tests for this.
Priority: P2 → P1
Summary: Avoid persisting the clusterURL in preferences. → Avoid using the persisted clusterURL (mainly by not persisting it!)
Comment on attachment 8744163 [details] [diff] [review]
0001-Don-t-persist-clusterURL-to-preferences.patch

Clearing feedback request while I work on additional tests
Attachment #8744163 - Flags: feedback?(rnewman)
Here's a new patch with tests and with additional mitigation against accidentally using the wrong cluster URL - I thought I'd get feedback from Kit and Thom before I request review from rnewman, as I might come back to you guys for a review of rnewman can't find the time.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd79d5a0d4de
Attachment #8744163 - Attachment is obsolete: true
Attachment #8749555 - Flags: feedback?(tchiovoloni)
Attachment #8749555 - Flags: feedback?(kcambridge)
Comment on attachment 8749555 [details] [diff] [review]
0001-Bug-1266640-Avoid-persistng-the-clusterURL-and-other.patch

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

Approach looks good!

::: services/sync/modules/service.js
@@ +93,1 @@
>      this._updateCachedURLs();

Will this do the right thing if `clusterURL` is cleared out? I'm wondering if we should change `_updateCachedURLs` to clear the URLs, instead of short-circuiting if we don't have a cluster URL.

Or is this what you meant by "that screws up many existing tests that explicitly set storageURL as a shortcut to avoid a full login" in Weave.js? If so, please disregard my comment. :-)

::: services/sync/modules/stages/cluster.js
@@ +80,5 @@
>        return false;
>      }
>  
> +    // convert from the funky resource object to the actual value.
> +    cluster = cluster.toString();

Oh, I see, it returns a `String` object (https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/services/sync/modules/resource.js#318). That's devious.
Attachment #8749555 - Flags: feedback?(kcambridge) → feedback+
(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Will this do the right thing if `clusterURL` is cleared out? I'm wondering
> if we should change `_updateCachedURLs` to clear the URLs, instead of
> short-circuiting if we don't have a cluster URL.
> 
> Or is this what you meant by "that screws up many existing tests that
> explicitly set storageURL as a shortcut to avoid a full login" in Weave.js?

Exactly, yeah - although it was only a couple of tests, so I think I should put more effort into just fixing those tests - bigger patch but better outcome.

> Oh, I see, it returns a `String` object
> (https://dxr.mozilla.org/mozilla-central/rev/
> e5a10bc7dac4ee2453d8319165c1f6578203eac7/services/sync/modules/resource.
> js#318). That's devious.

I admit I spat the dummy after discovering the "why" of the obscure failures I was seeing and didn't follow through to the "where" - thanks! Maybe I should just fix that instead of the explicit .toString and "should be unnecessary but wasn't" type check?
Comment on attachment 8749555 [details] [diff] [review]
0001-Bug-1266640-Avoid-persistng-the-clusterURL-and-other.patch

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

Looks great to me.
Attachment #8749555 - Flags: feedback?(tchiovoloni) → feedback+
For what it's worth, I'd second changing resource to not use String objects, but it's not like it's the only place in Sync where they're used (IMO they're generally a mistake, though).
(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Will this do the right thing if `clusterURL` is cleared out? I'm wondering
> if we should change `_updateCachedURLs` to clear the URLs, instead of
> short-circuiting if we don't have a cluster URL.

This version of the patch does that - the test issues I mentioned were actually a real problem in service.js - if sync ends up completing without a cluster URL we still attempted to use the metaURL to update the declinedEngines state - previously that would have tried to use the cached (and thus invalid) metaURL. The fix for that was to only attempt to update declined engines if we have a metaURL (and this patch ensures we do not have a metaURL when we don't have a cluster URL.

(In reply to Thom Chiovoloni [:tcsc] from comment #7)
> For what it's worth, I'd second changing resource to not use String objects,
> but it's not like it's the only place in Sync where they're used (IMO
> they're generally a mistake, though).

Yeah, that code is trying to be too clever by half:

>     let ret     = new String(data);
>     ret.url     = channel.URI.spec;
>     ret.status  = status;

So it's not as simple as using a normal string - I left that alone but updated the comment in cluster.js.

Richard, if you don't think you have time to look at this any time soon, could you please redirect the review request to Kit?

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=870771a9439d
Attachment #8749555 - Attachment is obsolete: true
Attachment #8750150 - Flags: review?(rnewman)
I hope to get to this today. (Sorry for the Tofino-related delays!)
Status: NEW → ASSIGNED
Comment on attachment 8750150 [details] [diff] [review]
0001-Bug-1266640-Avoid-persisting-the-clusterURL-and-forg.patch

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

::: services/sync/modules/browserid_identity.js
@@ +394,5 @@
>      this.resetSyncKey();
>      this._token = null;
> +    // The cluster URL comes from the token, so resetting it to empty will
> +    // force Sync to not accidentally use a value from an earlier token.
> +    Weave.Service.clusterURL = "";

Why the empty string here but `null` elsewhere?

::: services/sync/modules/service.js
@@ +62,5 @@
>    infoURL: null,
>    storageURL: null,
>    metaURL: null,
>    cryptoKeyURL: null,
> +  _clusterURL: null,

Add a comment about where this value comes from.

@@ +76,5 @@
>      // Only do work if it's actually changing
>      if (value == this.serverURL)
>        return;
>  
>      // A new server most likely uses a different cluster, so clear that

Move to the line below, with a newline above.

::: services/sync/modules/stages/cluster.js
@@ +79,5 @@
>      if (cluster == null) {
>        return false;
>      }
>  
> +    // convert from the funky "String object with additional properties" that

Nit: "Convert"

@@ -86,5 @@
>      }
>  
>      this._log.debug("Setting cluster to " + cluster);
>      this.service.clusterURL = cluster;
> -    Svc.Prefs.set("lastClusterUpdate", Date.now().toString());

Can we clean up prefs somehow?

::: services/sync/tests/unit/test_fxa_node_reassignment.js
@@ +194,5 @@
> +
> +  do_check_false(Service.isLoggedIn, "not already logged in");
> +  Service.sync();
> +  do_check_eq(Status.sync, SYNC_SUCCEEDED, "sync succeeded");
> +  do_check_eq(numTokenFetches, 0, "didn't fetch a new token");

Check the token here, too.
Attachment #8750150 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8750150 [details] [diff] [review]
> 0001-Bug-1266640-Avoid-persisting-the-clusterURL-and-forg.patch

Thanks! I addressed all comments except:

> @@ -86,5 @@
> >      }
> >  
> >      this._log.debug("Setting cluster to " + cluster);
> >      this.service.clusterURL = cluster;
> > -    Svc.Prefs.set("lastClusterUpdate", Date.now().toString());
> 
> Can we clean up prefs somehow?

I didn't think it worthwhile to add code here to delete the possibly existing but now-unused preference "lastClusterUpdate" - is that what you meant, and if so, do you feel strongly enough that I should open a followup to do this?
https://hg.mozilla.org/mozilla-central/rev/0c29ad917ac3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.