Closed Bug 968929 Opened 6 years ago Closed 6 years ago

Update FXAccountsClient to production server URI

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: spenrose, Assigned: spenrose)

References

Details

Attachments

(1 file, 1 obsolete file)

AKA api.accounts.firefox.com/v1
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
This bug is about changing the default. It sounds like we should just read the same pref.
Attached patch 968929-use-prod-uri.patch (obsolete) — Splinter Review
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.
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f0ea43e8f31
Status: NEW → RESOLVED
Closed: 6 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.
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.