This bug is to track implementation of the AITC client in Firefox Desktop. This is currently being worked on in a fork of services-central: https://github.com/anantn/services-central The API that the client is written against is described at: https://github.com/mozilla-services/docs/blob/master/source/aitc/apis-1.0.rst

The first time you open marketplace user has to log into PersonID. This also logs you into the dashboard If you go directly to the dashboard you will have login Install should not fail if AitC fails to be written to. We should keep retrying until we can sync. The dashboard will live on apps.persona.org How to get to the dashboard on the desktop: * Direct URL - apps.persona.org * about:apps - redirects to apps.persona.org * link from Marketplace - From the Purchased App section have a link to about:apps Operations of the dashboard: * Install new apps * Re-install "installed" apps (per device) * View all apps in the cloud ** nice-to have is to distinguish on device from cloud Non-goal: * Launching an app from the dashboard

(In reply to Jennifer Arguello :ticachica from comment #1) > > The first time you open marketplace user has to log into PersonID. This > also logs you into the dashboard Is that feasible? Sounds like sign into browser with something specific to marketplace. > > If you go directly to the dashboard you will have login > > Install should not fail if AitC fails to be written to. We should keep > retrying until we can sync. How often should retry occur (i.e. time)? > The dashboard will live on apps.persona.org > > How to get to the dashboard on the desktop: > * Direct URL - apps.persona.org > * about:apps - redirects to apps.persona.org > * link from Marketplace - From the Purchased App section have a link to > about:apps > Operations of the dashboard: > * Install new apps > * Re-install "installed" apps (per device) > * View all apps in the cloud > ** nice-to have is to distinguish on device from cloud We need to get corresponding bugs created for this and link it to this bug. > Non-goal: > * Launching an app from the dashboard Technically you can already do that from myapps.mozillalabs.com, although we still need resolution on what we should do in the short term (there's a bug filed about launching causing a pinned tab to occur). For the short term, should we default to the typical behavior?

(In reply to Jason Smith from comment #2) > > The first time you open marketplace user has to log into PersonID. This > > also logs you into the dashboard > > Is that feasible? Sounds like sign into browser with something specific to > marketplace. We only ask the user to login to the marketplace if they choose to install an app. We already do this, so we are good. > > Install should not fail if AitC fails to be written to. We should keep > > retrying until we can sync. > > How often should retry occur (i.e. time)? This is defined by the backoff algorithm and is part of the AitC server API. We start with a small retry interval and keep increasing it for every failure in order to avoid swamping the server. > > The dashboard will live on apps.persona.org ... > We need to get corresponding bugs created for this and link it to this bug. Yes, it is still unclear if we can get persona.org ready in time for this, if not, we are going to fall back to myapps.mozillalabs.com. > Technically you can already do that from myapps.mozillalabs.com, although we > still need resolution on what we should do in the short term (there's a bug > filed about launching causing a pinned tab to occur). For the short term, > should we default to the typical behavior? No, I believe we should disable the functionality of being able to launch apps from myapps.mozillalabs.com entirely.

(In reply to Jennifer Arguello :ticachica from comment #1) > The first time you open marketplace user has to log into PersonID. This > also logs you into the dashboard > > If you go directly to the dashboard you will have login Thanks for the clear description Jen! There is one case that is not covered by these two points, and that pertains to app installs that are not from the marketplace: - User installs app 'foo' on computer A from appdir (or any other non-marketplace location). - User goes to computer B and logs in with their PersonaID. - User is confused about app 'foo' not appearing on the dashboard. This is because no login occurred (on the marketplace or the dashboard) when app 'foo' was installed on computer A. I expect this case to occur rarely in the beginning (so I wouldn't consider it a blocker for now), but as more marketplaces appear in the future, it is something we have to tackle. Perhaps by including a login in the install process if the user isn't already logged to either the marketplace or dashboard.

(In reply to Anant Narayanan [:anant] from comment #3) > (In reply to Jason Smith from comment #2) > > > The dashboard will live on apps.persona.org > ... > > We need to get corresponding bugs created for this and link it to this bug. > > Yes, it is still unclear if we can get persona.org ready in time for this, > if not, we are going to fall back to myapps.mozillalabs.com. We're already tight on time as is. My opinion - I'd just stick with myapps.mozillalabs.com. Jen - Any objections? > > Technically you can already do that from myapps.mozillalabs.com, although we > > still need resolution on what we should do in the short term (there's a bug > > filed about launching causing a pinned tab to occur). For the short term, > > should we default to the typical behavior? > > No, I believe we should disable the functionality of being able to launch > apps from myapps.mozillalabs.com entirely. Why? This will stop users from being able to launch apps in FF 13 or higher and any other browser. For marketplace beta, there's a good chance we'll have users using the HTML 5 dashboard still, given that the large majority of our user base will be on release (FF 12) and beta (FF 13). There's also support for other browsers as well.

(In reply to Jason Smith from comment #5) > > No, I believe we should disable the functionality of being able to launch > > apps from myapps.mozillalabs.com entirely. > > Why? This will stop users from being able to launch apps in FF 13 or higher > and any other browser. For marketplace beta, there's a good chance we'll > have users using the HTML 5 dashboard still, given that the large majority > of our user base will be on release (FF 12) and beta (FF 13). There's also > support for other browsers as well. Hmm, tricky. One possibility is to disable launch functionality only if there is a native implementation of mozApps present (i.e. navigator.mozApps.html5implementation is undefined). Ian, thoughts?

If we want to discuss dashboard launching we should do so in a bug about the dashboard.

This is v1 of the AITC desktop client implementation. Putting the patch up for some early feedback to make sure there are no big surprises. Known issues with this patch that we will address shortly are: - Use of dump, this will be changed to error/debug logging as necessary. - An app is not marked as deleted upon the webapp-sync-uninstall event. - AITC functionality is not pref'ed, and thus cannot be turned off. Non-goals for this patch are: - Supporting device metadata synchronization. - Automated tests. These will very hard to write in time because of the dependence on several external servers: BrowserID, Sagrada Tokens, AITC itself. - Removing the existing "apps" sync engine: we should do this in a separate bug once this patch lands.

Autoland Patchset: Patches: 614662 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=248adc87ca17 Try run started, revision 248adc87ca17. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=248adc87ca17

Minor whitespace changes to the earlier patch for better indentation.

Comment on attachment 614664 [details] [diff] [review] AITC Desktop Client v1.1 Pretty cool! Allow me to drop a few comments from a drive-by skim: * There's a bunch of unrelated whitespace changes in Webapps.jsm. I'm all for removing trailing whitespace, but it would be good if those were in a separate patch. * In fact, the changes to the DOM code should be in a separate bug and reviewed by an appropriate peer (fabrice would be a good candidate). * Speaking of whitespace, you're using tabs in a lot of places. Please always indent by 2 spaces. I suggest configuring your editor to never ever produce tabs. * New license header, plz! http://www.mozilla.org/MPL/headers/ * In a few places you acquire XPCOM services. Some of those are already available through Services.jsm. Please use those shorthands. When they're not available through Services.jsm, please define local getters with XPCOM.defineLazyServiceGetter to avoid unnecessary XPCOM lookups. * You already have comments for these, but it can't hurt to confirm: Yes, the URLs that are in the code should definitely be prefs. And yes, the dump() stuff should go away; I suggest replacing with log4moz calls. Hook up a DumpAppender() with a pref, that way it's super easy (e.g. for QA) to turn on logging.

(In reply to Philipp von Weitershausen [:philikon] from comment #11) > * In fact, the changes to the DOM code should be in a separate bug and > reviewed by an appropriate peer (fabrice would be a good candidate). Good idea, I filed bug 745069 and will separate the DOM changes from this patch and put them there. > * Speaking of whitespace, you're using tabs in a lot of places. Please > always indent by 2 spaces. I suggest configuring your editor to never ever > produce tabs. Hmm, I don't see any tabs in attachment 614644 [details] [diff] [review] (except the one in the Makefile) - the earlier patch (v1) had them - let me check again. It's possible there are 4 spaces instead of 2 in some places. > * New license header, plz! http://www.mozilla.org/MPL/headers/ > > * In a few places you acquire XPCOM services. Some of those are already > available through Services.jsm. Please use those shorthands. When they're > not available through Services.jsm, please define local getters with > XPCOM.defineLazyServiceGetter to avoid unnecessary XPCOM lookups. > > * You already have comments for these, but it can't hurt to confirm: Yes, > the URLs that are in the code should definitely be prefs. And yes, the > dump() stuff should go away; I suggest replacing with log4moz calls. Hook up > a DumpAppender() with a pref, that way it's super easy (e.g. for QA) to turn > on logging. Sounds good!

(In reply to Anant Narayanan [:anant] from comment #12) > > * Speaking of whitespace, you're using tabs in a lot of places. Please > > always indent by 2 spaces. I suggest configuring your editor to never ever > > produce tabs. > > Hmm, I don't see any tabs in attachment 614644 [details] [diff] [review] > (except the one in the Makefile) - the earlier patch (v1) had them - let me > check again. It's possible there are 4 spaces instead of 2 in some places. I admit I was reading v1 while you uploaded v1.1. So if you already took care of this, great!

Try run for 248adc87ca17 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=248adc87ca17 Results (out of 15 total builds): exception: 4 failure: 11 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-248adc87ca17

Comment on attachment 614664 [details] [diff] [review] AITC Desktop Client v1.1 Review of attachment 614664 [details] [diff] [review]: ----------------------------------------------------------------- This is only part 1 of the review. I figure there is enough here to unblock work on changes. I'll start work on part 2 as soon as this is submitted... General feedback: * When defining functions, always define a function name so backtraces don't display anonymous functions. Always match the function name to the property name. I identified a few of these and stopped. * Add full stops (periods) to all sentences. * Use Error or Error-derived types instead of strings. * MPL 2.0 * Please document public APIs using Javadoc notation. See services/common/tokenserverclient.js for examples. * Always "use strict"; ::: services/aitc/Aitc.js @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** Consider renaming file to service.js. If not, please lowercase to aitc.js. @@ +33,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ MPL 2.0. @@ +37,5 @@ > + * ***** END LICENSE BLOCK ***** */ > + > +const Cc = Components.classes; > +const Ci = Components.interfaces; > +const Cu = Components.utils; Nit: We generally prefer destructuring assignment in our style: const {classes: Cc, interfaces: Ci, utils: Cu} = Components; @@ +52,5 @@ > + > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, > + Ci.nsISupportsWeakReference]), > + > + observe: function _observe(subject, topic, data) { match function name to property name @@ +54,5 @@ > + Ci.nsISupportsWeakReference]), > + > + observe: function _observe(subject, topic, data) { > + switch (topic) { > + case "app-startup": style: entire body of switch needs indent. ::: services/aitc/Makefile.in @@ +16,5 @@ > + > +PREF_JS_EXPORTS = $(srcdir)/services-aitc.js > + > +libs:: > + $(PYTHON) $(topsrcdir)/config/nsinstall.py $(srcdir)/modules/* $(FINAL_TARGET)/modules/services-aitc $(NSINSTALL) $(srcdir)/modules/* $(FINAL_TARGET)/modules/services-aitc ::: services/aitc/modules/browserid.js @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** The contents of this file are not AFAICT specific to AITC and can be used by other services (like Sync). We should split this file off into a separate review. We should move it to services/common or somewhere more global (like toolkit). I think services/common is fine for now. It can "graduate" elsewhere once it has matured. @@ +31,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ MPL 2.0 @@ +32,5 @@ > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ > + "use strict"; @@ +37,5 @@ > +const EXPORTED_SYMBOLS = ["BrowserID"]; > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; > + > +// TODO: Make pref so it's easy to switch to dev > +const ID_URI = "https://browserid.org"; And when you do make it a pref, use resource://services-common/preferences.js @@ +45,5 @@ > + this._container = null; > + this._emails = []; > +} > +BrowserIDSvc.prototype = { > + _getSandbox: function(cb) { define function name @@ +62,5 @@ > + */ > + // This way of obtaining a window is more fickle than the commented way > + // but appShellService doesn't work sometimes. Investigate. > + let wM = Cc["@mozilla.org/appshell/window-mediator;1"] > + .getService(Ci.nsIWindowMediator); style: line up ".getService" under the "C" in "Cc" @@ +66,5 @@ > + .getService(Ci.nsIWindowMediator); > + let win = wM.getMostRecentWindow("navigator:browser"); > + let doc = win.document; > + > + // Insert iframe in to create docshell style: add period to sentence. @@ +74,5 @@ > + doc.documentElement.appendChild(frame); > + > + // Set instance properties for reuse > + this._frame = frame; > + this._container = doc.documentElement; I would move all this DOM manipulation logic to a separate function. @@ +82,5 @@ > + webNav.stop(Ci.nsIWebNavigation.STOP_NETWORK); > + > + let self = this; > + let parseHandler = { > + handleEvent: function(event) { style: Define function name. @@ +87,5 @@ > + event.target.removeEventListener("DOMContentLoaded", this, false); > + let workerWindow = self._frame.contentWindow; > + let sandbox = new Cu.Sandbox(workerWindow, { > + sandboxPrototype: workerWindow, > + wantXrays: false style: vertically align values in object definitions. @@ +94,5 @@ > + } > + }; > + > + // Make channel > + let iOservice = Cc["@mozilla.org/network/io-service;1"] let ioService. @@ +95,5 @@ > + }; > + > + // Make channel > + let iOservice = Cc["@mozilla.org/network/io-service;1"] > + .getService(Ci.nsIIOService); style: Align with Cc @@ +101,5 @@ > + > + // Load the iframe > + this._frame.addEventListener("DOMContentLoaded", parseHandler, true); > + let uriLoader = Cc["@mozilla.org/uriloader;1"].getService(Ci.nsIURILoader); > + uriLoader.openURI(channel, true, this._frame.docShell); I'd like someone with more experience to review this content loading behavior, as I'm not confident in my understand of it. @@ +105,5 @@ > + uriLoader.openURI(channel, true, this._frame.docShell); > + }, > + > + // Obtain a BrowserID assertion (if user is already logged in) > + getAssertion: function(email, audience, cb) { Don't call cb blindly. If it is required, throw an Error if it is not proper. @@ +108,5 @@ > + // Obtain a BrowserID assertion (if user is already logged in) > + getAssertion: function(email, audience, cb) { > + let address = email; > + if (!address) { > + if (!this._emails.length === 0) { This doesn't do what you think it does. ! is higher priority than ===. So, this is effectively (!this._emails.length) === 0. This translates to true === 0 or false === 0, both of which are always false. Also, I'm not sure what the strong type comparison buys you in this case. You know this._emails is an array, so .length is always defined. You want to write: if (!this._emails.length) @@ +110,5 @@ > + let address = email; > + if (!address) { > + if (!this._emails.length === 0) { > + dump("!!! AITC error: getAssertion called when user is not loggedIn\n"); > + cb("ERROR: Not logged in", null); Error or Error-derived types are preferred. @@ +112,5 @@ > + if (!this._emails.length === 0) { > + dump("!!! AITC error: getAssertion called when user is not loggedIn\n"); > + cb("ERROR: Not logged in", null); > + return; > + } else { Since you return from the above, no need for an else {} @@ +116,5 @@ > + } else { > + // Just pick the first one in the list > + address = this._emails[0]; > + } > + } How about: let address = email || this._emails[0]; if (!address) { ... } I'm not 100% this is what you want. Please document the function so the docs are clear about the behavior if the email argument is falsy. @@ +122,5 @@ > + cb("ERROR: audience not provided", null); > + return; > + } > + > + this._getSandbox(function(sandbox) { A few places in this file we have the pattern of this._getSandbox(function(sandbox) { ... }. While I like that the logic for the encasing functions is all centrally defined, part of me thinks that these inner functions are long enough to warrant moving into the top-level prototype. Complicating things is that the pattern seems to use 2 callbacks: 1 called by _getSandbox() when DOMContentLoaded is fired and 1 called within that callback to pass results to the original caller. How about changing _getSandbox() to take the user-supplied callback so you don't have to create an extra closure every invocation. e.g. function _getSandbox(cb, cbArguments) { ... function onDOMContentLoaded() { onContentLoaded.apply(this, cbArguments); // The above needs to also pass sandbox. I'm being lazy in this example. } } And then writing getAssertion() (and related) like: function getAssertion(cb) { ... this._getSandbox(this._onGetAssertionContentLoaded, [cb]); } function _onGetAssertionContentLoaded(sandbox, cb) { function function successCb(res) { cb(null, res); } ... } @@ +134,5 @@ > + sandbox.importFunction(errorCb, "errorCb"); > + > + let scriptText = > + "window.BrowserID.User.getAssertion('" + address + "', '" + audience + > + "', successCb, errorCb);"; Ugh. There is no good way to write this. But, I think it can be better. How about: let scriptText = 'window.BrowserID.User.getAssertion(' + '"' + address + '", ' + '"' + audience + '", ' + 'successCb, ' + 'errorCb' + ');'; My personal preference is to use consistence quotes throughout. I'm sure rnewman or philikon can come up with an even better style with better vertical alignment. @@ +140,5 @@ > + }); > + }, > + > + // Get an assertion by asking the user to login in the context of win > + getAssertionWithLogin: function(win, cb) { Function name. Don't call cb blindly. @@ +159,5 @@ > + Cu.evalInSandbox(scriptText, sandbox, "1.8", ID_URI, 1); > + }, > + > + // Check if a user is logged in to browserid.org > + isLoggedIn: function _isLoggedIn(cb) { Match function name. @@ +167,5 @@ > + let list = []; > + try { > + list = JSON.parse(res); > + } catch (e) { > + cb(false); return; This initially threw me off because I inferred that the first argument to all callbacks in the public API was the error that occurred. Please document the arguments passed to the callback! Also, consider adopting the same callback behavior for all APIs. @@ +171,5 @@ > + cb(false); return; > + } > + > + let keys = Object.keys(list); > + if (keys.length === 0) { if (!x.length) @@ +174,5 @@ > + let keys = Object.keys(list); > + if (keys.length === 0) { > + cb(false); > + } else { > + self._emails = keys; What's going on here? We essentially have: let list = []; list = JSON.parse(res); let keys = Object.keys(list); self._emails = keys; If list is an Array, Object.keys(list) will be the numeric indices. @@ +187,5 @@ > + }); > + }, > + > + // List all emails associated with this user on browserid.org > + listEmails: function(cb) { How about getEmails() instead? "list" implies you are actually doing something with me (to me at least). @@ +188,5 @@ > + }, > + > + // List all emails associated with this user on browserid.org > + listEmails: function(cb) { > + let self = this; You only access self._emails, so how about creating a reference directly to _emails instead? Or, consider .bind(this) on the inline functions. @@ +189,5 @@ > + > + // List all emails associated with this user on browserid.org > + listEmails: function(cb) { > + let self = this; > + if (this._emails.length === 0) { if (!x.length) @@ +192,5 @@ > + let self = this; > + if (this._emails.length === 0) { > + this._isLoggedIn(function(yes) { > + if (yes) cb(self._emails); > + else cb([]); style: Always use {} (complete with newlines) for conditionals. @@ +202,5 @@ > + > + // Get the email last used to login to audience > + getEmailForAudience: function(audience, cb) { > + let self = this; > + this._getSandbox(function(sandbox) { At this point I realized that _getSandbox() is called a lot. Every single time it is loading content and doing DOM foo. Is there any way we could persist the sandbox instance? @@ +212,5 @@ > + }); > + } > +}; > + > +let BrowserID = new BrowserIDSvc(); XPCOMUtils.defineLazyGetter() @@ +213,5 @@ > + } > +}; > + > +let BrowserID = new BrowserIDSvc(); > + style: Remove trailing newline.

Moved the BrowserID module over to Bug 745345. I have an updated patch for the rest of the code, however, I will wait for part 2 of your feedback to make sure I address your comments before submitting a new version of the patch.

Comment on attachment 614664 [details] [diff] [review] AITC Desktop Client v1.1 Review of attachment 614664 [details] [diff] [review]: ----------------------------------------------------------------- Part 2. I stopped reviewing in the middle of main.js after I saw a new patch is available. Please submit the next patch and I will continue. ::: services/aitc/modules/client.js @@ +40,5 @@ > +const FREQ = 10000; > +const EXPORTED_SYMBOLS = ['AitcClient']; > +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource://services-sync/util.js"); We shouldn't need to import any Sync modules from here. Let's file a bug to refactor the necessary bits to services-common or services-crypto. I can probably do that patch for you. @@ +48,5 @@ > + > +function AuthRESTRequest(uri, authToken) { > + RESTRequest.call(this, uri); > + this.authToken = authToken; > + this._log.level = 30; This should come from a pref. You should also use the constants. e.g. this._log.level = Log4Moz.Level[Prefs.get("log.logger.authrequest.level")]; See services/common/rest.js for an example. @@ +68,5 @@ > + dump("!! Request result: " + this.uri.asciiSpec + " status: " + this.response.status + " bodylength: " + this.response.body.length + "\n"); > + onComplete(error); > + }, onProgress); > + } > +}; rnewman/philikon: How do you feel about making this pattern easier. We can add onDispatch, onXXX empty functions to RESTRequest. This would prevent callers from having to override a built-in method. Or, if we wanted to get rid of the requirement to create a new type, we could have a listeners/hooks system. e.g. addDispatchListener(function(request) { }) or addListener({onDispatch: function(request) { }}); anant: Since there is nothing AITC-specific about this type (it is a generic HTTP MAC authenticated request), let's move it to services-common. @@ +71,5 @@ > + } > +}; > + > +function AitcClient(token, registry) { > + this.uri = token.endpoint.replace(/\/+$/, ''); JavaScript: Y U NO PROVIDE DECENT STRING API? @@ +73,5 @@ > + > +function AitcClient(token, registry) { > + this.uri = token.endpoint.replace(/\/+$/, ''); > + dump('!!! AITC !!! token endpoint: ' + this.uri + '\n'); > + this.token = token; So you normalize the endpoint/URI then make a copy of it again? This just doesn't feel clean to me. Perhaps you should just store all the token bits as separate properties? @@ +91,5 @@ > + dump("!!! AITC !!! Error for request: " + req.response.status + " :: " + req.response.body + "\n"); > + }, > + > + _makeRequest: function(uri) { > + if (typeof uri != 'string') { Always use "" for strings (unless they contain literal ") @@ +92,5 @@ > + }, > + > + _makeRequest: function(uri) { > + if (typeof uri != 'string') { > + throw 'Bad request URI: ' + uri; Always throw Error instances. @@ +98,5 @@ > + return new AuthRESTRequest(uri, this.token); > + }, > + > + _makeAppURI: function(origin) { > + let part = btoa(Utils._sha1(origin)).replace(/\+/, '-').replace(/\//, '_').replace(/=/, ''); For single character replace, use literal characters instead of regular expressions. This function is also Utils.encodeBase64url() from services-sync/util.js. Let's move that to CommonUtils and use it. @@ +102,5 @@ > + let part = btoa(Utils._sha1(origin)).replace(/\+/, '-').replace(/\//, '_').replace(/=/, ''); > + return this.uri + '/apps/' + part; > + }, > + > + getApps: function(cb) { Please throw if cb is not proper. @@ +105,5 @@ > + > + getApps: function(cb) { > + let self = this; > + // If there's a unfinished PUT, just bail > + if (self._putInProgress) { You aren't in the closure yet. Use this, not self. @@ +106,5 @@ > + getApps: function(cb) { > + let self = this; > + // If there's a unfinished PUT, just bail > + if (self._putInProgress) { > + cb("AITC: PUT in progress, aborting GET!"); new Error("..."); Issuing a request while another is in progress would constitute misuse of the API by the caller. Therefore, I think you should throw instead. Also, a getter to allow the caller to "look before you leap" (in case they are lazy and don't want to keep track of PUT state) would be nice here. @@ +118,5 @@ > + } > + req.get(function (error) { > + if (error) { > + cb(error); > + dump("!!! AITC !!! Got from getApps error " + error + "\n"); You probably want to log before you call the callback. @@ +128,5 @@ > + return; > + } > + if (req.response.status == 200) { > + dump("!!! AITC !!! Got from getApps: " + req.response.body + " :: " + req.response.body.length + "\n"); > + let tmp = JSON.parse(req.response.body); If this throws, the error gets swallowed by XPCOM, I believe. You should probably put the whole onComplete handler inside a try..catch. See TokenServerClient for an example. You should probably verify the Content-Type, just to be sure. @@ +137,5 @@ > + } > + }); > + }, > + > + putApp: function(appRec, appLastModified, cb) { Mark "internal" as _putApp()? Is it only ever called by remoteInstall()? @@ +150,5 @@ > + req.setHeader('X-If-Unmodified-Since', appLastModified); > + } > + > + dump("!!! AITC !!! Calling putApp with " + JSON.stringify(appRec) + "\n"); > + req.put(JSON.stringify(appRec), function (error) { If putApp() is only called by remoteInstall(), this is fine. If not, you may want to validate that appRec is a specific type (with toJSON()) or you will want to pass an array of field names to JSON.stringify(). See https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/JSON/stringify. @@ +158,5 @@ > + return; > + } > + if (req.response.status == 412) { > + dump("!!! AITC !!! Got from putApp 412\n"); > + cb({preconditionFailed: true}); Always use Error types. Either invent your own Error-inheriting type for this condition. Or, store this flag off of Error. e.g. let error = new Error("..."); error.preconditionFailed = true; cb(error); @@ +161,5 @@ > + dump("!!! AITC !!! Got from putApp 412\n"); > + cb({preconditionFailed: true}); > + return; > + } else if (req.response.status != 201 && req.response.status != 204) { > + self._error(req); cb is never called. @@ +165,5 @@ > + self._error(req); > + return; > + } > + > + dump("!!! AITC !!! Got from putApp: " + req.response.body + " :: " + req.response.body.length + "\n"); 201 and 204 responses for the API don't have a body. It makes little sense to log one. (Although, if they ever did, that WOULD be interesting and worth logging.) @@ +166,5 @@ > + return; > + } > + > + dump("!!! AITC !!! Got from putApp: " + req.response.body + " :: " + req.response.body.length + "\n"); > + cb(); cb(null); - just to be explicit about the API. @@ +167,5 @@ > + } > + > + dump("!!! AITC !!! Got from putApp: " + req.response.body + " :: " + req.response.body.length + "\n"); > + cb(); > + self._putInProgress = false; What if there are multiple putApp() in progress? @@ +171,5 @@ > + self._putInProgress = false; > + }); > + }, > + > + remoteInstall: function(app) { Why no callback? @@ +180,5 @@ > + let record = { > + origin: app.origin, > + installOrigin: app.installOrigin, > + manifestPath: app.manifestURL, > + receipts: app.receipts Nit: vertically align values. @@ +186,5 @@ > + > + if ('installedAt' in app) > + record.installedAt = app.installedAt; > + if ('modifiedAt' in app) > + record.modifiedAt = app.modifiedAt; style: Always use {} @@ +188,5 @@ > + record.installedAt = app.installedAt; > + if ('modifiedAt' in app) > + record.modifiedAt = app.modifiedAt; > + > + this.putApp(record, null, function (error) {}); Consider logging error. (I know you log some in putApp(), but it feels more appropriate to have a more-specific logged message. @@ +191,5 @@ > + > + this.putApp(record, null, function (error) {}); > + }, > + > + processResponse: function(resp, callback) { At this point you are processing an apps object from the response, not a full response. Consider changing the function name. @@ +193,5 @@ > + }, > + > + processResponse: function(resp, callback) { > + let self = this; > + let allApps = DOMApplicationRegistry.getAllWithoutManifests(function (apps) { If 90% of your function body is an inner function, you should probably move that inner function to the next highest level. In this case, move this processing function to the prototype or call DOMApplicationRegistry.getAllWithoutManifests() before processResponse() and pass the result to this function. Also, allApps is unused. @@ +201,5 @@ > + let commands = []; > + > + // These are all the local apps > + for (let i in apps) { > + originToId[apps[i].origin] = i; If the app record/object itself stored the ID, we could eliminate this variable. Just something to think about. @@ +202,5 @@ > + > + // These are all the local apps > + for (let i in apps) { > + originToId[apps[i].origin] = i; > + existingByOrigin[apps[i].origin] = toDelete[apps[i].origin] = apps[i]; since you access apps[i] so often, let's just rewrite the loop using Iterator: for (let [id, app] in Iterator(apps)) { ... } @@ +206,5 @@ > + existingByOrigin[apps[i].origin] = toDelete[apps[i].origin] = apps[i]; > + } > + > + // Iterate over remote apps > + for (let i=0; i<resp.length; i++) { Rename "resp" to "remoteApps" and "apps" to "localApps" and this will be much easier to grok. I don't see any uses of "i" outside resp[i], so: for each (let app in remoteApps) { ... } @@ +212,5 @@ > + let origin = resp[i].origin; > + delete toDelete[origin]; > + > + // If there is a remote app that isn't local or if the remote app was installed later > + if ((!(origin in existingByOrigin)) || existingByOrigin[origin].installTime < resp[i].installTime) { style: don't exceed 80 chars @@ +232,5 @@ > + > + // Update manifests for all the commands (new remote apps) we have so far > + let done = 0; > + let finalCommands = []; > + let toUpdate = commands.length; nit: move declarations until they are actually needed. @@ +234,5 @@ > + let done = 0; > + let finalCommands = []; > + let toUpdate = commands.length; > + > + // Copied from Webapps.js, refactor into common? This violates DRY, so yes @@ +237,5 @@ > + > + // Copied from Webapps.js, refactor into common? > + function checkManifest(aManifest, aInstallOrigin) { > + // TODO : check for install_allowed_from > + if (aManifest.name == undefined) You never need to == undefined. if (!aManifest.name) or if (aManifest.name === undefined) @@ +252,5 @@ > + return true; > + } > + > + function finishedFetching(num) { > + if (num == toUpdate) { Use early return to reduce nesting. if (num != toUpdate) { return; } ... @@ +268,5 @@ > + } > + > + for (let j = 0; j < toUpdate; j++) { > + let app = commands[j]; > + let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest); Why isn't RESTRequest being used? @@ +294,5 @@ > + // Not 200 > + dump("!!! AITC !!! got non-200 while fetching manifest " + xhr.status + "\n"); > + } > + > + // Am I last? Comment in wrong place. @@ +296,5 @@ > + } > + > + // Am I last? > + done += 1; > + finishedFetching(done); I think the name could be better. onManifestProcessed? @@ +305,5 @@ > + finishedFetching(done); > + }, false); > + xhr.send(null); > + } catch (e) { > + dump("!!! AITC !!! Exception while fetching manifest " + e + "\n"); If this ever happens, finishedFetching() will wait for eternity, no? @@ +306,5 @@ > + }, false); > + xhr.send(null); > + } catch (e) { > + dump("!!! AITC !!! Exception while fetching manifest " + e + "\n"); > + } Consider factoring this loop's logic into a standalone function on the prototype. @@ +315,5 @@ > + checkServer: function() { > + let self = this; > + dump('!!! AITC !!! Starting server check\n'); > + this.getApps(function (error, apps) { > + if (apps && !error) { If there is no error, I'd expect the API contract to guarantee that apps conforms to a specific format. Please document the contract and add this logic to getApps(). @@ +320,5 @@ > + dump('!!! AITC !!! got apps ' + apps.length + "\n"); > + self.processResponse(apps, function () { > + dump('!!! saved result\n'); > + }); > + } I count 4 chained functions that process the HTTP response in some way. I think there is room for consolidation. @@ +324,5 @@ > + } > + }); > + }, > + > + runPeriodically: function() { IMO this belongs outside of the "client" type. Maybe toss it in with the AITC service? @@ +336,5 @@ > + notify: function (timer) { > + self.checkServer(); > + } > + }; > + this._timer.initWithCallback(event, FREQ, Ci.nsITimer.TYPE_REPEATING_SLACK); CommonUtils.namedTimer() @@ +345,5 @@ > + this._timer.cancel(); > + this._timer = null; > + } > + > + /* Unused methods, will be utilized in subsequent patches... FYI I won't r+ patches with large chunks of commented code like this. Either remove the code completely or uncomment. However, if uncommented, I will insist on test coverage. I only glanced at code below this because I don't want to waste time on something that may get cut. Please let me know if I should examine it closer. @@ +393,5 @@ > + }); > + }, > + > + _validUUID: function(uuid) { > + return uuid.search(/^[a-zA-Z0-9_-]+$/) == 0; This will yield false positives. ::: services/aitc/modules/main.js @@ +37,5 @@ > +const EXPORTED_SYMBOLS = ["Aitc"]; > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components; > + > +// Switch to https://dev-token.services.mozilla.com when it is safe > +// Make into prefs? Yes! @@ +59,5 @@ > +} > +AitcSvc.prototype = { > + // Obtain a token from Sagrada token server > + getToken: function(assertion, cb) { > + let url = TOKEN_SERVER + "/1.0/aitc/1.0"; Will this come from a discovery service eventually? @@ +66,5 @@ > + client.getTokenFromBrowserIDAssertion(url, assertion, function(err, tok) { > + if (!err) { > + dump("!!! AITC !!! Got token " + JSON.stringify(tok) + "\n"); > + cb(tok); > + } else { Generally, errors are handled first. But, since the success branch is so small, I think it is OK to leave it first. You should still prefer early return, though: if (!err) { cb(tok); return; } // Handle error. @@ +74,5 @@ > + } > + if (!err.response.success) { > + dump("!!! AITC !!! Non-200 while fetching token " + err.response.status + " :: " + err.response.body + "\n"); > + return; > + } I'm not sure all of this logic is necessary. TokenServerClientError instances (which err is guaranteed to be) do a good job of representing things in .message. @@ +79,5 @@ > + dump("!!! AITC !!! Got error " + err + "\n"); > + } > + }); > + }, > + I stopped reviewing here after I saw anant's comment that a new patch version is ready.

This patch addresses several comments previously raised, and also assumes the availability of the patches in bugs 745424, 745396, 745425, 745345 and 745069. This one is more suitable and ready for proper review, the previous one was posted mainly for early feedback.

Comment on attachment 615019 [details] [diff] [review] AITC Desktop Client v2 Review of attachment 615019 [details] [diff] [review]: ----------------------------------------------------------------- There were a lot of items from the previous review that went uncorrected. Please fix as much as you can before the next patch. AitcClient is doing a lot of different things: * Defining the HTTP client interacting with the server * Defining polling semantics for querying the server * Applying changes to and from the server In many cases, this logic is all conflated. I think we'd be in a happier place if things were more loosely coupled. I'm not sure if we should go with 2 or 3 components. But, I think a good start would be to isolate the HTTP client interaction bits from all the other logic. i.e. make an AITCClient type that simply does HTTP. Then, build an AITCAppsManager (or something) with all the logic for applying server records, etc. Maybe move the polling/timer stuff to AITCSvc? There are a number of options here. The current layout will work and I would probably r+ it in a pinch. But, just beware that it will likely lead to confusing and bug-prone code as it evolves. With that in mind, I don't currently like how the state of outstanding requests is managed. AitcClient._getApps raises an error if there is an in-progress request, which is good. However, you are relying on this behavior because the callers don't check for in-progress requests before initiating them. The event loop should not spin between when you check whether you can perform a request and when you actually request it to start. I would definitely add some checking earlier in the call chain so you don't get all these error messages. If I saw all those errors, I would think there was a bug somewhere! Anyway, I cut the review short at AitcClient._gotApps(). I think there is plenty to work on before the next review. ::: services/aitc/modules/client.js @@ +4,5 @@ > + > +"use strict"; > + > +const GET_FREQ = 10000; > +const PUT_FREQ = 20000; 1) Would there be any benefit to these being prefs? 2) Please put file-local constants after the "boilerplate" file headers (after Cu.import, before local definitions) @@ +13,5 @@ > +Cu.import("resource://services-common/rest.js"); > +Cu.import("resource://services-common/utils.js"); > +Cu.import("resource://services-common/log4moz.js"); > +Cu.import("resource://services-common/preferences.js"); > +Cu.import("resource://gre/modules/Webapps.jsm"); Put non-services imports before services imports. @@ +16,5 @@ > +Cu.import("resource://services-common/preferences.js"); > +Cu.import("resource://gre/modules/Webapps.jsm"); > + > +function AitcClient(token, registry) { > + this.uri = token.endpoint.replace(/\/+$/, ''); Nit: "" not '' @@ +17,5 @@ > +Cu.import("resource://gre/modules/Webapps.jsm"); > + > +function AitcClient(token, registry) { > + this.uri = token.endpoint.replace(/\/+$/, ''); > + this.token = { id: token.id, key: token.key }; Nit: cuddle braces @@ +20,5 @@ > + this.uri = token.endpoint.replace(/\/+$/, ''); > + this.token = { id: token.id, key: token.key }; > + > + this.registry = registry || DOMApplicationRegistry; > + this._log = Log4Moz.repository.getLogger("AitC.Client"); Loggers are prefixed with their location in the module tree. Here, we should go with Services.AITC.Client. @@ +40,5 @@ > + * Poll the AITC server for any changes and process them. Call this whenever > + * the user is actively looking at the apps dashboard. It is safe to call > + * this function multiple times. > + */ > + runPeriodically: function() { runPeriodicaly: function runPeriodically() { @@ +46,5 @@ > + return; > + } > + > + // Do one GET check right now, and then once every FREQ seconds > + // TODO: Honor backoff values when we poll This is absolutely required for operational reasons before this ships. If we don't have this, we lose our primary unintentional DDoS mitigation. If you absolutely need to get this checked in to foster more widespread testing, please file a follow-up bug which blocks the release/enabling of the feature. @@ +48,5 @@ > + > + // Do one GET check right now, and then once every FREQ seconds > + // TODO: Honor backoff values when we poll > + this._checkServer(); > + CommonUtils.namedTimer(this._checkServer, GET_FREQ, this, "_timer"); Doh! I was wrong in my initial review when I told you to use namedTimer here. namedTimer is a one shot timer and you really want a repeating one. I've filed bug 745885. @@ +56,5 @@ > + * Stop polling for changes. Call this as soon as the user > + * isn't looking at the apps dashboard anymore. It is safe to call > + * this function even if runPeriodically() wasn't called before. > + */ > + stop: function() { Please rename to something less generic. stopServerPolling? Perhaps also rename runPeriodically to match. I like having matching start-stop pairs: it adds a certain symmetry to things and makes the code more readable IMO. @@ +61,5 @@ > + if (!this._timer) { > + return; > + } > + this._timer.cancel(); > + this._timer = null; The clearing part is actually done as part of cancel()! @@ +66,5 @@ > + }, > + > + /** > + * Initiates an update of a newly installed to the AITC server. Call this > + * when an application is installed locally. newly installed app? @@ +97,5 @@ > + receipts: app.receipts, > + manifestPath: app.manifestURL, > + installOrigin: app.installOrigin > + }; > + if ('modifiedAt' in app) { Use double quotes. @@ +100,5 @@ > + }; > + if ('modifiedAt' in app) { > + record.modifiedAt = app.modifiedAt; > + } > + if ('installedAt' in app) { ditto @@ +110,5 @@ > + _checkServer: function _checkServer() { > + this._log.info("Starting scheduled server check"); > + > + let self = this; > + this._getApps(function (error, remoteApps) { name function @@ +114,5 @@ > + this._getApps(function (error, remoteApps) { > + if (remoteApps && !error) { > + // _gotApps checks for the validity of remoteApps. > + self._log.info("Server check got " + remoteApps.length + "apps"); > + DOMApplicationRegistry.getAllWithoutManifests(function(localApps) { name function @@ +115,5 @@ > + if (remoteApps && !error) { > + // _gotApps checks for the validity of remoteApps. > + self._log.info("Server check got " + remoteApps.length + "apps"); > + DOMApplicationRegistry.getAllWithoutManifests(function(localApps) { > + self._gotApps(remoteApps, localApps, function() { name function @@ +116,5 @@ > + // _gotApps checks for the validity of remoteApps. > + self._log.info("Server check got " + remoteApps.length + "apps"); > + DOMApplicationRegistry.getAllWithoutManifests(function(localApps) { > + self._gotApps(remoteApps, localApps, function() { > + self._log.info("processResponse completed and saved result"); Holy function nesting, Batman! My spider sense tells me that at least the cb for _getApps() should be pulled out into a prototype-level function. @@ +121,5 @@ > + }); > + }); > + } else { > + self._log.error("_getApps failed, will retry on next _checkServer"); > + } Refactor function to use early return. Also, please differentiate between an error condition and a successful but empty server response (e.g. 304 responses). if (error) { self._log.error("..."); return; } else if (!remoteApps) { self._log.info("No new apps..."); return; } DOMApplicationRegistry... @@ +132,5 @@ > + }, > + > + _makeRequest: function _makeRequest(uri) { > + if (typeof uri != "string") { > + throw new Error("Bad request URI: " + uri); But RESTRequest also handles nsIURI! And, if it isn't an nsIURI, it tries to make one. This will fail if not the right type, so this check is not needed. That means the function is one line. That means it can be inlined. @@ +138,5 @@ > + return new AuthRESTRequest(uri, this.token); > + }, > + > + _makeAppURI: function _makeAppURI(origin) { > + let part = btoa(Utils._sha1(origin)).replace(/\+/, '-').replace(/\//, '_').replace(/=/, ''); Referencing something in another module whose name begins with _ is a giant red flag. I agree that it is silly for sha1() to hex encode (it should return raw bytes). The proper solution here is to create a CryptoUtils.sha1Base64() and/or CryptoUtils.sha1Base64URLFriendly(). Also, regular expressions here are completely overkill. Use e.g. .replace("+", "-", "g") Nit: Use double quotes. @@ +151,5 @@ > + // If there's an unfinished PUT or a PUT that didn't succeed, just bail > + if (this._putsInProgress.length) { > + cb(new Error("PUT(s) in progress, aborting GET!")); > + this._log.warn("getApps called, but aborting due to outstanding PUT(s)!"); > + return; Log before calling callbacks because a) the callback could do a lot of work and logging and log order would be weird b) callback could throw and the log entry would never get dropped. Also, I'm not sure calling a callback is warranted here. If the function is a noop that simply raises an error, raise the error. There is nothing async requiring a callback to be used. @@ +176,5 @@ > + req.response.headers["Content-Type"] == "application/json") { > + try { > + let tmp = JSON.parse(req.response.body); > + cb(null, tmp["apps"]); > + self.appsLastModified = parseInt(req.response.headers['x-timestamp']); Please add a comment stating that appsLastModified is intentionally not updated until after processing so failures can be tried again and not skipped over. This also raises the question of what happens if cb() always raises with the same input. I guess we get stuck in an endless loop since the same record will always be downloaded since we don't advance appsLastModified? @@ +185,5 @@ > + } > + } else { > + cb(new Error("Unexpected error with getApps"), null); > + self._error(req); > + } Refactor to use early return. @@ +190,5 @@ > + }); > + }, > + > + _putApp: function _putApp(appRec, appLastModified) { > + // Add to queue and process it Add full stop. @@ +199,5 @@ > + this._processPutQueue(); > + }, > + > + _processPutQueue: function _processPutQueue() { > + if (!this._processPutQueue.length) { I assume you meant this._putsInProgress? @@ +215,5 @@ > + // again before the previous one finished? > + CommonUtils.namedTimer( > + this._processPutQueue, PUT_FREQ, this, "_putTimer" > + ); > + } Can you explain what you are trying to accomplish with the timer? I have an idea, but I want to make sure we are on the same page. @@ +223,5 @@ > + for (let i = 0; i < this._putsInProgress.length; i++) { > + let app = this._putsInProgress[i]; > + this._tryPuttingApp(app, function(err, success) { > + // Remove from queue if successful, or err.removeFromQueue is true > + if (success === true || (err && err.removeFromQueue)) { Do you really care about type equality? Probably not. @@ +224,5 @@ > + let app = this._putsInProgress[i]; > + this._tryPuttingApp(app, function(err, success) { > + // Remove from queue if successful, or err.removeFromQueue is true > + if (success === true || (err && err.removeFromQueue)) { > + self._putsInProgress.splice(i, 1); This is wrong. splice modifies the array in place, effectively shifting everything down and reordering the indexes. Only the first call of splice here is guaranteed to have the intended effect. @@ +226,5 @@ > + // Remove from queue if successful, or err.removeFromQueue is true > + if (success === true || (err && err.removeFromQueue)) { > + self._putsInProgress.splice(i, 1); > + } > + }); So, what happens when this._putsInProgress.length > 0 and _putApp() is called? If the callback to _tryPuttingApp() has not fired yet, you'll call _tryPuttingApp() again for the entries that were in _putsInProgress before _putApp was called! @@ +237,5 @@ > + if (app.modified) { > + req.setHeader("X-If-Unmodified-Since", app.modified); > + } > + > + req.put(JSON.stringify(appRec), function(error) { name function @@ +255,5 @@ > + case 400: > + case 412: > + case 413: > + self._log.warn("_tryPuttingApp returned: " + req.response.status); > + let err = new Error(); Populate Error. @@ +262,5 @@ > + break; > + > + default: > + cb(new Error("Unexpected error with _tryPuttingApp"), null); > + self._error(req); Log before calling callback. @@ +285,5 @@ > + let manifest = JSON.parse(req.response.body); > + if (!manifest.name) { > + callback(new Error("Invalid manifest fetched", null)); > + return; > + } Should you log here? It might be useful to capture the HTTP body for debugging. @@ +289,5 @@ > + } > + // Success > + callback(null, manifest); > + } catch (e) { > + callback(new Error(e), null); What if the callback threw inside the try? You could get a double call of the callback! @@ +296,5 @@ > + }, > + > + _gotApps: function _gotApps(remoteApps, localApps, callback) { > + let commands = []; > + let toDelete = originToId = existingByOrigin = {}; Objects are assigned by reference, not value. @@ +297,5 @@ > + > + _gotApps: function _gotApps(remoteApps, localApps, callback) { > + let commands = []; > + let toDelete = originToId = existingByOrigin = {}; > + I kinda stopped reviewing at this point. I'd like to see the next iteration of the patch before I go on. @@ +379,5 @@ > + app.value.manifest = manifest; > + finalCommands.push({id: app.id, value: app.value}); > + self._log.info(app.id + " was added to finalCommands"); > + } else { > + self._log.debug("Couldn't fetch manifest at " + url + ": " + err); You might want to log at a higher level. @@ +390,5 @@ > + }); > + } > + } > + > + /* Unused methods, will be utilized in subsequent patches... Do you intend to enable this in a subsequent patch? If not, please remove it. ::: services/aitc/services-aitc.js @@ +1,1 @@ > +pref("dom.mozApps.whitelist", "https://myapps.mozillalabs.com"); Is there a better place to define this? services/ seems like the wrong place for dom branch prefs. @@ +1,3 @@ > +pref("dom.mozApps.whitelist", "https://myapps.mozillalabs.com"); > + > +pref("services.aitc.dashboard", "https://myapps.mozillalabs.com"); either .dashboardURL or .dashboard.url. You never know when you'll need to provide more prefs related to dashboard. @@ +2,5 @@ > + > +pref("services.aitc.dashboard", "https://myapps.mozillalabs.com"); > +pref("services.aitc.marketplace", "https://marketplace.mozilla.org"); > +// Switch to https://dev-token.services.mozilla.com when it is safe > +pref("services.aitc.tokenServer", "http://token2.reg.mtv1.dev.svc.mozilla.com"); ditto ::: services/aitc/tests/unit/xpcshell.ini @@ +1,2 @@ > +[DEFAULT] > +head = You'll need to load some files from services-common. See the xpcshell.ini files in the services tree to see how relative imports work.

Is there a description somewhere that explains why this patch behaves the way it does? I don't understand why apps-in-the-cloud behavior needs to be tied to tracking whether the Apps Marketplace is currently being displayed in Firefox, and polling a server continuously while that's true, and I don't see any relevant explanations in this bug.

Jen - Could you clarify? Gavin's point is valid - This does seem a bit odd.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20) > Is there a description somewhere that explains why this patch behaves the > way it does? I don't understand why apps-in-the-cloud behavior needs to be > tied to tracking whether the Apps Marketplace is currently being displayed > in Firefox, and polling a server continuously while that's true, and I don't > see any relevant explanations in this bug. It's not tracking the Apps Marketplace (marketplace.mozilla.org), it's tracking the Dashboard (myapps.mozillalabs.com). They are different things, the dashboard is intended to be the user's view into all their apps that are in the "cloud". The dashboard is a remote page (and not, for example, something like about:apps) is to allow a common dashboard to exist between Firefox and other browsers. As noted earlier in the bug, myapps.mozillalabs.com is just a working name, this will likely be moved to persona.org/apps, so the user will always know to go to that URL -- irrespective of which browser they are using -- to see what apps they have installed across all their devices. The rationale behind the polling is that we only want to do it when the user is actively looking at the dashboard. If an app is installed elsewhere, and the user is currently on the dashboard, they would expect that the app appears fairly quickly. Note that the polling is only for GETs (i.e. new apps installed elsewhere, if any, will be pulled down) and will follow backoff headers as sent by the AITC server. PUT requests are made instantaneously in the background (provided the user is signed in of course) on receipt of an "app-installed" event.

I will also pre-emptively answer another question that I anticipate -- why is the URL marketplace.mozilla.org in the code? :) The only way in which the AITC client is tied to the Mozilla Marketplace is via BrowserID. It is an optimization path to prevent the user from authenticating twice, once in the Marketplace and then again to the AITC server. Since Marketplace uses BrowserID, and we trust it, we will try to obtain the email that was used to sign in to the marketplace (which you were required to do as part of the install process), and reuse that email to identify you to the AITC server. That is to say, if I install an app via the Mozilla Marketplace as anant@mozilla.com, that app will be pushed to an AITC account identified as anant@mozilla.com; without requiring me to login again. This, of course, means that if for some reason we aren't able to find out what email you used to login to the marketplace (happens between browser restarts, for example); or, if you install an app from some other marketplace, or from a developer who is self-publishing his app - you will be required to sign in to the AITC server. This login prompt is shown the next time you visit the dashboard. It's all a bit tricky, hope that clarifies the logic to some extent!

(In reply to Anant Narayanan [:anant] from comment #23) > That is to say, if I install an app via the Mozilla Marketplace as > anant@mozilla.com, that app will be pushed to an AITC account identified as > anant@mozilla.com; without requiring me to login again. We could also extend this optimization to other marketplaces which use BrowserID (I should really be calling it Persona, by now). We can add a preference so the user can add to this list manually, in addition to maintaining our own set of app marketplaces that support BrowserID. The Mozilla Marketplace is the only marketplace that is compatible with our API, so we're starting there.

(In reply to Anant Narayanan [:anant] from comment #23) > I will also pre-emptively answer another question that I anticipate -- why > is the URL marketplace.mozilla.org in the code? :) > > The only way in which the AITC client is tied to the Mozilla Marketplace is > via BrowserID. It is an optimization path to prevent the user from > authenticating twice, once in the Marketplace and then again to the AITC > server. Since Marketplace uses BrowserID, and we trust it, we will try to > obtain the email that was used to sign in to the marketplace (which you were > required to do as part of the install process), and reuse that email to > identify you to the AITC server. > > That is to say, if I install an app via the Mozilla Marketplace as > anant@mozilla.com, that app will be pushed to an AITC account identified as > anant@mozilla.com; without requiring me to login again. > > This, of course, means that if for some reason we aren't able to find out > what email you used to login to the marketplace (happens between browser > restarts, for example); or, if you install an app from some other > marketplace, or from a developer who is self-publishing his app - you will > be required to sign in to the AITC server. This login prompt is shown the > next time you visit the dashboard. > > It's all a bit tricky, hope that clarifies the logic to some extent! A couple of questions/comments: 1. Is it possible to generalize the logic here to where if a user logs into browser ID anywhere (e.g. The times crossword site, mozillians), then authentication with the AITC server is done there and once? 2. Is it okay to rely on myapps.mozillalabs.com as the centralized point right now? There's a lot of flux going on what to do with "web dashboards" for the apps project. Random question - Why couldn't we just do the integration something like this? 1. Login into browser ID anywhere 2. Authenticate against AITC server 3. Pull apps from AITC server 4. Install those applications natively Or would something like that be something different that is future down the line? (In reply to Anant Narayanan [:anant] from comment #24) > (In reply to Anant Narayanan [:anant] from comment #23) > > That is to say, if I install an app via the Mozilla Marketplace as > > anant@mozilla.com, that app will be pushed to an AITC account identified as > > anant@mozilla.com; without requiring me to login again. > > We could also extend this optimization to other marketplaces which use > BrowserID (I should really be calling it Persona, by now). We can add a > preference so the user can add to this list manually, in addition to > maintaining our own set of app marketplaces that support BrowserID. Something in about:config (like the whitelisting about:config) would be good to include then. > > The Mozilla Marketplace is the only marketplace that is compatible with our > API, so we're starting there. Isn't it possible that apps could be installed elsewhere outside of Mozilla Marketplace? For example, I believe users are able to host their own apps. For instance, how would this use case be handled with Bill Walker's app: 1. Go to birdwalker.com 2. Select install application What's next after this?

(In reply to Jason Smith from comment #25) > 1. Is it possible to generalize the logic here to where if a user logs into > browser ID anywhere (e.g. The times crossword site, mozillians), then > authentication with the AITC server is done there and once? No. Just because I logged into the times site with a particular persona doesn't mean I want all my apps to be associated with that email. However, if I use a particular email to login to an *app marketplace*, it is reasonable to assume that I would like that app synced to the same account I just logged in with. > 2. Is it okay to rely on myapps.mozillalabs.com as the centralized point > right now? There's a lot of flux going on what to do with "web dashboards" > for the apps project. Random question - Why couldn't we just do the > integration something like this? The phrase "Apps in the Cloud" implies that there is central location that users can visit to get to their apps, by using BrowserID to sign in. We will host one such dashboard (I don't think it should be myapps.mozillalabs.com, persona.org/apps is a *much* better place). This is not to say ours will be the only dashboard. All APIs are public and documented, users are free to choose any AITC service that complies with these APIs. We build the system to support this right from the start. > 1. Login into browser ID anywhere > 2. Authenticate against AITC server > 3. Pull apps from AITC server > 4. Install those applications natively No, we can't do this for reasons mentioned above. It breaks user expectations when they use different Personas to install different sets of apps and they all merge into one account. > > We could also extend this optimization to other marketplaces which use > > BrowserID (I should really be calling it Persona, by now). We can add a > > preference so the user can add to this list manually, in addition to > > maintaining our own set of app marketplaces that support BrowserID. > > Something in about:config (like the whitelisting about:config) would be good > to include then. Yes, that's what I meant by "preference". about:config is one frontend for setting them, but we can always build our own if we want the UI to be a bit more user-friendly :) > Isn't it possible that apps could be installed elsewhere outside of Mozilla > Marketplace? For example, I believe users are able to host their own apps. > For instance, how would this use case be handled with Bill Walker's app: > > 1. Go to birdwalker.com > 2. Select install application > > What's next after this? Please see my earlier comment: (Anant Narayanan [:anant] from comment #23) > This, of course, means that if for some reason we aren't able to find out > what email you used to login to the marketplace (happens between browser > restarts, for example); or, if you install an app from some other > marketplace, or from a developer who is self-publishing his app - you will > be required to sign in to the AITC server. This login prompt is shown the > next time you visit the dashboard.

(In reply to Jason Smith from comment #25) > 1. Login into browser ID anywhere > 2. Authenticate against AITC server > 3. Pull apps from AITC server > 4. Install those applications natively I will hasten to add that this proposal is only unimplementable in the short-term. I think it will make more sense when we implement the "sign in to the browser" feature. When the user signs in to the browser on startup, we can safely assume that all actions taken in that session are to be tied to that Persona, including app installs. However, we're not going to see this sign-in feature for the next 3 months (at the least), so we're building what makes sense in Firefox today. Additionally, what we implement now will also serve as the fallback when the user doesn't "sign in to the browser" when we roll-out that feature, so this code is not going to go away.

(In reply to Anant Narayanan [:anant] from comment #26) > > 2. Is it okay to rely on myapps.mozillalabs.com as the centralized point > > right now? There's a lot of flux going on what to do with "web dashboards" > > for the apps project. Random question - Why couldn't we just do the > > integration something like this? > > The phrase "Apps in the Cloud" implies that there is central location that > users can visit to get to their apps, by using BrowserID to sign in. We will > host one such dashboard (I don't think it should be myapps.mozillalabs.com, > persona.org/apps is a *much* better place). > > This is not to say ours will be the only dashboard. All APIs are public and > documented, users are free to choose any AITC service that complies with > these APIs. We build the system to support this right from the start. Will we need to update MDN for this particular feature then, since these are public consumable APIs? If so, we should flag this with the "dev-doc-needed" keyword. > > > 1. Login into browser ID anywhere > > 2. Authenticate against AITC server > > 3. Pull apps from AITC server > > 4. Install those applications natively > > No, we can't do this for reasons mentioned above. It breaks user > expectations when they use different Personas to install different sets of > apps and they all merge into one account. Okay. But what about the native piece? Why is the apps in the cloud piece happening on a dashboard, rather than doing the direct integration into the native system? Or would that be a feature down the road. > > > > We could also extend this optimization to other marketplaces which use > > > BrowserID (I should really be calling it Persona, by now). We can add a > > > preference so the user can add to this list manually, in addition to > > > maintaining our own set of app marketplaces that support BrowserID. > > > > Something in about:config (like the whitelisting about:config) would be good > > to include then. > > Yes, that's what I meant by "preference". about:config is one frontend for > setting them, but we can always build our own if we want the UI to be a bit > more user-friendly :) > We should be careful though to watch out for what happened with the mozapps management API whitelisting approach (myapps isn't whitelisted right now, which pretty much makes it unable to be used unless you set the config explicitly). What was proposed for a "user friendly" approach was filed as a feature request in bug 745046. That's another option to consider, even maybe integrating it with the install door-hanger piece if it was possible: - Install & Sync - Install - Cancel This does make think if there's a place that we should consider integrating in the Firefox UI.

(In reply to Jason Smith from comment #28) > (In reply to Anant Narayanan [:anant] from comment #26) > > This is not to say ours will be the only dashboard. All APIs are public and > > documented, users are free to choose any AITC service that complies with > > these APIs. We build the system to support this right from the start. > > Will we need to update MDN for this particular feature then, since these are > public consumable APIs? If so, we should flag this with the "dev-doc-needed" > keyword. We usually don't document server APIs on MDN. Our Sync service also has such a "public" API (in quotes, because all our code is open source, there really isn't any such as a private API, perhaps only the notion of functions that may break in the future). The AITC APIs are documented in a similar fashion at the same site: http://docs.services.mozilla.com/aitc/index.html > Okay. But what about the native piece? Why is the apps in the cloud piece > happening on a dashboard, rather than doing the direct integration into the > native system? Or would that be a feature down the road. We would definitely like AITC to integrate well with the native apps that we generate, as well as make sure an app is uninstalled if the user removes the native application from their machine (f.e. by dragging to Trash on mac, uninstall wizard on Windows). The technical complexities of implementing this are much larger in scope than only dealing with the dashboard, and so we punted this for future revisions. > We should be careful though to watch out for what happened with the mozapps > management API whitelisting approach (myapps isn't whitelisted right now, > which pretty much makes it unable to be used unless you set the config > explicitly). What was proposed for a "user friendly" approach was filed as a > feature request in bug 745046. That's another option to consider, even maybe > integrating it with the install door-hanger piece if it was possible: The dashboard at myapps will be whitelisted by default when Bug 720415 lands. The feature requested in Bug 745046 is definitely a great way of exposing this preference to the user!

Comment on attachment 615019 [details] [diff] [review] AITC Desktop Client v2 Review of attachment 615019 [details] [diff] [review]: ----------------------------------------------------------------- I focused this review on main.js. Although, there are large chunks of code that I only glanced over. The reason is there is a significant issue I would like addressed before I go further. The implementation of the logic to save apps records to AITC seems very naive. And, from talking to Anant, it sounds like it is this way by design and that the decision was beyond his [as reflected by the code]. I would like to call out why this is wrong and plead that it be changed. As I understand it, AITC is a central, canonical store for app purchases/receipts. It is effectively a cache of all transactions made between the user and the various app marketplaces. While AITC is not technically required, it is a necessary optimization to facilitate rapid and simple access to your apps data. Without the AITC server, clients would need to contact every marketplace and obtain apps info. This necessitates the client knowing what app marketplaces to contact, which comes with its own challenges. Anyway, my concern is over the robustness of the "syncing" of app receipts from marketplaces to AITC. In the current implementation, there are many scenarios in which this would fail: * AITC server is unavailable. No service has 100% uptime. * The network goes offline between when an app is purchased in the marketplace and when AITC is updated. * The device/application is shut down between when an app is purchased and when AITC is updated. The 2nd and 3rd points may sound like edge cases. But, when you consider that the AITC service could be operating slowly - response times in seconds, not milliseconds - or high network latency (mobile networks in developing countries or AT&T in San Francisco) these scenarios are very real. If the record never makes it to AITC, we have a bad user experience. As I understand how things work, the device doing the purchasing will be OK, as the app has been installed locally and this installation will persist across parent application restarts. However, when a new device comes along, AITC won't have a record of the app, so the app won't be present. At that point, the user will scratch her head and be like "wut? where is my app?" Maybe users know they need to go to the marketplace to install the app. When you have one marketplace, that's all fine. But, when you have multiple and you may not remember from which one you purchased an app, that could pose a significant problem. Compounding concern is that some apps cost money. If an app is "lost" from my account, this breeds distrust and resentment of the product. "I *purchased* this app: what do you mean it is gone?!" Putting on my product and UX faux hats, I insist that the propagation of apps from install to AITC be as robust as possible. I'm not sure if this should be a landing blocker or not: that's not my call. That being said, until someone says otherwise, I will enforce the robustness of propagation/syncing through the review of this code. Of course, someone saying otherwise could just be posting a link to the product requirements (which I should probably see anyway so my review can cover compliance with that as well). So, how can we make this more robust? There are many options. And, they vary in robustness and complexity. The option I would most like to see *from a client perspective* is cooperation between the marketplace and AITC at app purchase time. The client would issue a single request to the marketplace to purchase an app. As part of that transaction, the marketplace performs some back-channel communication with AITC (using client-provided credentials) to update AITC's app records. When the marketplace sends a response to the client, the app is purchased and AITC is already updated. No extra work for the client. The benefit here is all the complex logic is offloaded to the server, which can be updated whenever. We don't have this luxury in client code. The problem is a host of new questions. If AITC is down, does the transaction complete? What's the recovery strategy if so? If not? Should marketplaces have access to AITC at all? Do we want a central broker involved in all transactions? How do you technically allow the marketplace to perform an AITC request on behalf of the client [without giving the marketplaces too much data or power]? I'm assuming the above was discussed and dismissed for whatever reasons. If it wasn't discussed, we really need to discuss it, as I believe it is the option that provides the best end-user experience. This is partially due to the fact that complexity is offloaded to the servers, where it is far more likely to be done correctly. Assuming server-side cooperation with AITC is off the table, we'll need to make the client more robust. Here are some of the things we should consider: * Have the client store an intent to install an app immediately before it asks the marketplace to do so. This intent must be persisted across application restarts. The client should periodically check the set of intents against local apps. If there is an intent that doesn't correspond to a known app state, the client should consult the marketplace and ensure the intent was never acted on. If it wasn't you can probably delete the intent (the user will try the purchase again). If it was, you may need to propagate that to AITC. Consider storing these install intents in AITC for durability. * As soon as a marketplace transaction is complete, the client should store an "outbound" record intended for AITC. Like purchase intents, these records must be persisted across application restarts. Otherwise, they may be lost and apps never propagate to AITC. * Persist the set of known marketplaces in the client and/or AITC. Periodically query said marketplaces and reconcile differences with AITC. It's the only way to be sure -- Ripley. There are certainly other options. And, some might be better than what I have suggested. Please suggest some! The central theme is that any activity related to purchases/installs is recorded before it is performed. If something goes wrong with that action, the client can simply try again (if appropriate) until the request has completed successfully. Whatever we end up doing, we'll need test cases that cover the following scenarios: * Client makes install/purchase request to marketplace. Request never makes it to server. (This is probably handled elsewhere, before the scope of this patch.) * Client makes install/purchase request to marketplace. Request completes successfully on server. Response is lost in transmission. Server has recorded purchase. Client hasn't. How is this reconciled? (This might be solved outside the context of this patch.) * Client makes install/purchase and sees successful response. Error during app install. User has paid money for the transaction but has nothing to show for it. What should the state of AITC be? (Current patch waits for install notification, so AITC does *not* get updated.) * Client installs/purchases and successfully installs the app. AITC is down. Application is restarted. When/how does AITC get updated? * Client successfully installs the app. Request issued to AITC. Request lost in transmission. * Client successfully installs the app. Request to AITC is processed successfully but response is lost in transmission. In summary, there's a lot of work to be done around robustness. Because money is involved and I don't want to be the reviewer responsible for people seemingly losing their money, I'm going to insist on robustness for this patch. ::: services/aitc/modules/main.js @@ +25,5 @@ > + )]; > + this._log.info("Loading AitC"); > +} > +AitcSvc.prototype = { > + get DASHBOARD() { I know it is effectively a constant, but please lower-case properties. Also, I think dashboardURL, marketplaceURL, etc are more appropriate since that is what they actually are. @@ +40,5 @@ > + > + // Obtain a token from Sagrada token server, given a BrowserID assertion > + getToken: function getToken(assertion, cb) { > + let self = this; > + let url = TOKEN_SERVER + "/1.0/aitc/1.0"; TOKEN_SERVER is not a valid symbol here. You mean this.TOKEN_SERVER (or this.tokenServerURL once pref names are changed). In addition, the idea behind token server URLs is that the client shouldn't have to construct them. The entire URL should come from some external entity. See the documentation in services/common/tokenserverclient.js. In the context of Mozilla, token URLs should come from a service discovery service (not yet implemented) or be hard-coded. So, you should hard-code the full token URL as a preference. So, you should end up with this.tokenURL, which is a getter or instance variable that resolves to https://token-foo.mozilla.com/1.0/aitc/1.0 @@ +43,5 @@ > + let self = this; > + let url = TOKEN_SERVER + "/1.0/aitc/1.0"; > + let client = new TokenServerClient(); > + > + client.getTokenFromBrowserIDAssertion(url, assertion, function(err, tok) { Name all functions. @@ +45,5 @@ > + let client = new TokenServerClient(); > + > + client.getTokenFromBrowserIDAssertion(url, assertion, function(err, tok) { > + if (!err) { > + self._log.info("Got token from server: " + JSON.stringify(tok)); 1) I think printing the full token is overkill. Just log that you got a new token. 2) This is a potential security vulnerability. The contents of the token are your service credentials. Anybody who can read the log (which could be other services on your device) could have access to your token and use it to access your data. Some say that we can plug this vulnerability by limiting who has access to the logs. That is correct. But, that is a problem I would like to avoid. And, we avoid it by not logging overly sensitive information. @@ +46,5 @@ > + > + client.getTokenFromBrowserIDAssertion(url, assertion, function(err, tok) { > + if (!err) { > + self._log.info("Got token from server: " + JSON.stringify(tok)); > + cb(tok); No error argument to callback? Use (error, result) for callback signatures. @@ +52,5 @@ > + } > + > + if (!err.response) { > + self._log.error("Error while fetching token " + err.message); > + return; So the original callback never gets called? How do you know an error occurred? @@ +56,5 @@ > + return; > + } > + if (!err.response.success) { > + self._log.error("Error while fetching token (non-200) " + err.message); > + return; Ditto. @@ +58,5 @@ > + if (!err.response.success) { > + self._log.error("Error while fetching token (non-200) " + err.message); > + return; > + } > + self._log.error("Unknown error while fetching token " + err.message); Ditto. @@ +95,5 @@ > + }); > + } > + } > + > + BrowserID.isLoggedIn(function(err, yes) { Name all functions. @@ +166,5 @@ > + onLocationChange: function onLocationChange(browser, progress, req, location, flags) { > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + if (win.gBrowser.selectedBrowser == browser) { > + let uri = location.spec.substring(0, DASHBOARD.length); > + if (uri == DASHBOARD) { DASHBOARD is not a valid symbol. this.DASHBOARD (or this.dashboardURL when the properties are renamed). @@ +185,5 @@ > + } > + > + // Add listeners for all windows opened in the future > + function winWatcher(subject, topic) { > + if (topic != "domwindowopened") return; Always use { }. @@ +212,5 @@ > + ); > + if (uri == DASHBOARD) { > + dashboardLoaded(browser); > + } > + } I'm going to defer review for the window/tab/URL watching bits. I just don't know the subject well enough. @@ +216,5 @@ > + } > + > + // Add listeners for app installs/uninstall > + Svc.Obs.add("webapps-sync-install", this); > + Svc.Obs.add("webapps-sync-uninstall", this); Where does Svc come from? I think you copied this pattern from Sync code, where Svc is exported by util.js and Svc.Obs maps to Observers, which is exported from resource://services-common/observers.js.

I should point out that the app purchase model -- that is, make a purchase from a marketplace and update our own remote receipt store -- is a distributed transaction with some simplifying features (no conflicts in AITC, for example). What gps described are a few methods for dealing with this kind of simple distributed transaction: * Pre-handshake with recovery * Persistent queuing of writes with repeated retries * Eventual consistency through reconciling. If the current approach includes none of these, then it's using what I would tend to call the "ostrich strategy", which does not work well at scale.

Greg, thanks for your analysis - comments inline: (In reply to Gregory Szorc [:gps] from comment #30) > So, how can we make this more robust? There are many options. And, they vary > in robustness and complexity. > > The option I would most like to see *from a client perspective* is > cooperation between the marketplace and AITC at app purchase time. The > client would issue a single request to the marketplace to purchase an app. > As part of that transaction, the marketplace performs some back-channel > communication with AITC (using client-provided credentials) to update AITC's > app records. When the marketplace sends a response to the client, the app is > purchased and AITC is already updated. No extra work for the client. The > benefit here is all the complex logic is offloaded to the server, which can > be updated whenever. We don't have this luxury in client code. The problem > is a host of new questions. If AITC is down, does the transaction complete? > What's the recovery strategy if so? If not? Should marketplaces have access > to AITC at all? Do we want a central broker involved in all transactions? > How do you technically allow the marketplace to perform an AITC request on > behalf of the client [without giving the marketplaces too much data or > power]? > > I'm assuming the above was discussed and dismissed for whatever reasons. If > it wasn't discussed, we really need to discuss it, as I believe it is the > option that provides the best end-user experience. This is partially due to > the fact that complexity is offloaded to the servers, where it is far more > likely to be done correctly. Server side co-operation will require marketplaces to adopt BrowserID, or request the user for an additional BrowserID login in addition to their existing login system. This is much easier to do in a single marketplace (that's how Google and Apple do it after all, and ours can too), but the fact that we want to encourage multiple marketplaces will hinder us. We could enforce BrowserID as the only authentication mechanism for Marketplaces that want to be compatible with our Apps implementation (there's nothing Mozilla-specific about BrowserID after all), but my gut feeling is that it might be too early to force that and that it might hurt the growth of marketplaces beyond our own. > * Have the client store an intent to install an app immediately before it > asks the marketplace to do so. This intent must be persisted across > application restarts. The client should periodically check the set of > intents against local apps. If there is an intent that doesn't correspond to > a known app state, the client should consult the marketplace and ensure the > intent was never acted on. If it wasn't you can probably delete the intent > (the user will try the purchase again). If it was, you may need to propagate > that to AITC. Consider storing these install intents in AITC for durability. How does storing intents locally help with cases (1), (2) and (3) you describe above? I don't see how storing an intent is different from storing the app record itself (let's assume for now that records will persist across restarts, it doesn't now because of an implementation oversight by me, it's not a "feature" of the design in general). Additionally, this requires the Marketplace to expose an additional API (check whether or not an app was installed by a user). Making things as easy as possible for marketplaces has been one of our goals. > * As soon as a marketplace transaction is complete, the client should store > an "outbound" record intended for AITC. Like purchase intents, these records > must be persisted across application restarts. Otherwise, they may be lost > and apps never propagate to AITC. Why store anything other than the application record itself? Again, I don't see how this mitigates any of the problems you mentioned earlier. Any data, whether it be an intent, an outbound record, or the app record itself; are all subject to the same fundamental problem of the AITC server being unavailable when the install happens. I think it's much better for us to limit that to just one piece of data (which I suggest be the the app record, which is near-instantaneously generated after an app install succeeds). We should, of course, persist this across application restarts. > Whatever we end up doing, we'll need test cases that cover the following > scenarios: > > * Client makes install/purchase request to marketplace. Request never makes > it to server. (This is probably handled elsewhere, before the scope of this > patch.) > > * Client makes install/purchase request to marketplace. Request completes > successfully on server. Response is lost in transmission. Server has > recorded purchase. Client hasn't. How is this reconciled? (This might be > solved outside the context of this patch.) I don't think these test cases belong in the AITC code (as you point out), but I also think that these tests cases are far too general. This answers to these are not any different from how *any* service handles transactions HTTP. You click the "Pay Now" button in your bank's website, the record is registered on the server, response is lost - user's know they must refresh, and only if the transaction didn't succeed should they retry again. I agree that Marketplaces should make this amply clear. In either case, as far as AITC is concerned, the install never happened. There is no reconciliation to do here. The user knows that the app did not get installed, so they have no expectation that it will appear on the dashboard. (If not, we should make it clear to them that this is the case). > * Client makes install/purchase and sees successful response. Error during > app install. User has paid money for the transaction but has nothing to show > for it. What should the state of AITC be? (Current patch waits for install > notification, so AITC does *not* get updated.) This is a tricky one, and one we've discussed before. The conclusion we arrived at was that the user should now be directed to receive a refund from the Marketplace, if this continues to happen after a few retries of the install. In our own Marketplace, refunds are instant if they are requested within 10 minutes of the purchase. As far as AITC is concerned, an installation never happened. Note that it is most likely that the user will try to launch an application immediately after they buy it, but in this case they won't even get that far because the install dialog will make it clear to them that the install didn't succeed. > * Client installs/purchases and successfully installs the app. AITC is down. > Application is restarted. When/how does AITC get updated? I think we should give a lot more thought into this particular case. When we originally designed the server to be authoritative, it was my deduction that installs should fail if they were unable to be written to the AITC server. And indeed, this is how the original client was written. However, at a product meeting last week, we were told that installs shouldn't fail if AITC goes down. Perhaps I didn't make a case for that forcefully enough at the meeting. Explaining the ramifications of this (as you have), especially in the case of paid apps might give us a stronger case for installs to fail. On the other hand, that is will bring up uptime requirements for AITC *dramatically*. If we go this approach, all app installs across all marketplaces for all Firefox users will fail when AITC goes down. FWIW, this is how Google's marketplace and Apple's marketplace function. But they have the big advantage of the marketplace and app storage server be effectively one and the same to the user, a luxury we don't have. > * Client successfully installs the app. Request issued to AITC. Request lost > in transmission. Yes, we should check that the client retries until it gets a response from the server. > * Client successfully installs the app. Request to AITC is processed > successfully but response is lost in transmission. Agreed, luckily for us PUTs are idempotent so this will be easy. > In summary, there's a lot of work to be done around robustness. Because > money is involved and I don't want to be the reviewer responsible for people > seemingly losing their money, I'm going to insist on robustness for this > patch. I agree with your sentiment, robustness is very important and everyone involved in the AITC project from the start has felt this way. The diverging opinions are around where we should build this robustness. We're currently trying to make the AITC server be the pillar of the system and are avoiding the urge to move a lot of logic into the client (though it should certainly be doing a few things it doesn't do now, like persisting outgoing records through restarts). For a typical user, the number of PUTs are going to be so abysmally low, that I do not think it is unreasonable to have the AITC server be the thing that provides robustness to the system (See https://github.com/anantn/aitc/wiki/Scaling-to-1M for some estimates of load for 1M users).

(In reply to Anant Narayanan [:anant] from comment #32) > For a typical user, the number of PUTs are going to be so abysmally low, > that I do not think it is unreasonable to have the AITC server be the thing > that provides robustness to the system (See > https://github.com/anantn/aitc/wiki/Scaling-to-1M for some estimates of load > for 1M users). We're working like crazy to make the AITC servers as robust as humanly possible. It's not going to be enough. Sadly, there will be network glitches, there will be db hangups, cosmic rays will strike. Even in a five-nines situation (which I think is even beyond what we're promising), you're looking at five hours of downtime a year. Since there's money involved, there has to be some awareness on the client side. It may be as simple as "huh, that request didn't work like I thought, I should hold onto this and try again later". AITC is pretty resilient to posting up the same data again, and, as you observe, the write volume is low enough that repeating uncertain queries isn't going to strain the server.

(In reply to Toby Elliott [:telliott] from comment #33) > Since there's money involved, there has to be some awareness on the client > side. It may be as simple as "huh, that request didn't work like I thought, > I should hold onto this and try again later". AITC is pretty resilient to > posting up the same data again, and, as you observe, the write volume is low > enough that repeating uncertain queries isn't going to strain the server. Yes, as noted above, we are absolutely going to make sure that if the user installs an app, the client will do everything it can to make sure that the record reaches the server (even across application restarts). In fact, in the current implementation, we don't even allow any GETs until all queued PUTs have been successfully processed (in order to avoid reconciliation of any kind).

We should consider showing a little spinning icon on an app that is installed locally but still in the PUT queue when the user is looking at the dashboard. This will help the user distinguish between apps that are on all their devices, and apps that haven not yet finished syncing. When we implement the Device API on the client and rework the dashboard to utilize it, this will become ever more apparent. Noting it here, because the most common failure case we will be seeing is the AITC server being down immediately after an install. We should do everything we can to inform the user of that situation, and ensure that the PUT eventually succeeds.

(In reply to Gregory Szorc [:gps] from comment #30) > As I understand it, AITC is a central, canonical store for app > purchases/receipts. It is effectively a cache of all transactions made > between the user and the various app marketplaces. While AITC is not > technically required, it is a necessary optimization to facilitate rapid and > simple access to your apps data. Not quite: you may self-install apps at their web sites, in which case there is no marketplace. Also, a future role for AITC is to store per-device information, e.g. which apps should appear on which devices, and in what order. That's detailed in the spec: https://wiki.mozilla.org/Apps/AITC. So AITC is not just a cache. > Anyway, my concern is over the robustness of the "syncing" of app receipts > from marketplaces to AITC. In the current implementation, there are many > scenarios in which this would fail: Agreed. > If the record never makes it to AITC, we have a bad user experience. Agreed. > Putting on my product and UX faux hats, I insist that the propagation of > apps from install to AITC be as robust as possible. I think that's an unbounded requirement. We should define use cases we want to support and which we don't. For example, if a user quits Firefox before the app has been sent to AITC, I don't think we need to have other devices know about this new app until the user restarts the Firefox instance that purchased the app. Of course, when the original Firefox is restarted, then we should recover. > The option I would most like to see *from a client perspective* is > cooperation between the marketplace and AITC at app purchase time. Almost certainly a non-starter. Privacy reveal of the AITC server you happen to use, plus a requirement that AITC be network-accessible from the marketplace. AITC could be a local Dropbox folder in the future. The design is such that marketplaces don't know your AITC situation, your user-agent does. > * Have the client store an intent to install an app immediately before it > asks the marketplace to do so. Not doable without changing the navigator.apps API to include this intent, which is unlikely and fairly heavyweight. If the marketplace returns an error or FF crashes after the purchase but before the install, then recovery can happen the next time the user visits that marketplace, which is likely to be right away (reload, or FF restart.) And if a user is wondering why his app is missing, the natural thing to do is to revisit that marketplace. > * As soon as a marketplace transaction is complete, the client should store > an "outbound" record intended for AITC. Like purchase intents, these records > must be persisted across application restarts. Otherwise, they may be lost > and apps never propagate to AITC. This part sounds right, although it can be done a bit more simply: just record, for each app, whether it needs to be pushed to AITC. That bit is set for newly installed apps. > * Persist the set of known marketplaces in the client and/or AITC. > Periodically query said marketplaces and reconcile differences with AITC. > It's the only way to be sure -- Ripley. I don't think we should be assuming any interface to AITC other than the user-agent connecting to it, precisely because we want this to be a user-agent-centric design. There might not be an AITC for users who don't want to be connected. Or there might be a very different mechanism for syncing apps. I agree with rnewman that this makes it a distributed transaction problem. That means we need recovery mechanisms in the user-agent, for sure. Based on one of your points, I propose simply building in a bit more robustness in the client: store all the PUTs you need to make in a durable way. Keep trying. If failures occur for longer than X amount of time, we may need to notify the user. Heck, even Apple notifies the user when they have a server problem.

(In reply to Ben Adida [:benadida] from comment #36) Thank you for the very detailed information! It will help making reviewing this much easier! > We should define use cases we want to support and which we don't. I agree wholeheartedly. > For example, if a user quits Firefox before > the app has been sent to AITC, I don't think we need to have other devices > know about this new app until the user restarts the Firefox instance that > purchased the app. Of course, when the original Firefox is restarted, then > we should recover. Are you declaring that this be a product requirement or merely suggesting it? Speaking of product requirements, I would like to see a list. https://wiki.mozilla.org/Apps seems to have tons of documentation. But, bits about AITC seem to be sparse. Perhaps I'm just not looking in the correct place? > > The option I would most like to see *from a client perspective* is > > cooperation between the marketplace and AITC at app purchase time. > > Almost certainly a non-starter. Privacy reveal of the AITC server you happen > to use, plus a requirement that AITC be network-accessible from the > marketplace. AITC could be a local Dropbox folder in the future. The design > is such that marketplaces don't know your AITC situation, your user-agent > does. That's too bad (from a client implementation perspective). But, I can see why it would be ruled out. I also didn't realize we had those potential future plans for AITC. Is there anything else I should know about so I can keep them in mind when considering future-proofing as part of the review? > > * As soon as a marketplace transaction is complete, the client should store > > an "outbound" record intended for AITC. Like purchase intents, these records > > must be persisted across application restarts. Otherwise, they may be lost > > and apps never propagate to AITC. > > This part sounds right, although it can be done a bit more simply: just > record, for each app, whether it needs to be pushed to AITC. That bit is set > for newly installed apps. Great idea! Who is responsible for the apps backend so we can see about making this happen?

(In reply to Gregory Szorc [:gps] from comment #37) > (In reply to Ben Adida [:benadida] from comment #36) > > This part sounds right, although it can be done a bit more simply: just > > record, for each app, whether it needs to be pushed to AITC. That bit is set > > for newly installed apps. > > Great idea! Who is responsible for the apps backend so we can see about > making this happen? I don't understand the need or purpose for an outbound bit. Why is it different than assuming that an app in the local PUT queue is something that needs to be pushed to AITC?

Product requirements were laid out and sent out by email very, very long ago (even before had a public list), but I will post them here on behalf of Ragavan: -- * Users should be able to log into their App dashboard on any device and access all of their installed Apps. [Login] ** Once available, users should be able to log in with their verified email address. * Users' app entitlements (free and purchased apps) should persist across individual device crashes, lost/stolen devices. [Backup/Recovery] ** If a user loses their only device on which they have been using Apps, they should be able to access all of their Apps on their replacement device by simply logging into their App Dashboard. ** When bringing up a new device, users should not be required to remember all the stores from which they may have acquired Apps in the past. ** It would be acceptable, but not preferable, to have users log into individual stores to revalidate their purchase(s). * Apps should have the ability to sync small amounts of data they store in localStorage. [App data sync] ** Apps should not expect that all of the data they store in localStorage will be synced. Only data explicitly marked to be synced will be. ** Backing up this data is not required and each App is expected to be able to recreate it in the event of data loss. ** App developers should not use this data store for sensitive information as this information will not be encrypted on disk. ** Since the data is not encrypted, it should be available with a username and password across any modem browser (no encryption key required). * The list of a user's Apps should only be accessible to that user. [Security/Privacy] ** Users may choose to make public one or more of their Apps. (e.g. Recommend an App to a friend, Invite a friend to connect from within an App etc.) ** Mozilla may access a user's list of Apps for the purpose of providing applications and services (e.g. Dashboard, Backup/Recovery, Recommendations etc.) * These requirements are intentionally high level. They're more *directional* than *prescriptive*. That said, I'm happy to dig into details as needed, so please feel free to ask questions. * There will undoubtedly be more things for future releases, so expect this set to change as we go forward. -- Note: We have intentionally scoped this initial release to exclude the ability for apps to store data on AITC via localStorage. A feature that was added later was to allow users to determine which of their apps are installed on which devices, but due to technical constraints around whether or not we can reliably determine that a given app is "installed", we have excluded this from the initial release as well.

(In reply to Gregory Szorc [:gps] from comment #37) > > This part sounds right, although it can be done a bit more simply: just > > record, for each app, whether it needs to be pushed to AITC. That bit is set > > for newly installed apps. > > Great idea! Who is responsible for the apps backend so we can see about > making this happen? I meant that the bit would be set in the AITC *client* indicating that it has yet to be pushed to the AITC server. So this task is on Anant, I believe. Anant: I agree that whether you keep a PUT queue or a bit per app doesn't matter. This should be your implementation decision.

I would prefer the "pending AITC sync" flag to be in the apps storage backend, if possible. The reason is that storage service is already built and authoritative. It should be easier to add a flag to an existing storage service than to build a new one from scratch. If you have a separate storage service for managing this bit, now you have to sync 3 things instead of 2. I prefer KISS.

(In reply to Gregory Szorc [:gps] from comment #41) > I would prefer the "pending AITC sync" flag to be in the apps storage > backend, if possible. The reason is that storage service is already built > and authoritative. It should be easier to add a flag to an existing storage > service than to build a new one from scratch. If you have a separate storage > service for managing this bit, now you have to sync 3 things instead of 2. I > prefer KISS. When you say storage service, what are you referring to? As stated previously, I'm having trouble understanding the purpose of a pending flag. Why do we need one? Is it to recover if Firefox crashes in the time between and being installed and the AITC engine receiving the event? That gap in time is on the order of a few hundred milliseconds, so while possible, it is *very* unlikely. What do you find problematic about the idea of a put queue that this patch implements? The implementation is incorrect in its current form (I have an updated patch, but I want to work out the BrowserID parts before I update this bug), but the general idea seems good to me.

(In reply to Anant Narayanan [:anant] from comment #42) > Is it to recover if Firefox crashes in the > time between and being installed and the AITC engine receiving the event? Copy-paste fail, I meant: Is it to recover if Firefox crashes in the time between an app being installed and the AITC engine receiving the event? (As the app will be put in the queue as soon as the event is received).

(In reply to Anant Narayanan [:anant] from comment #42) > When you say storage service, what are you referring to? I don't know exactly. Presumably, the thing in Firefox that is storing the set of installed apps has persistent storage somewhere. We should store a flag there. > As stated previously, I'm having trouble understanding the purpose of a > pending flag. Why do we need one? Is it to recover if Firefox crashes in the > time between and being installed and the AITC engine receiving the event? > That gap in time is on the order of a few hundred milliseconds, so while > possible, it is *very* unlikely. It is mostly the time between when the app is installed and AITC is updated. If we can also eliminate the time between when the app is installed and the observer notification, that would be splendid. > What do you find problematic about the idea of a put queue that this patch > implements? The implementation is incorrect in its current form (I have an > updated patch, but I want to work out the BrowserID parts before I update > this bug), but the general idea seems good to me. Consider the implementation of a put queue vs the implementation of a flag in the underlying store. You know what a put queue looks like. If there is a flag, the workflow is: 1) Query for list of apps that haven't been pushed to AITC 2) Iterate through list and upload to AITC. Set flag as necessary. These "sync" operations can be triggered by install events and supplemented with a recurring timer (that fires every few minutes or so) to catch the apps that fell through the cracks. This model has no put queue, additional storage/state management, etc. It will be much simpler.

(Begin Monday morning drive-by.) > As stated previously, I'm having trouble understanding the purpose of a > pending flag. Why do we need one? Is it to recover if Firefox crashes in the > time between and being installed and the AITC engine receiving the event? > That gap in time is on the order of a few hundred milliseconds, so while > possible, it is *very* unlikely. Or crashes during the HTTP request. Or whatever. Your record for "need to upload this" ought to be written into persistent storage at the same time as the app record itself -- ideally into the same database in the same transaction. If not, you *will* encounter situations in the wild in which "very unlikely" happens a lot. Perhaps a huge GC or fsync happens when flushing session state before your event gets processed by the event loop, and the user force-quits Firefox. You definitely don't want an important write to be interrupted like that *after* a user perceives the app installation process to be complete. > What do you find problematic about the idea of a put queue that this patch > implements? The implementation is incorrect in its current form (I have an > updated patch, but I want to work out the BrowserID parts before I update > this bug), but the general idea seems good to me. From a quick scan of the patch, it doesn't look like the PUT queue is persistent. Any approach must fulfill two criteria, IMO: * The local write of "need to upload this" must be durable and written immediately * Failures to upload a particular record do not block other writes, and can be detected to inform the user. Both of those imply writing some metadata on the app record, rather than just queuing an HTTP POST body. More broadly, I think what Greg is implying is that the AITC client should not really be trying to persist data in-memory and handle transactionality itself. The storage service -- the backend database for apps -- should be safely storing records and all the metadata they need to be uploaded, and the AITC client should be doing the transient work to make sure they've been uploaded. If the client is doing more than storing a timestamp, it's probably doing work that should be pushed lower in the stack.

It seems to be we should be storing (persistently) the server last-modified time for each app, apps that have not been PUT would have a null time. We could keep this information in DOMApplicationRegistry (which I think counts as the "storage service").

Not to pile on here, but there was a specific bit mentioned that seems to have been lost in the mix, namely "periodically query the marketplaces and reconcile differences." Ben mentioned that this shouldn't happen in AITC (I agree) which means that user agents would need to periodically reconcile between marketplaces and AITC. This doesn't need to block first implementation, but sounds like an important robustness feature. Thoughts? Feels like something we can spin out into a followup, blocking k9o if nothing else...

(In reply to Gregory Szorc [:gps] from comment #44) > 1) Query for list of apps that haven't been pushed to AITC > 2) Iterate through list and upload to AITC. Set flag as necessary. > > These "sync" operations can be triggered by install events and supplemented > with a recurring timer (that fires every few minutes or so) to catch the > apps that fell through the cracks. > > This model has no put queue, additional storage/state management, etc. It > will be much simpler. But less efficient since we're traversing over the entire list of apps instead of just knowing which apps need to be put. Keep in mind that app installs are going to be very infrequent, I don't think we should be going through the entire list of apps just to see if we need to PUT something.

(In reply to Anant Narayanan [:anant] from comment #48) > But less efficient since we're traversing over the entire list of apps > instead of just knowing which apps need to be put. Keep in mind that app > installs are going to be very infrequent, I don't think we should be going > through the entire list of apps just to see if we need to PUT something. I doubt you will be able to measure the efficiency loss. And, when you consider the increased robustness and resulting simplicity in AITC code, the tradeoff (if there is one) is worth it.

(In reply to Ian Bicking (:ianb) from comment #46) > It seems to be we should be storing (persistently) the server last-modified > time for each app, apps that have not been PUT would have a null time. We > could keep this information in DOMApplicationRegistry (which I think counts > as the "storage service"). I think this is a fantastic idea. It's likely to give you some free wins later as the client gets more advances - the ability to regenerate a last-state-updated timestamp, and more robust collision/change detection.

(In reply to Richard Newman [:rnewman] from comment #45) > Your record for "need to upload this" ought to be written into persistent > storage at the same time as the app record itself -- ideally into the same > database in the same transaction. > > If not, you *will* encounter situations in the wild in which "very unlikely" > happens a lot. > > Perhaps a huge GC or fsync happens when flushing session state before your > event gets processed by the event loop, and the user force-quits Firefox. > You definitely don't want an important write to be interrupted like that > *after* a user perceives the app installation process to be complete. Thanks, this helps me understand your point of view better. I think the main reason behind our disagreement is that I know what the app storage implementation looks like, while both Greg and yourself give it more credit than is due. The implementation is not "transactional", and nowhere near a "database". It stores a list of apps in-memory and writes it to a file webapps.json on shutdown. Yes, that isn't ideal; yes, it should be more robust; yes, it should have tests (it doesn't have any currently). But it doesn't, we can fix that later, let's keep a laser sharp focus on AITC. > From a quick scan of the patch, it doesn't look like the PUT queue is > persistent. Any approach must fulfill two criteria, IMO: That the queue is not persistent is an implementation flaw, it is certainly meant to, and will be addressed in the next revision. > * The local write of "need to upload this" must be durable and written > immediately > * Failures to upload a particular record do not block other writes, and can > be detected to inform the user. Agreed on both counts. I will change my implementation to have the PUT queue be persistent through mozStorage; instead of in-memory and writing to disk on shutdown (which will fail if Firefox crashes, because the shutdown event is never received). > More broadly, I think what Greg is implying is that the AITC client should > not really be trying to persist data in-memory and handle transactionality > itself. > > The storage service -- the backend database for apps -- should be safely > storing records and all the metadata they need to be uploaded, and the AITC > client should be doing the transient work to make sure they've been > uploaded. If the client is doing more than storing a timestamp, it's > probably doing work that should be pushed lower in the stack. Here, I will humbly disagree. Information on whether or not an app is synced belongs in the AITC client, app storage doesn't need to know or care.

Lots of comments in the time I took to write that last one! Just want to summarize: - We all agree that the information about which apps are yet to be PUT needs to persistent across crashes and application restarts. - I propose changing the current PUT queue implementation to use mozStorage instead (a write will be made as soon as an install event is received). We need to rewrite the mozApps implementation at a later point. Let's work with what we have right now.

(In reply to Anant Narayanan [:anant] from comment #51) > Thanks, this helps me understand your point of view better. I think the main > reason behind our disagreement is that I know what the app storage > implementation looks like, while both Greg and yourself give it more credit > than is due. The implementation is not "transactional", and nowhere near a > "database". It stores a list of apps in-memory and writes it to a file > webapps.json on shutdown. It doesn't write on shutdown, but everytime a change to the app list is made. The truth is that it's not transactionnal and I'd like to move it to an indexedDB storage. This looks like a good opportunity to do it now. > Here, I will humbly disagree. Information on whether or not an app is synced > belongs in the AITC client, app storage doesn't need to know or care. I have no strong opinion on this, but the "classic" sync client we wrote for apps (that probably fulfills 99% of the needs here) didn't need more data to be stored in the DOMApplicationRegistry. I'm still challenging the very usefulness of all this.

(In reply to Toby Elliott [:telliott] from comment #50) > (In reply to Ian Bicking (:ianb) from comment #46) > > It seems to be we should be storing (persistently) the server last-modified > > time for each app, apps that have not been PUT would have a null time. We > > could keep this information in DOMApplicationRegistry (which I think counts > > as the "storage service"). > > I think this is a fantastic idea. It's likely to give you some free wins > later as the client gets more advances - the ability to regenerate a > last-state-updated timestamp, and more robust collision/change detection. I like the idea, but we still need to store some information in the AITC client, such as the number of times we tried to PUT a particular app; and if that's beyond a particular number we need to notify the user etc. As long as we're storing a little, we might as well store everything we need. Additionally, we can build a persistent queue in the AITC client that's more robust than the current app storage implementation.

(In reply to Fabrice Desré [:fabrice] from comment #53) > I have no strong opinion on this, but the "classic" sync client we wrote for > apps (that probably fulfills 99% of the needs here) didn't need more data to > be stored in the DOMApplicationRegistry. I'm still challenging the very > usefulness of all this. That client, IIRC, used a tracker (which is persisted to disk); Sync took care of retries etc. Not perfect, but functional. For all current Sync work we're discouraging the use of trackers, instead using timestamp-addressable lookup in the backend storage, which is why many of Firefox's data sources now have millisecond-precision modified times and flags for deletion. That's essentially what we're advocating here.

(In reply to Richard Newman [:rnewman] from comment #55) > (In reply to Fabrice Desré [:fabrice] from comment #53) > > > I have no strong opinion on this, but the "classic" sync client we wrote for > > apps (that probably fulfills 99% of the needs here) didn't need more data to > > be stored in the DOMApplicationRegistry. I'm still challenging the very > > usefulness of all this. > > That client, IIRC, used a tracker (which is persisted to disk); Sync took > care of retries etc. Yep > Not perfect, but functional. For all current Sync work we're discouraging > the use of trackers, instead using timestamp-addressable lookup in the > backend storage, which is why many of Firefox's data sources now have > millisecond-precision modified times and flags for deletion. Interesting, thanks for the info! > That's essentially what we're advocating here. Ok, but so why not use the current sync framework?

I didn't realize DOMApplicationRegistry was so simple. That explains a lot of the perspective differences. So, the question becomes what to do now. I contend that the best end state is for DOMApplicationRegistry to manage as much as possible. If nothing else, there will be one persistent store to manage, not two. From AITC's perspective, that's a huge win. I'd argue that the cost of changing DOMApplicationRegistry today is less than or equal to the cost for implementing things in AITC today, especially when you consider the cost for refactoring it later. This is based on a cursory glance of the existing code and that the priority of this feature will ensure people are able to help. Therefore, we should modify DOMApplicationRegistry to track things instead of coding a temporary solution in AITC. While DOMApplicationRegistry may not be as robust as we want it to be, I don't think it will be any worse than what we'd do for AITC. And, when it is upgraded in the future, AITC will gain those benefits transparently. Let's file a new bug to make changes to DOMApplicationRegistry. Perhaps we can recycle bug 745069?

(In reply to Gregory Szorc [:gps] from comment #57) > I'd argue that the cost of changing DOMApplicationRegistry today is less > than or equal to the cost for implementing things in AITC today, especially > when you consider the cost for refactoring it later. This is based on a > cursory glance of the existing code and that the priority of this feature > will ensure people are able to help. Therefore, we should modify > DOMApplicationRegistry to track things instead of coding a temporary > solution in AITC. I don't think this solution will be temporary. I still believe that all metadata regarding the sync state of an application belongs in the AITC. Putting the number of PUT retries, for instance, doesn't belong either in the app record or the app registry. Since we need *some* persistent storage in AITC anyway, why not move the flag too? > While DOMApplicationRegistry may not be as robust as we want it to be, I > don't think it will be any worse than what we'd do for AITC. And, when it is > upgraded in the future, AITC will gain those benefits transparently. > > Let's file a new bug to make changes to DOMApplicationRegistry. Perhaps we > can recycle bug 745069? I'm trying to avoid AITC spiraling into a dependency chain that's too big to manage. It is already a pretty large undertaking (with the BrowserID module being the other big component, which has issues of its own). We missed FF14, but we really, really want to make FF15. At some point we have to balance correctness with timelines. We could go on a big tangent here, and end up fixing a bug in IndexedDB, which we will no doubt discover when we rewrite DOMApplicationRegistry :) I fully agree that a rewrite of the registry is necessary and imminent, but I maintain that it is not required for robustness of AITC. We can build a robust PUT queue based on persistent storage, and I believe that will fulfill all your robustness requirements except one: FF crashes right after an install but before the app was put in the queue, and this is a known tradeoff I am willing to take at this stage.

(In reply to Anant Narayanan [:anant] from comment #58) > (In reply to Gregory Szorc [:gps] from comment #57) > > I'd argue that the cost of changing DOMApplicationRegistry today is less > > than or equal to the cost for implementing things in AITC today, especially > > when you consider the cost for refactoring it later. This is based on a > > cursory glance of the existing code and that the priority of this feature > > will ensure people are able to help. Therefore, we should modify > > DOMApplicationRegistry to track things instead of coding a temporary > > solution in AITC. > > I don't think this solution will be temporary. I still believe that all > metadata regarding the sync state of an application belongs in the AITC. I can see both sides of this debate. But, since sync to AITC appears to be a core or nearly ubiquitous feature of webapps, I think the sync state should reside with the apps storage service. > Putting the number of PUT retries, for instance, doesn't belong either in > the app record or the app registry. Since we need *some* persistent storage > in AITC anyway, why not move the flag too? I don't think we'll need to record such fine-grained per-app details. IMO we only need: * per-app time when it was last synced to AITC or null if never synced. * Global AITC client state to record backoff info and server errors. AITC client/server state doesn't necessarily need to survive app restarts. However, we probably want to persist things like last time successfully synced so we can alert the user if AITC has been non-functional for a number of days. This can be persisted in prefs (like how Sync does it). > > While DOMApplicationRegistry may not be as robust as we want it to be, I > > don't think it will be any worse than what we'd do for AITC. And, when it is > > upgraded in the future, AITC will gain those benefits transparently. > > > > Let's file a new bug to make changes to DOMApplicationRegistry. Perhaps we > > can recycle bug 745069? > > I'm trying to avoid AITC spiraling into a dependency chain that's too big to > manage. It is already a pretty large undertaking (with the BrowserID module > being the other big component, which has issues of its own). We missed FF14, > but we really, really want to make FF15. I understand that. I think the effort to implement things in DOMApplicationRegistry is roughly the same as doing it in AITC. And it is better. And it saves time down the road. > At some point we have to balance correctness with timelines. We could go on > a big tangent here, and end up fixing a bug in IndexedDB, which we will no > doubt discover when we rewrite DOMApplicationRegistry :) I'm not directly advocating moving DOMApplicationRegistry to IndexedDB today. That move can be made when the PMs want better transactionality through the whole chain. Now, I would advise them to do that ASAP, but it isn't my call. That reminds me, an out from all of this is someone with authority can override me on my insistence for robustness. I still haven't heard anybody do that, so I'm still holding my line on making this as robust as possible [because money is involved and it is the right thing to do]. Delegating sync state management to DOMApplicationRegistry is the best way to do that because it doesn't add more links to the app purchase/transaction chain.

A complete refactor of the AitC client. It now consists of 4 modules: - main.js: Responsible for monitoring when the user is on the apps dashboard as well as app install/uninstall events. - manager.js: Responsible for acting on changes observed by main.js, which may include obtaining a BrowserID assertion and a Sagrada token. - client.js: Implements the client-side API for AitC. - storage.js: Provides a file-backed Queue data structure, as well as an interface to reconcile remote changes with the local app registry and apply them. I've decided to use a local file to persist the PUT queue. A technically more robust solution would be to roll this functionality into DOMApplicationRegistry, but that would be a significant undertaking as uninstalls needs to be handled as well (app records for uninstalls needs to be kept around until the PUT succeeds, which the current registry doesn't do). I suggest we tackle that refactor, along with many other much-needed improvements to DOMApplicationRegistry, at a later date. This PUT queue approach satisfies all robustness requirements laid out earlier, with one exception: a crash in between an app's successful install and the write to the PUT queue will result in the app never being uploaded. It is possible to recover from this scenario, see Bug 750614.

Greg, there are two known issues with this patch: the token is still being logged, and the token server URL is hard-coded. I'm waiting on Bug 750566 to land before I make final finishing touches to the patch; as I'm currently unable to verify uninstall functionality. We'll likely need one more review cycle, but hopefully it will be very short.

Question - Is this implementation with the old dashboard (myapps.mozillalabs.com) or the new dashboard in development on persona.org (https://persona-dev.mozillalabs.com/)?

+ BrowserID.getAssertion(gotSilentAssertion, { + audience: this.DASHBOARD, sameEmailAs: this.MARKETPLACE + }); Anant, Have a usability question that had come up, and wanted to make sure I was wrong. This code seems to get the browserid assertion for the e-mail address that was last logged into the Marketplace ? -- Does this mean apps would get synced to a specific e-mail address, and apps are not synced to the parent browserID e-mail address? The rest of my thoughts are based upon if this above observation is true, so if i am wrong then stop reading :) . #1) BrowserID was supposed to be one id, not several. The multiple e-mail addresses are because you don't want to share specific id's with specific sites, not because you'd want to segment your apps ? #2) I can think of some wonky scenarios where users buy specific things under specific accounts, but then they all get synced up to the last logged in browserid e-mail address, when you run appsync. Which defeats the purpose of buying under different e-mail addresses. #3) Current implementation would be really hard to figure out from the end user how this was working ? Especially when you add multiple device types to the mix.

(In reply to dclarke@mozilla.com from comment #63) > This code seems to get the browserid assertion for the e-mail address that > was last logged into the Marketplace ? > > -- Does this mean apps would get synced to a specific e-mail address, and > apps are not synced to the parent browserID e-mail address? That's correct, apps are synced to the same email address that was used to login to the Marketplace. There is no such thing as a "parent BrowserID e-mail address". > #1) BrowserID was supposed to be one id, not several. The multiple e-mail > addresses are because you don't want to share specific id's with specific > sites, not because you'd want to segment your apps ? No, a BrowserID account is a list of email addresses, and no particular address in the list is different from the other in any way. I think multiple email addresses can be used to segment your app purchases into different AitC accounts. I think the name "Persona" matches well with this expectation, i.e. you identify each of your personas with a particular email address. > #2) I can think of some wonky scenarios where users buy specific things > under specific accounts, but then they all get synced up to the last logged > in browserid e-mail address, when you run appsync. Which defeats the purpose > of buying under different e-mail addresses. We're careful to only reuse the last email used at the Mozilla marketplace, not any email address that was last used to login. This is a short term measure as the Marketplace is likely to be the only place to get apps for a few months. You're right that if I login to the Marketplace as user A and then go to some other site to install an app, it will be synced to A. However, if I simply switch my BrowserID login to B on any other site other than the marketplace, the app won't go to B, we will instead ask the user to explicitly login on their next dashboard visit. > #3) Current implementation would be really hard to figure out from the end > user how this was working ? Especially when you add multiple device types to > the mix. Yes, multiple device types would complicate this a bit, but we're not implementing the device API in this version. I'm hoping to rely on the "Login to the Browser" feature by the time we get to that. It will then always be crystal clear which account the apps are being sent to - namely the account you used to login to Firefox.

(In reply to Jason Smith [:jsmith] from comment #62) > Question - Is this implementation with the old dashboard > (myapps.mozillalabs.com) or the new dashboard in development on persona.org > (https://persona-dev.mozillalabs.com/)? This patch is against the old dashboard. The switch to the new dashboard is very trivial to do (just changing a default pref).

Comment on attachment 619840 [details] [diff] [review] AITC Desktop Client v3 Review of attachment 619840 [details] [diff] [review]: ----------------------------------------------------------------- This is looking much better! This review focused mostly on client.js and build system foo. I didn't look at {main,manager,storage,service,services-aitc}.js. Since client.js is mostly standalone and since it is almost ready to land, let's split it out into a separate bug/review and land it first. Then, we can focus on the other bits. Also, the tests really need to land with the code it tests. Please include more AitcClient tests in the next patch (even if they don't work - I know you are waiting on the mock server). Again, this is looking pretty darn good. Very close to getting r+. ::: services/aitc/modules/client.js @@ +25,5 @@ > + > + this._backoff = false; > + if (PREFS.get("backoff", 0)) { > + this._backoff = true; > + } What if there are multiple AitcClient instances pointing to different servers? You don't want them sharing the preferences, do you? (This is done throughout this file.) A valid answer here is "we don't support that." But, that would be poor software engineering. I highly discourage this from being the answer. But, I may let it slide. The workaround would be to have the constructor take a map of state. Then, build a simple function elsewhere which loads state from preferences. @@ +27,5 @@ > + if (PREFS.get("backoff", 0)) { > + this._backoff = true; > + } > + > + this._appsLastModified = null; Shouldn't this be preserved across client runs? @@ +91,5 @@ > + req.setHeader("x-if-modified-since", this._appsLastModified); > + } > + > + let self = this; > + req.get(function(error) { This is a long closure. I'd prefer to see it pulled out to the prototype. @@ +121,5 @@ > + // Convert apps from remote to local format > + let apps = []; > + for each (let app in tmp) { > + apps.push(self._makeLocalApp(app)); > + } consider: let apps = tmp.map(self._makeLocalApp, self); @@ +127,5 @@ > + self._log.info("getApps succeeded and got " + apps.length); > + cb(null, apps); > + > + // Don't update lastModified until we know cb succeeded. > + self._appsLastModified = parseInt(req.response.headers['x-timestamp']); Should this be before the callback? @@ +129,5 @@ > + > + // Don't update lastModified until we know cb succeeded. > + self._appsLastModified = parseInt(req.response.headers['x-timestamp']); > + } catch (e) { > + self._log.error("Exception in getApps " + e); cb is never called in this case. What if calling cb throws? @@ +141,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)) ? Could there be values that evaluate to false? @@ +167,5 @@ > + * registry expects. (Inverse of _makeRemoteApp) > + */ > + _makeLocalApp: function _makeLocalApp(app) { > + for each (let key in this._requiredRemoteKeys) { > + if (!app[key]) { if (!(key in app)) ? @@ +191,5 @@ > + * Try PUT for an app on the server and determine if we should retry > + * if it fails. > + */ > + _putApp: function _putApp(app, cb) { > + if (!this._checkBackoff) { You meant to do a function call? @@ +198,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 functionality in this module related to a queue. Change the property to something more generic. .backoffPrevented = true? @@ +206,5 @@ > + > + let uri = this._makeAppURI(app.origin); > + let req = new TokenAuthenticatedRESTRequest(uri, this.token); > + if (app.modified) { > + req.setHeader("X-If-Unmodified-Since", app.modified); "" + app.modified @@ +211,5 @@ > + } > + > + let self = this; > + this._log.info("Trying to _putApp to " + uri); > + req.put(JSON.stringify(app), function _tryPuttingAppFinished(error) { Please move long closure to prototype. @@ +234,5 @@ > + case 413: > + let msg = "_putApp returned: " + req.response.status; > + self._log.warn(msg); > + err = new Error(msg); > + err.removeFromQueue = true; Need better property name. See above comment. @@ +259,5 @@ > + > + _makeAppURI: function _makeAppURI(origin) { > + let part = CommonUtils.encodeBase64URL( > + CryptoUtils._sha1(origin) > + ).replace(/=/, ""); You don't need a regexp for single character replacement. .replace("=", "") @@ +269,5 @@ > + if (!this._backoff) { > + return true; > + } > + > + let time = new Date().getTime(); Date.now() @@ +271,5 @@ > + } > + > + let time = new Date().getTime(); > + let lastReq = parseInt(PREFS.get("lastReq", 0)); > + let backoff = parseInt(PREFS.get("backoff", 0)); When not store the backoff interval directly in the client instance? @@ +273,5 @@ > + let time = new Date().getTime(); > + let lastReq = parseInt(PREFS.get("lastReq", 0)); > + let backoff = parseInt(PREFS.get("backoff", 0)); > + > + if (lastReq + (backoff * 1000) < time) { Instead of calculating the allowed resumption time using lastReq, I would just store the time that backoff expires. The value would only be calculated once, so there are fewer places it could break. @@ +274,5 @@ > + let lastReq = parseInt(PREFS.get("lastReq", 0)); > + let backoff = parseInt(PREFS.get("backoff", 0)); > + > + if (lastReq + (backoff * 1000) < time) { > + this._log.warn("X-Backoff is " + backoff + ", not making request"); Nit: IMO it would be more helpful to log the remaining backoff interval. @@ +286,5 @@ > + > + // Set values from X-Backoff and Retry-After headers, if present > + _setBackoff: function _setBackoff(req) { > + let backoff = 0; > + let time = new Date().getTime(); Date.now() @@ +294,5 @@ > + backoff = req.response.headers['x-backoff']; > + } > + if (req.response.headers['retry-after']) { > + backoff = req.response.headers['retry-after']; > + } Please log when these headers are seen. ::: services/aitc/tests/unit/test_load_modules.js @@ +1,3 @@ > +const modules = [ > + "main.js", > + "client.js", You're missing some modules.

(In reply to Anant Narayanan [:anant] from comment #64) > (In reply to dclarke@mozilla.com from comment #63) > > This code seems to get the browserid assertion for the e-mail address that > > was last logged into the Marketplace ? > > > > -- Does this mean apps would get synced to a specific e-mail address, and > > apps are not synced to the parent browserID e-mail address? > > That's correct, apps are synced to the same email address that was used to > login to the Marketplace. There is no such thing as a "parent BrowserID > e-mail address". > > > #1) BrowserID was supposed to be one id, not several. The multiple e-mail > > addresses are because you don't want to share specific id's with specific > > sites, not because you'd want to segment your apps ? > > No, a BrowserID account is a list of email addresses, and no particular > address in the list is different from the other in any way. I think multiple > email addresses can be used to segment your app purchases into different > AitC accounts. I think the name "Persona" matches well with this > expectation, i.e. you identify each of your personas with a particular email > address. Interesting I had believed browserID was a method of sharing information about your persona(singular). The multiple e-mail addresses was used as a way of sharing just the information that you wanted to share with a specific site. Not as a way of maintaining several distinct identities. > > #2) I can think of some wonky scenarios where users buy specific things > > under specific accounts, but then they all get synced up to the last logged > > in browserid e-mail address, when you run appsync. Which defeats the purpose > > of buying under different e-mail addresses. > > We're careful to only reuse the last email used at the Mozilla marketplace, > not any email address that was last used to login. This is a short term > measure as the Marketplace is likely to be the only place to get apps for a > few months. You're right that if I login to the Marketplace as user A and > then go to some other site to install an app, it will be synced to A. What we dont' state here is the obvious case where a user logs into Marketplace with B, and then all your apps get synced to B, but at the same time you have a previous revision synced to A. Your apps don't get synced to your other devices anymore and you aren't sure why. I would rather a popup which listed e-mail addresses, and i picked which e-mail address i wanted to use to sync. > However, if I simply switch my BrowserID login to B on any other site other > than the marketplace, the app won't go to B, we will instead ask the user to > explicitly login on their next dashboard visit. > > #3) Current implementation would be really hard to figure out from the end > > user how this was working ? Especially when you add multiple device types to > > the mix. > > Yes, multiple device types would complicate this a bit, but we're not > implementing the device API in this version. I'm hoping to rely on the > "Login to the Browser" feature by the time we get to that. It will then > always be crystal clear which account the apps are being sent to - namely > the account you used to login to Firefox. agreed

(In reply to dclarke@mozilla.com from comment #67) > Interesting I had believed browserID was a method of sharing information > about your persona(singular). The multiple e-mail addresses was used as a > way of sharing just the information that you wanted to share with a specific > site. Not as a way of maintaining several distinct identities. You can think of it your way too, it just so happens that the email the user chose to share with the marketplace is the same one they implicitly share with the AitC server. > What we dont' state here is the obvious case where a user logs into > Marketplace with B, and then all your apps get synced to B, but at the same > time you have a previous revision synced to A. Your apps don't get synced to > your other devices anymore and you aren't sure why. Yes, but there an underlying problem that causes this behavior and that is the local app repo not supporting multiple app "buckets". We are tracking that in bug 746204. I would not though that merely logging in to the marketplace as B is not sufficient for all apps to be uploaded to B's AitC account. The user would have to install, or uninstall an app from the marketplace as B for that to happen. The unfortunate side of effect of all the other apps also being dragged along is temporary and should go away when we either fix 746204 or get login to the browser working.

(In reply to Gregory Szorc [:gps] from comment #66) > @@ +129,5 @@ > > + > > + // Don't update lastModified until we know cb succeeded. > > + self._appsLastModified = parseInt(req.response.headers['x-timestamp']); > > + } catch (e) { > > + self._log.error("Exception in getApps " + e); The server sends an "X-Last-Modified" header which would be a much better choice here than X-Timestamp. There's a rather long-winded thread on services-dev discussing the rationale behind this: https://mail.mozilla.org/private/services-dev/2012-March/001226.html

Anant - We need to check if this implementation integrates well with the proposal in bug 749033.

Comment on attachment 619840 [details] [diff] [review] AITC Desktop Client v3 Review of attachment 619840 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/aitc/modules/main.js @@ +72,5 @@ > + let listener = { > + onLocationChange: function onLocationChange(browser, pr, req, loc, flag) { > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + if (win.gBrowser.selectedBrowser == browser) { > + let uri = loc.spec.substring(0, self.DASHBOARD.length); Current dashboard uri is "https://myapps.mozillalabs.com". The code will match a spec of "https://myapps.mozillalabs.comabcd.mydomain.com" due to the substring. @@ +82,5 @@ > + }; > + // Called when the current tab selection changes. > + function tabSelected(event) { > + let browser = event.target.linkedBrowser; > + let uri = browser.currentURI.spec.substring(0, self.DASHBOARD.length); ditto @@ +113,5 @@ > + browser.addTabsProgressListener(listener); > + browser.tabContainer.addEventListener("TabSelect", tabSelected); > + > + // Also check the currently open URI. > + let uri = browser.contentDocument.location.toString().substring( ditto ::: services/aitc/modules/storage.js @@ +147,5 @@ > + try { > + data = JSON.parse( > + NetUtil.readInputStreamToString(stream, stream.available()) > + ); > + stream.close(); Should the close() be in a finally clause?

Ye landing plan herewith: 1. Build changes and AitC REST client (bug 754538) 1a. Tests for AitC REST client (bug 750948) 2. Storage layer (bug 755375) 3. AitC BrowserID module (bug 745345) 4. AitC Manager and Service code (bug 757261) These bugs must land in that order. Try results with those patches applied are in: https://tbpl.mozilla.org/?tree=Try&rev=c78da7a37f06 Try builds at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/anarayanan@mozilla.com-c78da7a37f06/ Give it a whirl!

Tried testing the try build, but had no luck on my machine getting AITC integration to work :(. Here's what I tried: Scenario 1 1. Launch firefox with a fresh profile 2. Go to myapps.mozillalabs.com Result - No browser ID prompt Error Console - Nothing Scenario 2 1. Launch firefox with a fresh profile 2. Enable the myapps dashboard through about:config 3. Go to myapps.mozillalabs.com Result - No browser ID prompt Error Console - Nothing Scenario 3 1. Launch firefox with a fresh profile 2. Go to marketplace.mozilla.org 3. Login with a browser ID account with apps already installed Result - Successful login to marketplace, but an error was shown in the console that looks suspicious Error Console: Timestamp: 5/30/2012 9:52:30 AM Error: [Exception... "'PopupNotifications_show: invalid browser' when calling method: [nsILoginManagerPrompter::promptToSavePassword]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: resource:///components/nsLoginManager.js :: <TOP_LEVEL> :: line 938" data: no] Source File: resource:///components/nsLoginManager.js Line: 938

Tried the 2nd try build Anant put together - still got the same behavior in comment 73.

Builds should appear soon: https://tbpl.mozilla.org/?tree=Try&rev=1c582a2ae421 http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/anarayanan@mozilla.com-1c582a2ae421 Third time's the charm? :)

(In reply to David Chan [:dchan] from comment #71) > ::: services/aitc/modules/main.js > @@ +72,5 @@ > > + let listener = { > > + onLocationChange: function onLocationChange(browser, pr, req, loc, flag) { > > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > > + if (win.gBrowser.selectedBrowser == browser) { > > + let uri = loc.spec.substring(0, self.DASHBOARD.length); > > Current dashboard uri is "https://myapps.mozillalabs.com". The code will > match a spec of "https://myapps.mozillalabs.comabcd.mydomain.com" due to the > substring. Good catch! I've fixed this in the patch over at bug 757261. > @@ +113,5 @@ > > + browser.addTabsProgressListener(listener); > > + browser.tabContainer.addEventListener("TabSelect", tabSelected); > > + > > + // Also check the currently open URI. > > + let uri = browser.contentDocument.location.toString().substring( > > ditto > > ::: services/aitc/modules/storage.js > @@ +147,5 @@ > > + try { > > + data = JSON.parse( > > + NetUtil.readInputStreamToString(stream, stream.available()) > > + ); > > + stream.close(); > > Should the close() be in a finally clause?

(In reply to Anant Narayanan [:anant] from comment #75) > Builds should appear soon: > > https://tbpl.mozilla.org/?tree=Try&rev=1c582a2ae421 > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/anarayanan@mozilla. > com-1c582a2ae421 > > Third time's the charm? :) Looks like the try build didn't pass for windows.

(In reply to Jason Smith [:jsmith] from comment #77) > (In reply to Anant Narayanan [:anant] from comment #75) > > Third time's the charm? :) > > Looks like the try build didn't pass for windows. No, it's just pending. The Windows load has been exceptionally high, so we're still in queue (http://build.mozilla.org/builds/pending/try.html). Win64-opt is done though, if you want to grab that one.

(In reply to Anant Narayanan [:anant] from comment #78) > (In reply to Jason Smith [:jsmith] from comment #77) > > (In reply to Anant Narayanan [:anant] from comment #75) > > > Third time's the charm? :) > > > > Looks like the try build didn't pass for windows. > > No, it's just pending. The Windows load has been exceptionally high, so > we're still in queue (http://build.mozilla.org/builds/pending/try.html). > > Win64-opt is done though, if you want to grab that one. Oh woops. Anyways, some initial feedback from testing the win 64 build in notes form: Win 64 build try build quick test - BrowserID pop-up is appearing on myapps - No ability to port over existing apps installed to be synced to cloud - Seeing the below error a lot, particularly on an install of a web app Timestamp: 5/31/2012 7:52:07 AM Error: [Exception... "'ReferenceError: e is not defined' when calling method: [nsIStreamListener::onStopRequest]" nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)" location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0" data: no] - Basic sync test did not work (install app on one profile upon logging in on marketplace, view on marketplace on separate profile)

(In reply to Jason Smith [:jsmith] from comment #79) > - No ability to port over existing apps installed to be synced to cloud That's expected, we haven't built the functionality yet. We'll see if we can get to this before landing. > - Seeing the below error a lot, particularly on an install of a web app I have enabled logging in all these builds to make sure to capture stdout so I can get more information. > - Basic sync test did not work (install app on one profile upon logging in > on marketplace, view on marketplace on separate profile) Tell me more. You mean viewing in the dashboard on a separate profile? Did you login with the same ID in both places? Same as above, capture stdout and pastebin it so I can analyze. I'll do a build on my windows machine now so we can get a Win32-debug build, in case the try build doesn't finish before the bug bash today.

(In reply to Anant Narayanan [:anant] from comment #80) > (In reply to Jason Smith [:jsmith] from comment #79) > > - No ability to port over existing apps installed to be synced to cloud > > That's expected, we haven't built the functionality yet. We'll see if we can > get to this before landing. > > > - Seeing the below error a lot, particularly on an install of a web app > > I have enabled logging in all these builds to make sure to capture stdout so > I can get more information. > > > - Basic sync test did not work (install app on one profile upon logging in > > on marketplace, view on marketplace on separate profile) > > Tell me more. You mean viewing in the dashboard on a separate profile? Did > you login with the same ID in both places? Same as above, capture stdout and > pastebin it so I can analyze. > > I'll do a build on my windows machine now so we can get a Win32-debug build, > in case the try build doesn't finish before the bug bash today. Just to document what happened in IRC: Pastebin: http://pastebin.com/AyyFVmed Pastebin above should contain the problem, as I had the problem occur after login to browser ID on myapps with an app already installed. Anant identified the problem here: PUT https://stage-aitc10.services.mozilla.com/1.0/26320804/apps/Xf3RXn6hfPR2WFaJhTbDDvDljDI 400 400 is a bad request Note to self - capturing stdout logs is done through firefox -console > output.txt

Here is my summary of my attempts to delete apps from a current account with 4 total apps (30-day pastebin): http://jbonacci.pastebin.mozilla.org/1654890 The pastebins referenced in that one are also 30-day, so we have some time to investigate this delete app issue to determine if there is one (or more) bug to write...

Random question - Is this bug existing as a meta bug? Just wondering, cause I saw https://wiki.mozilla.org/Apps/Status which implies we did land the required changes for AITC desktop support. Or am I mistaken somewhere? Anant - Could you clarify what we are tracking to be completed here? Or am I just confused?

(In reply to Jason Smith [:jsmith] from comment #84) > Random question - Is this bug existing as a meta bug? Just wondering, cause > I saw https://wiki.mozilla.org/Apps/Status which implies we did land the > required changes for AITC desktop support. Or am I mistaken somewhere? > > Anant - Could you clarify what we are tracking to be completed here? Or am I > just confused? The initial implementation (consisting of bug 755375, bug 754538, bug 757261 and bug 745345) has been completed, but we still have a bunch of follow-up bugs to finish before we can enable AITC by default. I think we should leave this meta bug open until: a) All existing bugs blocking this one have been resolved (bug . b) We have identified the set of bugs that are considered "P1" for the AITC client to be pref'ed on, marked them as blockers to this one, and fixed them.

Sounds good to me. So is there a pref I need to set in about:config to enable this feature in nightly so that I can at least do some initial testing on a nightly production build? I know originally I recall the pref "services.sync.engine.apps" when set to true would disable the feature, opposite would enable it. Or do I still need to work off of try builds?

(In reply to Jason Smith [:jsmith] from comment #86) > Sounds good to me. So is there a pref I need to set in about:config to > enable this feature in nightly so that I can at least do some initial > testing on a nightly production build? I know originally I recall the pref > "services.sync.engine.apps" when set to true would disable the feature, > opposite would enable it. Or do I still need to work off of try builds? In addition to making sure "services.sync.engine.apps" is false, you should set "services.aitc.enabled" to true in the latest Aurora/Nightly builds to test the feature.

meta bug - removing qa+ - will verify on a per bug basis

A code review was done for this a while ago. The only notable issue was fixed by Anant

This was done in its dependent bugs.