Closed Bug 754538 Opened 10 years ago Closed 9 years ago

Implement AITC Client

Categories

(Web Apps Graveyard :: AppsInTheCloud, defect)

defect
Not set
normal

Tracking

(blocking-kilimanjaro:+)

VERIFIED FIXED
blocking-kilimanjaro +

People

(Reporter: anant, Assigned: anant)

References

Details

(Whiteboard: [qa!])

Attachments

(2 files, 5 obsolete files)

This includes only the client portion of the patch from bug 744257. It also includes build system changes necessary to make the "aitc" directory in /services.
Attached patch AITC Client RC1 (obsolete) — Splinter Review
Cleaned up nits from previous bug.
Attachment #623393 - Flags: review?(gps)
Attached patch AITC Storage RC1 (obsolete) — Splinter Review
The storage module is also fairly standalone and can be reviewed independently. The patch includes tests, yay!
Attachment #623473 - Flags: review?(gps)
Comment on attachment 623393 [details] [diff] [review]
AITC Client RC1

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

There are some obvious problems with this patch, such as ReferenceErrors from out-of-scope or improperly referenced variables.

This leads me to my next point: these issues would be uncovered by unit tests. You need to write tests.

r- for lack of tests and obvious bugs.

Style and nit wise, the code is *a lot* easier to read and understand than it started at. Aside from the obvious bugs, this is almost ready to land! Keep up the progress.

::: services/aitc/AitcComponents.manifest
@@ +1,3 @@
> +# service.js
> +component {a3d387ca-fd26-44ca-93be-adb5fda5a78d} service.js
> +contract @mozilla.org/services/aitc;1 {a3d387ca-fd26-44ca-93be-adb5fda5a78d}

Gavin: is this the appropriate location for services in the services/ tree? Sync today uses @mozilla.org/weave/service;1. I'm not sure if "services" is too ambiguous.

::: services/aitc/modules/client.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const EXPORTED_SYMBOLS = ['AitcClient'];

Nit: Double quotes for all strings.

@@ +14,5 @@
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +
> +/**
> + * First argument is the full URL to the token server endpoint.

No it isn't.

@@ +16,5 @@
> +
> +/**
> + * First argument is the full URL to the token server endpoint.
> + * Second argument is an object of type "Preferences" as created by
> + * preferences.js in services-common.

I would first document the interface this object needs to provide {get,put}. You can state that a Preferences instance is typically used. But, it could easily be a stubbed type if needed.

I'm tempted to request that you just change this to a simple object. If that object needs to intercept gets or sets, it could be a Proxy (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Proxy). However, I've heard that interface is changing, so maybe it's best to stay away.

@@ +31,5 @@
> +  this._state = state;
> +  this._backoff = false;
> +  if (state.get("backoff", 0)) {
> +    this._backoff = true;
> +  }

this._backoff = !!state.get("backoff", false);

@@ +33,5 @@
> +  if (state.get("backoff", 0)) {
> +    this._backoff = true;
> +  }
> +
> +  this._appsLastModified = parseInt(state.get("lastModified", "0"));

Always specify the radix argument to parseInt: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/parseInt

parseInt(state.get("lastModified", "0"), 10);

@@ +86,5 @@
> +      throw new Error("getApps called but no callback provided");
> +    }
> +
> +    if (!this._checkBackoff()) {
> +      cb(null, null);

You should probably document this behavior in the API so clients know to check for it. Otherwise, I could see somebody writing code that blindly trusted that the value is non-null if there is no error.

Edit: I see now that _processGetApps can also cb(null, null) if the response was a 304. Does the caller ever care about distinguishing between a request that never happened vs one that completed successfully? I think they might (e.g. keeping track of the last good request and alerting if the elapsed time is greater than a threshold). If this isn't a requirement, then consider ignoring my advice.

@@ +93,5 @@
> +
> +    let uri = this.uri + "/apps/?full=1";
> +    let req = new TokenAuthenticatedRESTRequest(uri, this.token);
> +    if (this._appsLastModified) {
> +      req.setHeader("x-if-modified-since", this._appsLastModified);

Please capitalize header names. Yes, I know they are case-insensitive and this is technically right. However, it breaks convention.

@@ +136,5 @@
> +      this._log.info("getApps succeeded and got " + apps.length);
> +    } catch (e) {
> +      let err = new Error("Exception in getApps " + e);
> +      this._log.error(err);
> +      cb(err, null);

return?

@@ +141,5 @@
> +    }
> +    
> +    // Return success.
> +    try {
> +      cb(null, apps);

Isn't apps locally scoped to the try block?

@@ +143,5 @@
> +    // Return success.
> +    try {
> +      cb(null, apps);
> +      // Don't update lastModified until we know cb succeeded.
> +      this._appsLastModified = parseInt(req.response.headers['x-timestamp']);

radix

@@ +144,5 @@
> +    try {
> +      cb(null, apps);
> +      // Don't update lastModified until we know cb succeeded.
> +      this._appsLastModified = parseInt(req.response.headers['x-timestamp']);
> +      state.put("lastModified", ""  + this._appsLastModified);

this._state

@@ +157,5 @@
> +   * don't store them on the server.
> +   */
> +  _makeRemoteApp: function _makeRemoteApp(app) {
> +    for each (let key in this.requiredLocalKeys) {
> +      if (!app[key]) {

if (!(key in app)) ? Otherwise you could alert on falsy values.

@@ +183,5 @@
> +   * registry expects. (Inverse of _makeRemoteApp)
> +   */
> +  _makeLocalApp: function _makeLocalApp(app) {
> +    for each (let key in this._requiredRemoteKeys) {
> +      if (!app[key]) {

if (!(key in app)) ?

@@ +214,5 @@
> +      // better to keep server load low, even if it means user's apps won't
> +      // reach their other devices during the early days of AITC. We should
> +      // revisit this when we have a better of idea of server load curves.
> +      err = new Error("X-Backoff in effect, aborting PUT");
> +      err.removeFromQueue = false;

There is no concept of a queue in this file. Can the name be something more generic?

@@ +221,5 @@
> +    }
> +
> +    let uri = this._makeAppURI(app.origin);
> +    let req = new TokenAuthenticatedRESTRequest(uri, this.token);
> +    if (app.modified) {

Don't you mean app.modifiedAt?

@@ +242,5 @@
> +      cb(error, null);
> +      return;
> +    }
> +
> +    this._setBackoff(req);

req isn't defined.

@@ +281,5 @@
> +  },
> +
> +  _makeAppURI: function _makeAppURI(origin) {
> +    let part = CommonUtils.encodeBase64URL(
> +      CryptoUtils._sha1(origin)

We now have CryptoUtils.UTF8AndSHA1() for you to use!

@@ +287,5 @@
> +    return this.uri + "/apps/" + part;
> +  },
> +
> +  // Before making a request, check if we are allowed to.
> +  _checkBackoff: function _checkBackoff() {

How about "_isRequestAllowed"? I think this makes the code easier to read.

@@ +293,5 @@
> +      return true;
> +    }
> +
> +    let time = Date.now();
> +    let backoff = parseInt(state.get("backoff", 0));

Where does "state" come from? I'm pretty sure this is throwing because of an unknown variable. You mean this._state?

Specify radix to parseInt.

@@ +311,5 @@
> +    let backoff = 0;
> +    let time = Date.now();
> +
> +    if (req.response.headers["x-backoff"]) {
> +      backoff = req.response.headers["x-backoff"];

You assigned an integer above but header values are strings. Do you want to work in a parseInt?

@@ +320,5 @@
> +      this._log.warn("Retry-Header header was seen: " + backoff);
> +    }
> +    if (backoff) {
> +      this._backoff = true;
> +      state.set("backoff", "" + (time + (backoff * 1000)));

this._state

::: services/aitc/service.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://services-common/log4moz.js");
> +Cu.import("resource://services-common/preferences.js");
> +Cu.import("resource://services-common/utils.js");

In this file, you may want to eliminate the services-common imports or at least delay loading them until the observers below.

In the case where AITC isn't active (although this may not be often so we could ignore it), you would be wasting memory by importing these only to not use them.

Also, by loading them up here, they have to be parsed at application start time AFAIK. If you defer loading to later (presumably after first paint), you put less burden on application start-up.

Unfortunately, I have no numbers to back up my claims. I'm just deferring to practices I see elsewhere.

Gavin: what's your opinion?

@@ +24,5 @@
> +  observe: function observe(subject, topic, data) {
> +    switch (topic) {
> +      case "app-startup":
> +        let os = Cc["@mozilla.org/observer-service;1"].
> +                 getService(Ci.nsIObserverService);

Nit on style:

Cc["@mozilla.org/observer-service;1"]
  .getService(Ci.nsIObserverService);

I may have told you the wrong style in a previous review. If so, I apologize. I get this messed up all the time.
Attachment #623393 - Flags: review?(gps) → review-
Attached patch AITC Client RC2 (obsolete) — Splinter Review
Updated patch with fixes, I'm going to wait for the tests from bug #750948 before asking for another review.

(In reply to Gregory Szorc [:gps] from comment #3)
> @@ +86,5 @@
> > +      throw new Error("getApps called but no callback provided");
> > +    }
> > +
> > +    if (!this._checkBackoff()) {
> > +      cb(null, null);
> 
> You should probably document this behavior in the API so clients know to
> check for it. Otherwise, I could see somebody writing code that blindly
> trusted that the value is non-null if there is no error.
> 
> Edit: I see now that _processGetApps can also cb(null, null) if the response
> was a 304. Does the caller ever care about distinguishing between a request
> that never happened vs one that completed successfully? I think they might
> (e.g. keeping track of the last good request and alerting if the elapsed
> time is greater than a threshold). If this isn't a requirement, then
> consider ignoring my advice.

I think it's better for client.js to make that determination as we need to distinguish between backoffs and real failures. At a later point we can have the client emit an event that the manager will listen for and convert to a user-facing message.

> > +    let uri = this.uri + "/apps/?full=1";
> > +    let req = new TokenAuthenticatedRESTRequest(uri, this.token);
> > +    if (this._appsLastModified) {
> > +      req.setHeader("x-if-modified-since", this._appsLastModified);
> 
> Please capitalize header names. Yes, I know they are case-insensitive and
> this is technically right. However, it breaks convention.

They were capital before (I prefer it that way too), and then I saw a few files in services/common/ with lowercase headers and assumed that was the preferred style.


> ::: services/aitc/service.js
> @@ +9,5 @@
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/FileUtils.jsm");
> > +Cu.import("resource://services-common/log4moz.js");
> > +Cu.import("resource://services-common/preferences.js");
> > +Cu.import("resource://services-common/utils.js");
> 
> In this file, you may want to eliminate the services-common imports or at
> least delay loading them until the observers below.
> 
> In the case where AITC isn't active (although this may not be often so we
> could ignore it), you would be wasting memory by importing these only to not
> use them.
> 
> Also, by loading them up here, they have to be parsed at application start
> time AFAIK. If you defer loading to later (presumably after first paint),
> you put less burden on application start-up.
> 
> Unfortunately, I have no numbers to back up my claims. I'm just deferring to
> practices I see elsewhere.

Better to be safe than sorry, there's no reason they *have* to be at the top, so I moved them.

Nice scoping catches, by the way!
Attachment #623393 - Attachment is obsolete: true
Nominating for k9o - this relates to AITC client for both desktop and mobile
blocking-kilimanjaro: --- → ?
Anant: can you please split storage into its own bug? Otherwise, the reviews will get intermingled.
Comment on attachment 623473 [details] [diff] [review]
AITC Storage RC1

I can't find a way to make a patch obsolete without uploading a new one - the storage bits are now in bug 755375.
Attachment #623473 - Flags: review?(gps)
blocking-kilimanjaro: ? → +
Attachment #623473 - Attachment is obsolete: true
client.js _makeRemoteApp and _makeLocalApp both strip the mandatory `name` field. Makes test_mock_server.js@test_install_app fail.
New version with a few fixes.
Attachment #623924 - Attachment is obsolete: true
Attachment #628524 - Flags: review?(mconnor)
Attachment #628524 - Flags: review?(gps)
Comment on attachment 628524 [details] [diff] [review]
AITC Client RC2.1

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

Looks good enough for r+. Still a few nits.

From bug comments, it sounds like you have more tests for this. If you do, please include them in the next patch.

::: services/aitc/modules/client.js
@@ +79,5 @@
> +    this._putApp(record, cb);
> +  },
> +
> +  /**
> +   * Fetch remote apps from server with GET.

Please document what cb can receive.

@@ +96,5 @@
> +    let req = new TokenAuthenticatedRESTRequest(uri, this.token);
> +    if (this._appsLastModified) {
> +      req.setHeader("X-If-Modified-Since", this._appsLastModified);
> +    }
> +

Please set an explicit request timeout. It's probably best to get that from a preference so we can push a hotfix to adjust the timeout, if needed.

@@ +98,5 @@
> +      req.setHeader("X-If-Modified-Since", this._appsLastModified);
> +    }
> +
> +    let self = this;
> +    req.get(function(err) {

Name function.

@@ +128,5 @@
> +      cb(new Error("Unexpected error with getApps"), null);
> +      return;
> +    }
> +
> +    let apps = [];

Nit: You don't need to assign a value since the value is never used.

@@ +137,5 @@
> +      apps = tmp.map(this._makeLocalApp, this);
> +      this._log.info("getApps succeeded and got " + apps.length);
> +    } catch (e) {
> +      let msg = new Error("Exception in getApps " + e);
> +      this._log.error(msg);

You may want to log CommonUtils.exceptionStr() instead.

@@ +226,5 @@
> +    let uri = this._makeAppURI(app.origin);
> +    let req = new TokenAuthenticatedRESTRequest(uri, this.token);
> +    if (app.modifiedAt) {
> +      req.setHeader("X-If-Unmodified-Since", "" + app.modified);
> +    }

Please set timeout on request.

@@ +232,5 @@
> +    let self = this;
> +    this._log.info("Trying to _putApp to " + uri);
> +    req.put(JSON.stringify(app), function(err) {
> +      self._setBackoff(req);
> +      self._processPutApp(err, cb, req);

Above, _setBackoff() was called in _processGetApps(). Please make style consistent for readability.

@@ +284,5 @@
> +
> +  _makeAppURI: function _makeAppURI(origin) {
> +    let part = CommonUtils.encodeBase64URL(
> +      CryptoUtils.UTF8AndSHA1(origin)
> +    ).replace("=", "");

We should probably add a "pad" argument to encodeBase64URL...

@@ +298,5 @@
> +    let time = Date.now();
> +    let backoff = parseInt(this._state.get("backoff", 0), 10);
> +
> +    if (time < backoff) {
> +      this._log.warn(backoff - time + " left for backoff, not making request");

Nit: add unit to time measurement (ms).

@@ +303,5 @@
> +      return false;
> +    }
> +
> +    this._backoff = false;
> +    this._state.set("backoff", "0");

It seems excessive to have two variables to track backoff. Perhaps just store the wall time backoff will end or null/0 if there is no backoff in effect. If the current time is after the stored backoff time, just clear it. Or, you could leave the past time stored if you really wanted to.

@@ +310,5 @@
> +
> +  // Set values from X-Backoff and Retry-After headers, if present
> +  _setBackoff: function _setBackoff(req) {
> +    let backoff = 0;
> +    let time = Date.now();

You don't use this until an if block at the end of this function. Move it to there, please.

@@ +314,5 @@
> +    let time = Date.now();
> +
> +    if (req.response.headers["X-Backoff"]) {
> +      backoff = parseInt(req.response.headers["X-Backoff"], 10);
> +      this._log.warn("X-Backoff header was seen: " + backoff);

Nit: log the original string value in case it is weird and parseInt loses the weirdness.

@@ +316,5 @@
> +    if (req.response.headers["X-Backoff"]) {
> +      backoff = parseInt(req.response.headers["X-Backoff"], 10);
> +      this._log.warn("X-Backoff header was seen: " + backoff);
> +    }
> +    if (req.response.headers["Retry-After"]) {

I wonder if this should be "else if." rfkelly?

@@ +318,5 @@
> +      this._log.warn("X-Backoff header was seen: " + backoff);
> +    }
> +    if (req.response.headers["Retry-After"]) {
> +      backoff = parseInt(req.response.headers["Retry-After"], 10);
> +      this._log.warn("Retry-Header header was seen: " + backoff);

Log original header.
Attachment #628524 - Flags: review?(gps) → review+
Comment on attachment 628524 [details] [diff] [review]
AITC Client RC2.1

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

looks good, followups mentioned are on the master list at https://etherpad.mozilla.org/aitc-followups

::: services/aitc/modules/client.js
@@ +113,5 @@
> +    
> +    if (err) {
> +      this._log.error("getApps request error " + err);
> +      cb(err, null);
> +      return;

As with the comment on PUTs, we should track errors and back off on our own, even if the server hasn't told us yet.

@@ +215,5 @@
> +      // PUT requests may qualify as the "minimum number of additional requests
> +      // required to maintain consistency of their stored data". However, it's
> +      // better to keep server load low, even if it means user's apps won't
> +      // reach their other devices during the early days of AITC. We should
> +      // revisit this when we have a better of idea of server load curves.

Future followup (adding to the list): At some point there will always certainly be a UI/postMessage hook to trigger these updates.  X-Backoff, in practice, is "please don't unless you need to" whereas 503+Retry-After is "don't bother, it won't work."  For PUTs in particular I think there's a clear case to be made for still doing these, even if we're refraining from checking for data.

@@ +216,5 @@
> +      // required to maintain consistency of their stored data". However, it's
> +      // better to keep server load low, even if it means user's apps won't
> +      // reach their other devices during the early days of AITC. We should
> +      // revisit this when we have a better of idea of server load curves.
> +      err = new Error("X-Backoff in effect, aborting PUT");

Backoff, not X-Backoff, since there's a couple of ways to get here.

@@ +247,5 @@
> +      return;
> +    }
> +
> +    let err = null;
> +    switch (req.response.status) {

409 needs to be handled here.  It's not unexpected, nor should we remove it from the queue.

@@ +256,5 @@
> +        break;
> +
> +      case 400:
> +      case 412:
> +      case 413:

If we get 413 here, it's probabaly a client bug.  I think we need to figure out how to a) prevent ourselves from tripping the limit and b) ensuring that we detect and recover from this, rather than just dropping the data.  Added to the list of followups.

@@ +284,5 @@
> +
> +  _makeAppURI: function _makeAppURI(origin) {
> +    let part = CommonUtils.encodeBase64URL(
> +      CryptoUtils.UTF8AndSHA1(origin)
> +    ).replace("=", "");

what GPS said! (added to followup list)

@@ +303,5 @@
> +      return false;
> +    }
> +
> +    this._backoff = false;
> +    this._state.set("backoff", "0");

why are you setting this to a string?

@@ +316,5 @@
> +    if (req.response.headers["X-Backoff"]) {
> +      backoff = parseInt(req.response.headers["X-Backoff"], 10);
> +      this._log.warn("X-Backoff header was seen: " + backoff);
> +    }
> +    if (req.response.headers["Retry-After"]) {

GPS flagged this as an "else if", as only one can win.  I would submit that Retry-After should win here.  The alternative is that the second if block needs to set backoff to the higher of two values.

@@ +322,5 @@
> +      this._log.warn("Retry-Header header was seen: " + backoff);
> +    }
> +    if (backoff) {
> +      this._backoff = true;
> +      this._state.set("backoff", "" + (time + (backoff * 1000)));

Backoff should ideally add some fuzz, so clients don't all retry at the same time. (the slack timer isn't _that_ fuzzy).  Stealing from _calculateBackoff in Sync [1], maybe something like this:

Math.floor((Math.random() * backoff + backoff) * 1000)

[1] http://mxr.mozilla.org/services-central/source/fx-sync/services/sync/modules/service.js#2061

@@ +324,5 @@
> +    if (backoff) {
> +      this._backoff = true;
> +      this._state.set("backoff", "" + (time + (backoff * 1000)));
> +    }
> +  },

General comment on backoff is that we're missing a major chunk of backoff, which is "doing it if the server is melting, even if Ops isn't out of bed yet."

That _shouldn't_ happen, but "we're getting unexpected 503s" should be something we track and start backing off ourselves, somehow.  (i.e. three failed poll requests in a row should cause us to slow down until we succeed again).  Added to followups.

::: services/aitc/service.js
@@ +26,5 @@
> +        break;
> +      case "final-ui-startup":
> +        // Start AITC service after 2000ms, only if classic sync is off.
> +        Cu.import("resource://services-common/preferences.js");
> +        if (Preferences.get("services.sync.engine.apps", false)) {

I think we need a kill-switch that doesn't depend on a different feature being enabled.  Add services.aitc.enabled here?
Attachment #628524 - Flags: review?(mconnor) → review+
Attached patch AITC Client RC2.2 (obsolete) — Splinter Review
Addressed most issues raised by Greg and Mike! Some comments on the ones  I didn't:

(In reply to Gregory Szorc [:gps] from comment #10)
> @@ +303,5 @@
> > +      return false;
> > +    }
> > +
> > +    this._backoff = false;
> > +    this._state.set("backoff", "0");
> 
> It seems excessive to have two variables to track backoff. Perhaps just
> store the wall time backoff will end or null/0 if there is no backoff in
> effect. If the current time is after the stored backoff time, just clear it.
> Or, you could leave the past time stored if you really wanted to.

One of them is local, the other one is persistent (as a pref). Not sure what you mean by two variables.


> @@ +316,5 @@
> > +    if (req.response.headers["X-Backoff"]) {
> > +      backoff = parseInt(req.response.headers["X-Backoff"], 10);
> > +      this._log.warn("X-Backoff header was seen: " + backoff);
> > +    }
> > +    if (req.response.headers["Retry-After"]) {
> 
> I wonder if this should be "else if." rfkelly?

I changed to else if but put Retry-After before X-Backoff as suggested by mconnor.

> ::: services/aitc/modules/client.js
> @@ +113,5 @@
> > +    
> > +    if (err) {
> > +      this._log.error("getApps request error " + err);
> > +      cb(err, null);
> > +      return;
> 
> As with the comment on PUTs, we should track errors and back off on our own,
> even if the server hasn't told us yet.

I think we can make a low-risk change in manager.js where we track the numbers of times a certain operation fails and not try beyond that threshold. Until we implement something better as a followup, of course.

> @@ +247,5 @@
> > +      return;
> > +    }
> > +
> > +    let err = null;
> > +    switch (req.response.status) {
> 
> 409 needs to be handled here.  It's not unexpected, nor should we remove it
> from the queue.

Added. I also made a special case for 401 (since tokens expire every 5 minutes, we might get a couple now and then).

> @@ +303,5 @@
> > +      return false;
> > +    }
> > +
> > +    this._backoff = false;
> > +    this._state.set("backoff", "0");
> 
> why are you setting this to a string?

Integer prefs cannot store large numbers.
There is some trailing whitespace in your latest patch. Please remove it.
The patch is also missing some Mercurial headers, including the author and summary lines. Please upload a final patch and request review.

Other than that, I think this one is ready to land on s-c. I have a try at http://tbpl.mozilla.org/?tree=Try&rev=5c2e41812a8d
Blocks: 755375
Attached patch AITC Client RC2.3 (obsolete) — Splinter Review
$ fortune -s
The difficult we do today; the impossible takes a little longer.
Attachment #629054 - Attachment is obsolete: true
Attachment #629236 - Flags: review?(gps)
digitirald ran into an exception that we can catch. remoteInstall shouldn't be called if there's not an entry for that app in the DOMApplicationRegistry, if it is, we'll return an error via the callback.
Attachment #629236 - Attachment is obsolete: true
Attachment #629236 - Flags: review?(gps)
Attachment #629265 - Flags: review?(gps)
Attachment #629265 - Attachment is patch: true
Comment on attachment 629265 [details] [diff] [review]
AITC Client RC2.4

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

Need proper summary line in patch.

::: services/aitc/modules/client.js
@@ +180,5 @@
> +    // Return success.
> +    try {
> +      cb(null, apps);
> +      // Don't update lastModified until we know cb succeeded.
> +      this._appsLastModified = parseInt(req.response.headers["X-Timestamp"]);

missig radix argument to parseInt.
https://hg.mozilla.org/services/services-central/rev/c6956e485b2e

Please file separate bugs for follow-ups!
Whiteboard: [fixed in services]
https://hg.mozilla.org/mozilla-central/rev/c6956e485b2e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Whiteboard: [qa+]
Verified on Nightly on Windows 7 through doing an initial test pass on AITC desktop. I can confirm that this has indeed landed. All follow-up issues should be filed in separate bugs.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Comment on attachment 629265 [details] [diff] [review]
AITC Client RC2.4

Bug landed, clearing flag.
Attachment #629265 - Flags: review?(gps)
Product: Web Apps → Web Apps Graveyard
You need to log in before you can comment on or make changes to this bug.