968929 - Update FXAccountsClient to production server URI

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect)

Description

[Sam Penrose]

AKA api.accounts.firefox.com/v1

Comment 1

[:Gavin Sharp [email: [e-mail]]]

identity.fxaccounts.auth.uri sets the auth URI to use for various products. Firefox's URI was updated in bug 960332.

Is this bug about updating Firefox OS's value (currently "https://api-accounts.dev.lcip.org/v1") or about changing the default in the case where the pref isn't set in (http://hg.mozilla.org/mozilla-central/annotate/d812f80a0f1d/services/fxaccounts/FxAccountsClient.jsm#l18)?

Comment 2

[Sam Penrose]

This bug is about changing the default. It sounds like we should just read the same pref.

Comment 3

[Sam Penrose]

Attachment: 968929-use-prod-uri.patch

Per Gavin's comment, just use the pref that the Sync effort settled on

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8374543 [details] [diff] [review]
968929-use-prod-uri.patch

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

::: services/fxaccounts/FxAccountsClient.jsm
@@ +14,5 @@
>  Cu.import("resource://services-crypto/utils.js");
>  Cu.import("resource://gre/modules/FxAccountsCommon.js");
>  Cu.import("resource://gre/modules/Credentials.jsm");
>  
> +let _host = Services.prefs.getCharPref("identity.fxaccounts.auth.uri");

I think you want to keep the try/catch as it was, and make the new server url the default value of _host.

The reason for this is that, in the event that a the identity.fxaccounts.auth.uri preference does not exists (either it's not in the build, or the user somehow deleted it), the getCharPref() call with throw an exception.

As an experiment, go to Tools -> Web Developer -> Scratchpad.  Then from the main menu, set Environment to Browser, and then run these two lines:

  Components.utils.import("resource://gre/modules/Services.jsm");
  Services.prefs.getCharPref('i.like.pie');

It will throw an exception, which we do not want to have happen in FxAccountsClient.jsm.

So you either need to do a try/catch as here, or check for the existence of the pref explicitly as in https://mxr.mozilla.org/mozilla-central/source/dom/identity/nsDOMIdentity.js#618; if there's a problem, fall back to the default.

So basically I think all you need to do is change the initial value of _host (line 19) in the initial code to the new server url.

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

If you can't find the Scratchpad, you will need to pref devtools.chrome.enabled to true.

Comment 6

[Sam Penrose]

Attachment: 968929-use-prod-uri.patch

OK, got it. I was confused by the patch that Gavin pointed at -- I thought it meant that the pref in question would always be set. Feels a bit weird that the same crucial magic string is written in two places, but if that it the way it is done, so be it.

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Sam Penrose from comment #6)
> Created attachment 8374572 [details] [diff] [review]
> 968929-use-prod-uri.patch
> 
> OK, got it. I was confused by the patch that Gavin pointed at -- I thought
> it meant that the pref in question would always be set. Feels a bit weird
> that the same crucial magic string is written in two places, but if that it
> the way it is done, so be it.

I agree, it's funky to have the same magic string in two places.  But I still think we need something to fall back on in the case of a broken profile.

Anyway, I think this should do it!

Thanks for the fix!
j

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8374572 [details] [diff] [review]
968929-use-prod-uri.patch

Ship it!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2f0ea43e8f31

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f0ea43e8f31

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #7)
> I agree, it's funky to have the same magic string in two places.  But I
> still think we need something to fall back on in the case of a broken
> profile.

I don't think we need to support the "user manually removes default pref" case, since that can't happen in any reasonable situation (we already rely on default prefs always being there in many other places).

That would suggest just removing the default fallback, and moving the other default from firefox.js/b2g.js into all.js.