Update FXAccountsClient to production server URI

RESOLVED FIXED in mozilla30

Status

()

Core
FxAccounts
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Sam Penrose, Assigned: Sam Penrose)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
AKA api.accounts.firefox.com/v1
(Assignee)

Updated

4 years ago
Blocks: 955951
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)?
Component: Identity → FxA
(Assignee)

Comment 2

4 years ago
This bug is about changing the default. It sounds like we should just read the same pref.
(Assignee)

Comment 3

4 years ago
Created attachment 8374543 [details] [diff] [review]
968929-use-prod-uri.patch

Per Gavin's comment, just use the pref that the Sync effort settled on
Attachment #8374543 - Flags: review?(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.
Attachment #8374543 - Flags: review?(jparsons)
If you can't find the Scratchpad, you will need to pref devtools.chrome.enabled to true.
(Assignee)

Comment 6

4 years ago
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.
Attachment #8374543 - Attachment is obsolete: true
Attachment #8374572 - Flags: feedback?(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 on attachment 8374572 [details] [diff] [review]
968929-use-prod-uri.patch

Ship it!
Attachment #8374572 - Flags: feedback?(jparsons) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2f0ea43e8f31
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2f0ea43e8f31
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla30
(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.
You need to log in before you can comment on or make changes to this bug.