Fetch and cache the experiment manifest on the client.

previous work: - snippets service * serves different lists of snippets to different clients, based on passed variable / attributes. - test pilot 1: "testpilot.mozillalabs.com/index.json" * very simple * not awesome for implying a 'service that can give different * a bit gross to delpoy - AMO / addon / `update.rdf` * big and complicated, but has lots of subsetting.

Prior art: snippets service, blocklist service. You might be able to share or copy code from either of those. Avoid main-thread I/O.

Snippets stuff: 10:17 <Osmose> https://github.com/mozilla/snippets-service would be the repo 10:17 <Osmose> https://github.com/mozilla/home-snippets-server would be the old-er repo 10:18 <Osmose> As for the browser code... 10:18 <Osmose> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js

WIP, so far i failed to get this skeleton notified & instantiated by update-timer.

bsmedberg, any ideas why this isn't working?

I missed actually changing the interface name, but it still is not working.

gps, i've heard you might have an idea here?

bbondy, the review history for nsUpdateTimerManager suggests you might know?

Comment on attachment 8380621 [details] [diff] [review] WIP Review of attachment 8380621 [details] [diff] [review]: ----------------------------------------------------------------- I'd put things under /toolkit rather than /browser since /browser is application specific and this code could very well be used in multiple applications. Rule of thumb: assume multiple application use up front unless the code is application-central. You are building a mostly background service, so this is toolkit territory. You'll need to update browser/installer/package-manifest.in to include Experiments.jsm and Experiments.manifest. Also, all new features should land behind a MOZ_<FEATURE> #ifdef somewhere so they can be easily disabled without backing out a bunch of code. ::: browser/experiments/Experiments.jsm @@ +17,5 @@ > + dump("*** Experiments_notify()\n"); > + } > + > + classID: Components.ID("{f7800463-3b97-47f9-9341-b7617e6d8d49}"), > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, This type doesn't implement the nsIObserver interface. Drop nsIObserver. ::: browser/experiments/nsIExperiments.idl @@ +8,5 @@ > + > +[scriptable, uuid(f7800463-3b97-47f9-9341-b7617e6d8d49)] > +interface nsIExperiments : nsISupports > +{ > + You don't need an XPIDL interface to provide an XPCOM service. Not sure why you added this here.

I think I provided answers in my not-review.

> I'd put things under /toolkit rather than /browser since /browser is > application specific and this code could very well be used in multiple > applications. I encouraged him to put this in browser/. We don't have specific plans to share this code yet, and we can always move it if necessary later, but it's unlikely that Android will share this. If we don't put it in browser, please don't put it in toolkit/ which is a dumping ground for random stuff. Create a new toplevel directory experiments/ and put it there. > Also, all new features should land behind a MOZ_<FEATURE> #ifdef somewhere > so they can be easily disabled without backing out a bunch of code. It's not a rule or recommendation for every new feature to land behind an ifdef: ifdefs are mainly used where we can't control risk just using prefs. In this case it seems to me that a pref would be fine.

You need brendan's permission to create new toplevel directories. Someone told me there was a policy on shipping new features behind #ifdefs. Maybe it was just a best practice. I know it's my practice - I want a one-line kill switch to prevent code from shipping, loading, and running in automation because said switch is extremely useful. Said switch also makes our go/no go uplift decisions much easier (just change a single line).

The policy is that we need to be able to disable new features. Any of the following methods are allowable: * backout * pref flip * build-time ifdef Build-time ifdefs are the hardest to maintain and are therefore only encouraged when the other forms don't work. Please put this under browser/

Got the skeleton problems resolved thanks to ttaubert now - yay for silent errors & failures.

I sketched the major parts out, still need to adapt it to the expected data format and clean up. Open questions: * we will have to kick off loading the cached manifest (and possibly kicking off experiment installs) on session start - what can we hook into for that? * I'm unsure what the best approach is to check if the manifest file actually changed on the server (before actually checking contents) gps, any ideas here?

Or maybe you glind?

Don't worry too much about checking if the file changed. If it's still in the cache, etag-based caching should short-circuit the load and otherwise it won't be a big deal to refetch it. Actual checking of conditions and enabling an experiment should be handled in bug 974009. Perhaps we should trigger that from the daily update here triggered from the update timer.

Cleaned up and modified to properly work for bug 974009. gps, how does this look?

Comment on attachment 8386167 [details] [diff] [review] Initial directory structure, fetch/cache manifest, simple test Review of attachment 8386167 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good. So, I'm not a huge fan of the monolithic, non-JSM .js files defining components/services. Instead, I prefer moving as much logic as possible into JSMs and types that aren't single-use. (See toolkit/components/crashes/ for an example of what I consider the "preferred" way.) The reason I prefer things this way is because the approach is much more flexible over time and much, much easier to test. Single-use singleton objects are notoriously difficult to test, especially in mochitest land, where the results/state of previous tests influences the current test. With multi-use objects, you can instantiate a new object for each test and throw it away when done. No complex state management or cleanup needed! I'm kind of curious what the subsequent patches are going to look like. There are some comments I could make about API design, encapsulation of state, etc. But I'll hold off until I see those patches. I've left a lot of comments in the review. Don't be discouraged - this patch is pretty high quality as-is. But when it comes to code interacting with remote services, I raise the quality bar. I'll let some things slide due to time crunch of this feature. But I'll budge less when it comes to things that involve avoidable complexity later. ::: browser/app/profile/firefox.js @@ +1395,5 @@ > pref("browser.cache.auto_delete_cache_version", 1); > + > +// Telemetry experiments settings. > +pref("experiments.enabled", false); > +pref("experiments.manifest.interval", 86400); What's a "manifest interval?" Let's make this manifest.fetchIntervalSeconds. ::: browser/experiments/Experiments.js @@ +34,5 @@ > + > +/** > + * Returns the value of the pref is present, otherwise aDefault. > + */ > +function getBoolPref(aName, aDefault) { Use Preferences.jsm. @@ +45,5 @@ > + > +/** > + * Returns the value of the pref is present, otherwise aDefault. > + */ > +function getIntPref(aName, aDefault) { Use Preferences.jsm. @@ +74,5 @@ > + configureLogging(); > + > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + gExperimentsEnabled = getBoolPref(PREF_EXPERIMENTS_ENABLED, false); > + Services.prefs.addObserver("experiments.", this, false); Preferences.jsm has an observe() API that I prefer to the low-level APIs. In short, it allows you to define a single callback for a single preference. This makes your code much, much more readable and maintainable since the massive observe() function and corresponding switch statement goes away. @@ +76,5 @@ > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + gExperimentsEnabled = getBoolPref(PREF_EXPERIMENTS_ENABLED, false); > + Services.prefs.addObserver("experiments.", this, false); > + > + this._loadCachedManifest(); This constructor function will get executed very early in application startup. We should defer processing of the manifest until at least a few seconds after the profile is activated because: a) we don't want to do too much at startup, slowing down start times b) other services and data that experiment applicability depend on may not be fully initialized until after a profile is available See the pattern in toolkit/components/crashes/CrashService.js or services/datareporting/DataReportingService.js for delayed initialization implementations. @@ +93,5 @@ > + > + this.updateManifest(); > + }, > + > + observe: function Experiments_observe(aSubject, aTopic, aData) { Nit: You don't need to name functions that are assigned to properties or variables. SpiderMonkey will automatically inherit the name of the property or variable. (The reason we name functions is so stacks and other debugging info is useful.) The only time you need to name a function is when defining an in-line callback. e.g. doSomething(function onDoSomething() { ... }); Nit: This is JavaScript, not C++. We don't need to a-prefix variables that are arguments. @@ +115,5 @@ > + break; > + case PREF_LOGGING_LEVEL: > + case PREF_LOGGING_DUMP: > + configureLogging(); > + break; FWIW, bug 956817 tracks adding pref-based logging management to Log.jsm as a Gecko service. (We keep having to re-roll this logic in nearly every new service that uses Log.jsm.) @@ +133,5 @@ > + if (this._updateManifestTask) { > + return this._updateManifestTask; > + } > + > + let uri = Services.prefs.getCharPref(PREF_MANIFEST_URI); Use Preferences.jsm. @@ +137,5 @@ > + let uri = Services.prefs.getCharPref(PREF_MANIFEST_URI); > + this._updateManifestTask = Task.spawn(function Experiments_updateManifest_updateTask() { > + if (this._loadCachedManifestTask) { > + // Don't interfere while we're loading the cached manifest. > + yield this._loadCachedManifestTask; What if that promise is rejected? Should that interfere with us? @@ +141,5 @@ > + yield this._loadCachedManifestTask; > + } > + > + let responseText = yield this._httpGetRequest(uri); > + gLogger.debug("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\""); This could be a massive log message - potentially many 10's of KB! Do we really need to log the fully body? If we do, I'd do that at trace, not debug, level. FWIW, if you use rest.js, you can have it do all the logging for you! @@ +143,5 @@ > + > + let responseText = yield this._httpGetRequest(uri); > + gLogger.debug("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\""); > + yield this._cacheManifest(responseText); > + let data = JSON.parse(responseText); If JSON.parse() fails, we would have cached an invalid document. Let's not do that. @@ +148,5 @@ > + this._updateExperiments(data); > + this._updateManifestTask = null; > + }.bind(this)); > + > + return this._updateManifestTask; You can fold this return into the this._updateManifestTask assignment above. @@ +156,5 @@ > + * Helper function to make HTTP GET requests. Returns a promise that is resolved with > + * the responseText when the request is complete. > + */ > + _httpGetRequest: function Experiments_httpGetRequest(url) { > + let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest); While XHR is simple and easy to use, I don't believe it allows us to adjust network priority. While we're generally not good about it today, background services in Firefox should be using a lower network priority so they don't interfere with the user's web browsing. To do this, I believe we'll need to use the Necko APIs. services/common/rest.js provides a decent high-level API for performing HTTP requests. It doesn't yet support setting low network priority, but that should be easy to implement. See bug 961065. XHR can probably stay for now. Just know it will likely need changed eventually. @@ +172,5 @@ > + deferred.reject(new Error("Experiments - XHR error for " + url + " - " + e.error)); > + } > + > + xhr.onload = function Experiments_httpGetRequest_onLoad(event) { > + if (xhr.status !== 200) { Is XHR able to send an If-Modified-Since or If-None-Match request? Either way, we should be sending one of these to give the server the opportunity to issue an HTTP 304 Not Modified. This will save bandwidth for the user and Mozilla. Bandwidth costs money. A few KB multiplied by over 100,000,000+ really adds up and can amount to thousands of dollars in costs to Mozilla! @@ +176,5 @@ > + if (xhr.status !== 200) { > + Cu.reportError("Experiments::httpGetRequest::onLoad() - Request to " + url + " returned status " + xhr.status); > + deferred.reject(new Error("Experiments - XHR status for " + url + " is " + xhr.status)); > + return; > + } Do we want any retry logic here? (This is probably a bsmedberg question.) If we do want retry logic, I'd drop rnewman or me a line on IRC and ask about the thundering herd problem before you start implementing. @@ +190,5 @@ > + * Cache the experiments manifest file on disk. Returns a promise that is resolved > + * when the manifest was written to disk. > + */ > + _cacheManifest: function Experiments_cacheManifest(aResponseText) { > + let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_MANIFEST); FWIW, there is a race between profile initialization and this code running. Fix startup as noted above to make it go away. @@ +194,5 @@ > + let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_MANIFEST); > + return Task.spawn(function Experiments_cacheManifest_fileTask() { > + let encoder = new TextEncoder(); > + let data = encoder.encode(aResponseText); > + yield OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" }); It's not yet documented, but you can throw a |compression: "lz4"| in here to add lz4 compression. Consider it a best practice going forward to lz4 compress any compressible data before saving to disk. This is because we're almost always willing to trade CPU for I/O. This is because I/O is almost always the "weakest link" for our users. @@ +214,5 @@ > + // We're already updating the manifest, no need to load the cached version. > + return this._updateManifestTask; > + } > + > + let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_MANIFEST); That's twice I've seen this line. you should move this into an instance property or a getter. ::: browser/experiments/test/xpcshell/test_experiments.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; Just so you know, this has no effect in xpcshell tests because this file is effectively #include'd and "use strict"; must be the first statement to take effect. Many (including myself) type "use strict"; out of habit and out of hope that someday xpcshell tests will allow strict mode execution. @@ +26,5 @@ > +function run_test() { > + gProfileDir = do_get_profile(); > + > + gHttpServer = new HttpServer(); > + gHttpServer.start(4444); Hard-coded port numbers are light banned in xpcshell tests because they prevent tests from running concurrently (a race condition on opening the port is introduced). Always listen on a dynamically-chosen port: gHttpServer.start(-1); let port = gHttpServer.identify.primaryPort; @@ +30,5 @@ > + gHttpServer.start(4444); > + gHttpServer.registerDirectory("/", do_get_cwd()); > + do_register_cleanup(function cleanup() { > + gHttpServer.stop(function() {}); > + }); The following should work: do_register_cleanup(() => gHttpServer.stop(() => {})); @@ +39,5 @@ > + > + run_next_test(); > +} > + > +add_task(function test_fetchAndCache() { We should be using function* for all generator functions going forward. The old, *-less syntax is deprecated and will eventually raise a SyntaxError (or something like that). add_task(function* test_fetchAndCache() { }); @@ +43,5 @@ > +add_task(function test_fetchAndCache() { > + Services.prefs.setCharPref("experiments.manifest.uri", "http://localhost:4444/experiments.manifest"); > + let experiments = new Experiments(); > + > + do_check_null(experiments._experiments, "There should be no cached experiments yet."); Please use Assert.equal and other Assert.jsm functions for new tests. (We're trying to standardize on Assert.jsm for all our test suites so there is more unity between test suites.) @@ +58,5 @@ > + > + let error; > + yield experiments.updateManifest().then(() => error = false, () => error = true); > + do_check_true(error, "Updating the manifest should not have succeeded."); > + do_check_null(experiments._experiments, "There should still be no cached experiments."); I'd like to see more test coverage of failures. Notably: * Server responds with invalid JSON * Server sends a compressed response (something we'll likely do in production because it saves bandwidth) * 304 handling (when implemented) * Explicit checks for 404 and/or 500 response code

XHR has a (chrome-only) .channel accessor which gives you the nsIChannel after you've called .open but before .send which can be used to set the priority.

Thanks, that's good input! (In reply to Gregory Szorc [:gps] from comment #20) > Nit: This is JavaScript, not C++. We don't need to a-prefix variables that > are arguments. Hm, i don't really care either way, but the style guide still says we should use it in JavaScript and our code base is... ambivalent. > @@ +141,5 @@ > > + yield this._loadCachedManifestTask; > > + } > > + > > + let responseText = yield this._httpGetRequest(uri); > > + gLogger.debug("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\""); > > This could be a massive log message - potentially many 10's of KB! Do we > really need to log the fully body? If we do, I'd do that at trace, not > debug, level. Not neccessarily need, but it can be pretty helpful to quickly check on things. I'll happily comment out any logging that we don't want to stay active. > @@ +148,5 @@ > > + this._updateExperiments(data); > > + this._updateManifestTask = null; > > + }.bind(this)); > > + > > + return this._updateManifestTask; > > You can fold this return into the this._updateManifestTask assignment above. |return this._updateManifestTask = Task.spawn(...);|? I'd rather not fold here for readability reasons. > Is XHR able to send an If-Modified-Since or If-None-Match request? > > Either way, we should be sending one of these to give the server the > opportunity to issue an HTTP 304 Not Modified. This will save bandwidth for > the user and Mozilla. Bandwidth costs money. A few KB multiplied by over > 100,000,000+ really adds up and can amount to thousands of dollars in costs > to Mozilla! Per comment 18 i was assuming we don't care due to caching? > I'd like to see more test coverage of failures. Notably: > > * Server responds with invalid JSON > * Server sends a compressed response (something we'll likely do in > production because it saves bandwidth) > * 304 handling (when implemented) > * Explicit checks for 404 and/or 500 response code Hm, i'll check into extended tests later; i will have to find out what we can do with the HttpServer (i.e. re compression and explicit 304/404/500).

(In reply to Gregory Szorc [:gps] from comment #20) > @@ +176,5 @@ > > + if (xhr.status !== 200) { > > + Cu.reportError("Experiments::httpGetRequest::onLoad() - Request to " + url + " returned status " + xhr.status); > > + deferred.reject(new Error("Experiments - XHR status for " + url + " is " + xhr.status)); > > + return; > > + } > > Do we want any retry logic here? (This is probably a bsmedberg question.) Seems like the daily (or similar) fetch interval should be enough?

I don't think retry is necessary for now.

(In reply to Georg Fritzsche [:gfritzsche] from comment #22) > Hm, i don't really care either way, but the style guide still says we should > use it in JavaScript and our code base is... ambivalent. [derail] Yeah, the coding standard document documents ancient coding standards :D It's pretty stale. Always brace, 2 spaces for JS, no argument prefixing. We've historically followed the coding standard of existing files, so if you spend all of your time in toolkit you'll still have to deal with aFail. We've actually just ditched prefixing in Java, too, fwiw. The nail in the coffin was a static member named "mFoo", which nobody noticed in six months.

Fixed the network priority & the parameter style now. gps, could you take another look before we base more on this? This should address all the above comments except having more tests (i don't want to block on them yet).

Comment on attachment 8386963 [details] [diff] [review] Fix network priority and parameters It looks like Unfocused will do the reviews here. Please note that i still need to extend the tests for this later on. Removing f?gps as i think i addressed his comments already.

Comment on attachment 8386963 [details] [diff] [review] Fix network priority and parameters Review of attachment 8386963 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/experiments/Experiments.js @@ +26,5 @@ > +const PREF_LOGGING_DUMP = PREF_LOGGING + ".dump"; > +const PREF_MANIFEST_URI = "manifest.uri"; > +const MAX_MANIFEST_VERSION = 1; > + > +const prefs = new Preferences("experiments."); Nit: gPrefs @@ +33,5 @@ > +let gLogger = null; > +let gLogAppenderDump = null; > + > +XPCOMUtils.defineLazyGetter(this, "gEncoder", function() { return new gChromeWin.TextEncoder(); }); > +XPCOMUtils.defineLazyGetter(this, "gDecoder", function() { return new gChromeWin.TextDecoder(); }); You're not using these. But if you were, I'd tell you that you didn't need them. @@ +35,5 @@ > + > +XPCOMUtils.defineLazyGetter(this, "gEncoder", function() { return new gChromeWin.TextEncoder(); }); > +XPCOMUtils.defineLazyGetter(this, "gDecoder", function() { return new gChromeWin.TextDecoder(); }); > + > +function configureLogging() { Don't need to reconfigure this every time you construct a new instance of Experiments. @@ +60,5 @@ > + _experiments: null, > + _updateManifestTask: null, > + _loadCachedManifestTask: null, > + > + classID: Components.ID("{f7800463-3b97-47f9-9341-b7617e6d8d49}"), Don't need this, it's not a component. @@ +131,5 @@ > + let responseText = yield this._httpGetRequest(uri); > + gLogger.trace("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\""); > + > + try { > + let data = JSON.parse(responseText); You need to do some sanity checking on the data you're receiving here. @@ +152,5 @@ > + */ > + _httpGetRequest: function (url) { > + let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest); > + try { > + xhr.open("GET", url, true); Please kill the third argument - any time I see a third argument there I wonder why it's synchronous XHR... presence of the third argument for that method should always stand out and being a big bad warning sign. Don't dilute that please :) @@ +166,5 @@ > + deferred.reject(new Error("Experiments - XHR error for " + url + " - " + e.error)); > + } > + > + xhr.onload = function (event) { > + if (xhr.status !== 200) { Crazily, you also have to check this isn't 0. @@ +217,5 @@ > + } > + > + let path = this._manifestCachePath; > + this._loadCachedManifestTask = Task.spawn(function () { > + let result = yield AppsUtils.loadJSONAsync(path, { compression: "lz4" }); I don't think you should rely on this - it's an unrelated module that's not designed to be used as a generic utility module by outside code. As such, they won't be forgiving when it comes to breaking things from underneath you. And then there's the likes of bug 980379 comment 2 too. Better to just adapt that code into your own function here (and file a followup bug to add a truly generic module to toolkit). As a bonus, that also means you can avoid the bug I just spotted in that function ;) (bug 981563) ::: browser/experiments/ExperimentsService.js @@ +4,5 @@ > + > +"use strict"; > + > +this.EXPORTED_SYMBOLS = [ > + "ExperimentsService", Is this a module or a service? (Pretty sure it should be just a service) ::: browser/experiments/moz.build @@ +2,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +EXTRA_COMPONENTS += [ > + 'Experiments.js', Pretty sure this is a module... (Should be in EXTRA_JS_MODULES, and be a .jsm)

Good points, thanks. (In reply to Blair McBride [:Unfocused] from comment #29) > ::: browser/experiments/ExperimentsService.js > @@ +4,5 @@ > > + > > +"use strict"; > > + > > +this.EXPORTED_SYMBOLS = [ > > + "ExperimentsService", > > Is this a module or a service? (Pretty sure it should be just a service) Right, it became a service out of necessity (for the update timer etc.). Is there anything i need to change to reflect that? Remove the export?

(In reply to Blair McBride [:Unfocused] from comment #29) > > +function configureLogging() { > > Don't need to reconfigure this every time you construct a new instance of > Experiments. Actually, we do only create one instance, except in tests, so i'm not sure if i want to worry about "nicer" initialization.

(In reply to Blair McBride [:Unfocused] from comment #29) > @@ +166,5 @@ > > + deferred.reject(new Error("Experiments - XHR error for " + url + " - " + e.error)); > > + } > > + > > + xhr.onload = function (event) { > > + if (xhr.status !== 200) { > > Crazily, you also have to check this isn't 0. And one more... do i need to here? I can't find proper documentation on when to expect what, and in our tree i see only 0-checks for xhr.onreadystatechange - the onload handlers in properly working modules seem fine with just a 200 check.

(In reply to Georg Fritzsche [:gfritzsche] from comment #30) > Is there anything i need to change to reflect that? Remove the export? Just remove the export. (In reply to Georg Fritzsche [:gfritzsche] from comment #32) > in our tree i see only 0-checks for > xhr.onreadystatechange - the onload handlers in properly working modules > seem fine with just a 200 check. Oh, huh. Ok! Seems fine without then.

This got folded into bug 974009 because it was too hard to keep the changes apart. Not sure to resolve as what, but i want to see this bug closed.