Clean up FxAccounts' public/internal API implementation

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Sync
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
The current public and internal API implementation of FxAccounts.jsm has become a little messy. We have two different prototypes, the public one calls a global called "internal" to access the internal one. This all makes it a little hard to follow the flow and I think a good first step to cleaning up FxA would be to address this.
(Assignee)

Comment 1

4 years ago
Created attachment 8369567 [details] [diff] [review]
0001-Bug-967120-Clean-up-FxAccounts-public-internal-API-i.patch

This patch doesn't change how the code works. All it does is clean up as described and move some code around.
Attachment #8369567 - Flags: review?(mhammond)
Comment on attachment 8369567 [details] [diff] [review]
0001-Bug-967120-Clean-up-FxAccounts-public-internal-API-i.patch

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

Looks fine to me (although I'm not sure it's a priority)

::: services/fxaccounts/FxAccounts.jsm
@@ +94,5 @@
>    this.generationCount = 0;
>  
>    this.fxAccountsClient = new FxAccountsClient();
>  
> +  // Normal (i.e., non-testing) initialization.

I think this line can be removed now (it was really just indicating that signedInUserStorage will only be set if the mock didn't provide it, but that's now handled differently, so I don't think this line adds value)

@@ +178,5 @@
> +      return data;
> +    });
> +  },
> +
> +  // set() makes sure that polling is happening, if necessary.

this comment is somewhat messy, and ideally should just be incorporated into the documentation for the getSignedInUser and setSignedInUser comments - but without that, it's probably better moved above both functions (like it was before this refactor)

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +112,5 @@
>        return this._now_is;
>      },
> +    getCertificateSigned: function (sessionToken, serializedPublicKey) {
> +      _("mock getCertificateSigned\n");
> +      _(Object.keys(this) + "\n");

this looks like left-over debugging?
Attachment #8369567 - Flags: review?(mhammond) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Mark Hammond [:markh] from comment #2)
> > +    getCertificateSigned: function (sessionToken, serializedPublicKey) {
> > +      _("mock getCertificateSigned\n");
> > +      _(Object.keys(this) + "\n");
> 
> this looks like left-over debugging?

Yeah... thanks for catching!
(Assignee)

Comment 4

4 years ago
(In reply to Mark Hammond [:markh] from comment #2)
> Looks fine to me (although I'm not sure it's a priority)

Not the same priority as a bug fixing the FxA workflow but I do think it's important to clean up what we wrote in the last weeks here and there as it does really help to maintain the code. I don't believe that anyone is totally confident discussing the call graph in FxAccounts.jsm currently and removing indirection helps a lot with that.

Should I request uplifting to Aurora before landing the patch? It would certainly make uplifting changes to this file harder if it's not on Aurora. How would we handle this for future "clean ups"? I would really like to further improve maintainability/stability even if that's of course not a top priority...

Gavin, any opinion about this?
Flags: needinfo?(gavin.sharp)
I think we should land this now and uplift it.
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 6

4 years ago
Ok, landed:

https://hg.mozilla.org/integration/fx-team/rev/2865fd5e5458
Backed out in https://hg.mozilla.org/integration/fx-team/rev/6fa2dd192173 for xpcshell (https://tbpl.mozilla.org/php/getParsedLog.php?id=34041085&tree=Fx-Team, looks like all platforms) and browser-chrome (https://tbpl.mozilla.org/php/getParsedLog.php?id=34041180&tree=Fx-Team, might be opt-only) bustage.
(Assignee)

Comment 8

4 years ago
One thing I found out is that "internal" is a global variable in FxAccounts.jsm. When creating multiple FxAcounts instances that means that all of these share the latest internal object that was assigned to "internal". This is quite bad and one of the reasons I was rewriting this module but the tests seem to somehow expect this behavior implicitly and I need to find out how to fix that.
(Assignee)

Comment 9

4 years ago
Created attachment 8372035 [details] [diff] [review]
0002-Bug-967120-Follow-up-to-fix-broken-xpcshell-and-moch.patch

All good locally, will push to try. All these fixes are necessary because those tests just worked by accident as all of the FxAccounts instances shared the latest internal object.
Attachment #8372035 - Flags: review?(mhammond)
(Assignee)

Comment 10

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d669b5dad934
Comment on attachment 8372035 [details] [diff] [review]
0002-Bug-967120-Follow-up-to-fix-broken-xpcshell-and-moch.patch

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

discussed briefly on IRC - my version of the same patch adds a comment at the top of the ctor:

// NOTE: _fxaService and _tokenServerClient are replaced with mocks by
// the test suite.

which seems worthwhile.

Also, there's one use in the cluster manager that should change too, like:

@@ -551,18 +554,19 @@ this.BrowserIDManager.prototype = {
 function BrowserIDClusterManager(service) {
   ClusterManager.call(this, service);
 }
 
 BrowserIDClusterManager.prototype = {
   __proto__: ClusterManager.prototype,
 
   _findCluster: function() {
+    let fxa = this.identity._fxaService; // will be mocked for tests.
     let promiseClusterURL = function() {
-      return fxAccounts.getSignedInUser().then(userData => {
+      return fxa.getSignedInUser().then(userData => {

::: services/sync/modules/browserid_identity.js
@@ +410,5 @@
>    _fetchTokenForUser: function(userData) {
>      let tokenServerURI = Svc.Prefs.get("tokenServerURI");
>      let log = this._log;
>      let client = this._tokenServerClient;
> +    let fxa = this._fxaService;

there's one more existing use of this._fxaService in this function that might as well change to use this new local
Attachment #8372035 - Flags: review?(mhammond) → review+
(Assignee)

Comment 12

4 years ago
Thanks! Will incorporate all suggested changes.
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/b36de5eeda6f
https://hg.mozilla.org/integration/fx-team/rev/8b84620e4901
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b36de5eeda6f
https://hg.mozilla.org/mozilla-central/rev/8b84620e4901
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
(Assignee)

Comment 15

4 years ago
Comment on attachment 8369567 [details] [diff] [review]
0001-Bug-967120-Clean-up-FxAccounts-public-internal-API-i.patch

[Approval Request Comment]
Risk to taking this patch (and alternatives if risky): Low risk, only cleanup.
String or IDL/UUID changes made by this patch: None.

We should take this on Aurora because it contains a few important fixes and would make writing patches harder otherwise.
Attachment #8369567 - Flags: approval-mozilla-aurora?
Attachment #8369567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5280e3c4b237
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ca38361b57a
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.