Closed Bug 965896 Opened 6 years ago Closed 6 years ago

allow legacy sync reset to expose FxA accounts

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: Gavin, Assigned: markh)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 2 obsolete files)

rnewman: this is an update to the "part 2" patch from bug 959222 which you previously gave feedback on.
Attachment #8369003 - Flags: review?(rnewman)
Whiteboard: [qa+]
Comment on attachment 8369003 [details] [diff] [review]
0001-Bug-965896-allow-a-legacy-sync-reset-to-re-enable-fx.patch

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

I thought this looked familiar :)

::: services/sync/modules/service.js
@@ +888,5 @@
>      Svc.Prefs.set("lastversion", WEAVE_VERSION);
>  
>      this.identity.deleteSyncCredentials();
>  
> +    // Reset the identity manager, then re-initialize it so the Fxa manager is

Nit: s/Fxa/FxA

@@ +893,5 @@
> +    // used.
> +    this.identity.username = "";
> +    Services.prefs.clearUserPref("services.sync.fxaccounts.enabled");
> +    this.status.__authManager = null;
> +    this.identity = Status._authManager;

Can this getter fail?
Attachment #8369003 - Flags: review?(rnewman) → review+
(In reply to Richard Newman [:rnewman] from comment #2)
Thanks!

> Nit: s/Fxa/FxA
Fixed.

> Can this getter fail?

In theory, no.  There are no error paths in browserid_identity, and the sync init code where the identity manager is first initialized also makes no attempt to catch errors, so I left this alone.

https://hg.mozilla.org/integration/fx-team/rev/e78a7c6d9601
(In reply to Phil Ringnalda (:philor) from comment #4)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/cff85c90a8a3
> for https://tbpl.mozilla.org/php/getParsedLog.php?id=34109165&tree=Fx-Team

Yeah, for (retrospectively) obvious reasons :(

rnewman: there are 44 explicit calls to Service.startOver() in the sync unit tests, and they all expect the old identity manager to exist after it completes.  Some do this just to cleanup (but following tests still expect the old identity), while others explicitly test certain things after it is called.  It could *almost* be argued this latter class of tests are no longer valid (but even if I could make a case for that, it still leaves the former class).

I've no good ideas here.  Among the not-good ideas are:

* A special pref just for testing that tells startOver to not reset the identity.

* Change every relevant add_test call to use a new helper like "add_identity_test" - something like "add_legacy_identity_test".  This could have an observer that notes the startover-finish observer and resets the identity back to a legacy one.  This might not even work in practice, as part of startOver is now async

* Add a "global" helper function - something like "startOverWithLegacyIdentity()" - that does the same observer trick (but this can probably spin until that async notification fires)

Do you have any better ideas, or failing that, which of the above do you consider the least-worst?
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
(In reply to Mark Hammond [:markh] from comment #5)

In short: probably a combination of all of these things :(


> * A special pref just for testing that tells startOver to not reset the
> identity.

I'd generalize this a little bit: make the behavior of startOver configurable (indeed, pref-driven), such that in one state a reset will switch over to the FxA identity system, and in the other it'll follow the old code path (modulo potential asyncness). Configure appropriately before each suite of tests runs. That gets you the old tests passing, perhaps with some mechanical async adjustments.

That doesn't solve the whole problem, of course -- you'll still have tests poking at the wrong things in FxA-mode. But I think it's one of the least-hacky starting points, given that we're warping a test suite to incorporate flexibility it was never designed to have.

For the tests that are explicitly testing or relying on the behavior of startOver -- well, those have to change, because you're changing the behavior of the thing under test. For those tests, yeah, start to partition them between old-only and new-only, so you end up with three sets of tests:

* old only
* run for old, then run for new
* new only

Then you'll need to make sure there's coverage for the new behavior.
Flags: needinfo?(rnewman)
This patch has a number of differences from the last one:

* browserid_identity.js used the module global |fxAccounts| instead of |this._fxaService| which causes a problem for testing - this._fxaService is replaced with a mocked version by the tests, where the global remains the real, unmocked version.  I added comments where this._fxaService is initially setup to try and avoid this happening in the future.

* startOver() now looks for a pref "services.sync-testing.startOverKeepIdentity" to see if it should clobber the identity or not.  This isn't in the "services.sync" branch as resetPrefs() is regularly called for that branch, which defeats the test suite's attempt at keeping it set to true.  This pref is set in head_helpers.js - we can work out how to better generalize some of this when we have more time to spend on tests.

* A new test specifically for startOver - it checks various things are reset and the FxA identity manager is created.

Enough changes here that a re-review is warranted :(
Attachment #8369003 - Attachment is obsolete: true
Attachment #8371973 - Flags: review?(rnewman)
Comment on attachment 8371973 [details] [diff] [review]
0002-Bug-965896-allow-a-sync-startOver-to-re-enable-Firef.patch

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

Looks sane to me.

::: services/sync/modules/service.js
@@ +892,5 @@
> +    // If necessary, reset the identity manager, then re-initialize it so the
> +    // FxA manager is used.  This is configurable via a pref - mainly for tests.
> +    let keepIdentity = false;
> +    try {
> +      keepIdentity = Services.prefs.getBoolPref("services.sync-testing.startOverKeepIdentity");

let keepIdentity = Svc.Prefs.get("services.sync-testing.startOverKeepIdentity", false);

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +27,5 @@
> +
> +  // remember some stuff so we can reset it after.
> +  let oldIdentidy = Service.identity;
> +  let oldClusterManager = Service._clusterManager;
> +  let defered = Promise.defer();

deferred
Attachment #8371973 - Flags: review?(rnewman) → review+
Comment on attachment 8371973 [details] [diff] [review]
0002-Bug-965896-allow-a-sync-startOver-to-re-enable-Firef.patch

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

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +10,5 @@
> +  initTestLogging("Trace");
> +  run_next_test();
> +}
> +
> +add_task(function test_startover() {

Nit: I've been told to use function* when declaring generator functions.
I just spotted something missing here.

Weave.fxAccountsEnabled needs to be flipped when this reset happens, too, else modules that dynamically check that -- such as FHR -- will be incorrect. (Encountered when reviewing Bug 958561 -- please coordinate with gps.)
(In reply to Richard Newman [:rnewman] from comment #10)
> I just spotted something missing here.
> 
> Weave.fxAccountsEnabled needs to be flipped when this reset happens, too,
> else modules that dynamically check that -- such as FHR -- will be
> incorrect.

Huh.  This version of the patch doesn't have the change to Weave.js specifically to handle this.  I wonder where it went :)  But thanks for noticing this, and I'll also add a check t the new test for this.
Thanks - fixed all review comments and reinstated the code (and added a test) to ensure .fxAccountsEnabled toggles correctly.

https://hg.mozilla.org/integration/fx-team/rev/d130046321ce
(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8371973 [details] [diff] [review]
> ::: services/sync/modules/service.js
> @@ +892,5 @@
> > +    // If necessary, reset the identity manager, then re-initialize it so the
> > +    // FxA manager is used.  This is configurable via a pref - mainly for tests.
> > +    let keepIdentity = false;
> > +    try {
> > +      keepIdentity = Services.prefs.getBoolPref("services.sync-testing.startOverKeepIdentity");
> 
> let keepIdentity =
> Svc.Prefs.get("services.sync-testing.startOverKeepIdentity", false);

Doh - I thought this wouldn't work due to Svc.Prefs using a different root branch, but assumed there might be magic I didn't know about so tried it anyway - and thought I'd re-run the tests.

Turns out I was correct first time, and wrong the second time :/

Re-landed with the original block left intact (ie, using Services.prefs directly)

https://hg.mozilla.org/integration/fx-team/rev/3e8524fcafcd
https://hg.mozilla.org/mozilla-central/rev/3e8524fcafcd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attached patch As landedSplinter Review
Patch as landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Sync.Next
User impact if declined: Existing users of Sync will be unable to reset and use Firefox Accounts
Testing completed (on m-c, etc.): Landed on m-c, has test coverage.
Risk to taking this patch (and alternatives if risky): Risk limited to sync
String or IDL/UUID changes made by this patch: None
Attachment #8371973 - Attachment is obsolete: true
Attachment #8375137 - Flags: review+
Attachment #8375137 - Flags: approval-mozilla-aurora?
Attachment #8375137 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Depends on: 1058703
You need to log in before you can comment on or make changes to this bug.