Webapps.jsm is a mess! Parts of the code is nested multiple (7) layers deep in anonymous functions and is pretty incomprehensible. In order to make the code easier to understand, the flow of execution better and any subsequent work faster and simpler a serious effort needs to be spent on this code. I have done just that! I've refactored large parts of this file to address all of the above points. It's important to note that no behavioral changes have taken place, this is purely an exercise in moving and tidying code. Myk has taken 2 passes through this code already and has offered suggestions and improvements which have been factored in.

This is a large refactoring with no automated tests included in the patch, which makes this really risky as it stands. We needs tests here.

The patch is reverting some changes that landed in the last few days, updating the tree would make it pretty smaller.

Martyn, see also bug 801610 where I was doing some refactoring work. I see your code mostly touches the package download code and the confirmInstall function, in my patch instead I was modifying the registry initialization and some other functions using promises and Task.jsm. So, I think our refactorings shouldn't conflict too much. I like the simplifications that you're doing, but maybe using promises and Task.jsm you could avoid the callback mess without creating functions with too many parameters (for example _onOpenSignedJarFileAsyncCallback has 12 parameters :D). I feel like splitting functions too much is overkill, if they only have a single caller.

(In reply to Marco Castelluccio [:marco] from comment #3) > I feel like splitting functions too much is overkill, if they only have a > single caller. I will vote in favor of splitting functions if it means we can write more or better unit tests, or if it enables the new functionality Martyn is about to implement.

(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #4) > I will vote in favor of splitting functions if it means we can write more or > better unit tests, or if it enables the new functionality Martyn is about to > implement. Which functionality is it? That would help making decisions if we know what's coming before the patch is written.

(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #4) > (In reply to Marco Castelluccio [:marco] from comment #3) > > I feel like splitting functions too much is overkill, if they only have a > > single caller. > > I will vote in favor of splitting functions if it means we can write more or > better unit tests, or if it enables the new functionality Martyn is about to > implement. Sure, but in this case I think the new functions are too much coupled. Some are really meant to be called sequentially and, to avoid the massive callback nesting, I think it'd be better to use promises and Task.jsm that would allow a much clearer flow. Anyway, we should design a clear interface for the registry based on our needs. I was thinking to split the installation flow in two main functions, one that downloads the needed resources and the other that performs the actual installation. This way if we already have the resources (for example on Android if we download an APK with the manifest, the package, etc.), we could directly call the second function. This would meet our needs on desktop for bug 906223. Does this meet our needs on Android?

Comment on attachment 797381 [details] [diff] [review] refactor(1).diff Review of attachment 797381 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review since I heard you are working on a new version.

(In reply to Marco Castelluccio [:marco] from comment #6) > I was thinking to split the installation flow in two main functions, one > that downloads the needed resources and the other that performs the actual > installation. > This way if we already have the resources (for example on Android if we > download an APK with the manifest, the package, etc.), we could directly > call the second function. This would meet our needs on desktop for bug > 906223. Does this meet our needs on Android? Yes, I think that would meet our needs on Android, where the new APK-centric implementation will download and (natively) install an APK when the user asks Fennec to install an app, and we'll want to make sure the APK was successfully installed before we register the app in the registry. So I would split the flow into something like an "install" function that triggers the installation process and a "register" function that adds the app to the registry once it has been installed successfully.

(In reply to Fabrice Desré [:fabrice] from comment #7) > Clearing review since I heard you are working on a new version. Indeed, Martyn has been using Promise and Task to simplify the code flows. He's nearly done, and I'm going to take his work, put any necessary finishing touches on it, and then attach a new version for review shortly!

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9) > (In reply to Fabrice Desré [:fabrice] from comment #7) > > Clearing review since I heard you are working on a new version. > > Indeed, Martyn has been using Promise and Task to simplify the code flows. > He's nearly done, and I'm going to take his work, put any necessary > finishing touches on it, and then attach a new version for review shortly! Myk, see also bug 801610 that contains a small refactoring (the minimum necessary to move most IO off the main-thread).

(In reply to Marco Castelluccio [:marco] from comment #10) > Myk, see also bug 801610 that contains a small refactoring (the minimum > necessary to move most IO off the main-thread). Noted! Heh, that's "small" the same way this one is, i.e. not very. And it looks like the two sets of changes conflict, unfortunately, so we're going to be in merge hell. :-/

Well, my refactoring was only meant to move the IO off the main thread and not to really improve the code (improving the code was a nice side effect). So I guess it is less important than this one.

(In reply to Marco Castelluccio [:marco] from comment #12) > Well, my refactoring was only meant to move the IO off the main thread and > not to really improve the code (improving the code was a nice side effect). > So I guess it is less important than this one. I'm not so sure, but this one seems like it'll help implement us the Android-specific changes to the install flow that are a high priority this quarter.

Here's an updated patch that is ready for review. As with the earlier patch, there are no behavioral changes; this just refactors parts of Webapps.jsm, mainly doInstallPackage, to improve readability and maintainability. This updated patch now uses Promises and Task to improve the flow, with a downloadPackage task that drives the download and verification process. But it still segregates parts of that process in their own functions, so that each part, and the overall task, are a manageable size that can be read and comprehended as a whole. Think of the overarching task as an outline that gives you a sense of the overall download process, while each function it calls shows the details of a discrete part of the process. In addition to shuffling code around, we made a few other key changes: * We renamed app and appObject variables to oldApp and newApp, to clarify the distinction between those two objects. * We improved the consistency of the code style, following the conventions of the file as well as the Mozilla style guide <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide>. * We removed names from function expressions we added or moved, as the DevTools team says they're no longer useful. And a couple extraneous ones (that could be done separately): * We automatically enabled the debug() function if MOZ_DEBUG is set, so it works in debug builds, and it isn't necessary to make a code change to get debug output. * We fixed a typo in a test that causes a variable reference to fail when attempting to report a test failure. The patch has been rebased against the latest code in mozilla-central. It passes all tests in dom/apps/tests/ as well as dom/tests/mochitest/webapps/. It also passes the new tests in the patch in bug 880043. Due to the limitations of diff generators with changes like this one, some of the functions in the patch will be easier to review by reading the code with the patch applied rather than the patch itself, particularly downloadPackage.

Comment on attachment 822458 [details] [diff] [review] patch v2: uses Promises/Task, makes other misc improvements Review of attachment 822458 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, that's a massive one! A did a first pass, but I'm far from being done. Note that /tests/dom/apps/tests/test_packaged_app_update.html in M2 is failing with this patch. Given how extensive the changes are, I'll also give a build to QA for a thorough smoketest before landind anything. ::: dom/apps/src/Webapps.jsm @@ +2142,5 @@ > delete this.queuedPackageDownload[aManifestURL]; > > + this.downloadPackage(manifest, newApp, false, > + this._onDownloadPackage.bind(this, newApp, installSuccessCallback) > + ); nit: why not turn downloadPackage into a promise? @@ +2172,3 @@ > > + let appNote = JSON.stringify(appObject); > + appNote.id = aId; I think we don't use appNote anymore. Just remove it if so, it used to be there when we had some sync support. @@ +2203,5 @@ > > + return [appNote, appObject]; > + }, > + > + _copyStates: function(aData, aAppObject) { change the parameter names to make clear what we copy where. something like _copyStates(aFrom, aTo) @@ +2301,4 @@ > if (!aData.isPackage) { > this.updateAppHandlers(null, app.manifest, app); > if (aInstallSuccessCallback) { > + aInstallSuccessCallback(manifest); why manifest and not app.manifest? @@ +2347,5 @@ > + _onDownloadPackage: function(aNewApp, aInstallSuccessCallback, aId, > + aManifest) { > + debug("_onDownloadPackage"); > + // Success! Move the zip out of TmpD. > + let app = DOMApplicationRegistry.webapps[aId]; nit: can probably be this.webapps[aId] now @@ +2865,2 @@ > > + _openSignedJarFile: function(aOldApp, aZipFile) { nit: s/aOldApp/aApp @@ +2889,4 @@ > > + _readPackage: function(aRv, aZipReader, aZipFile, aOldApp, aNewApp, > + aIsLocalFileInstall, aIsUpdate, aManifest, > + aRequestChannel, aHash) { 10 parameters.... I'd like to avoid that. @@ +2899,5 @@ > + throw "INVALID_SIGNATURE"; > + } else { > + isSigned = false; > + aZipReader = Cc["@mozilla.org/libjar/zip-reader;1"] > + .createInstance(Ci.nsIZipReader); so, we don't need aZipReader as a parameter?

(In reply to Fabrice Desré [:fabrice] from comment #16) > Thanks, that's a massive one! We're just getting started! ;-) > A did a first pass, but I'm far from being done. Note that > /tests/dom/apps/tests/test_packaged_app_update.html in M2 is failing with > this patch. It looks like intermittent failure bug 915879, which is Windows-specific. I haven't tried reproducing on Windows yet, but I'll do so shortly. Ideally, these changes will have made the intermittent failure be consistent, since then I can figure it out and fix it! I won't ask for re-review until I've investigated this, but in the meantime I addressed your other comments… > Given how extensive the changes are, I'll also give a build to > QA for a thorough smoketest before landind anything. Good idea! > ::: dom/apps/src/Webapps.jsm > @@ +2142,5 @@ > > delete this.queuedPackageDownload[aManifestURL]; > > > > + this.downloadPackage(manifest, newApp, false, > > + this._onDownloadPackage.bind(this, newApp, installSuccessCallback) > > + ); > > nit: why not turn downloadPackage into a promise? Nice catch, it actually does return a promise, I just hadn't updated the callers to use it. Fixed! https://github.com/mykmelez/mozilla-central/commit/4c3a837c743a9f015cf24118b8872fb1b30d6a3f > @@ +2172,3 @@ > > > > + let appNote = JSON.stringify(appObject); > > + appNote.id = aId; > > I think we don't use appNote anymore. Just remove it if so, it used to be > there when we had some sync support. Indeed, Marco removed it in <http://hg.mozilla.org/mozilla-central/rev/6ca07e89167e>, so this must be a remnant from a subsequent merge. Fixed! https://github.com/mykmelez/mozilla-central/commit/7213aa0bf092597d055eae1e692a1a0b6bd31065 > @@ +2203,5 @@ > > > > + return [appNote, appObject]; > > + }, > > + > > + _copyStates: function(aData, aAppObject) { > > change the parameter names to make clear what we copy where. something like > _copyStates(aFrom, aTo) Hmm, good point, but we ended up using this function only once, so I have un-refactored it, as its body isn't substantial enough to encapsulate in the absence of multiple callers. https://github.com/mykmelez/mozilla-central/commit/098a9a59e46fbfc5ffde5a9dbdffcadca60c49da > @@ +2301,4 @@ > > if (!aData.isPackage) { > > this.updateAppHandlers(null, app.manifest, app); > > if (aInstallSuccessCallback) { > > + aInstallSuccessCallback(manifest); > > why manifest and not app.manifest? Hmm, that looks like a mistake. Undone! https://github.com/mykmelez/mozilla-central/commit/f0c5c0dfcc63ba282c6aa7321773211050a30c03 > @@ +2347,5 @@ > > + _onDownloadPackage: function(aNewApp, aInstallSuccessCallback, aId, > > + aManifest) { > > + debug("_onDownloadPackage"); > > + // Success! Move the zip out of TmpD. > > + let app = DOMApplicationRegistry.webapps[aId]; > > nit: can probably be this.webapps[aId] now Indeed, done. https://github.com/mykmelez/mozilla-central/commit/16166924ba1e9aadd082a3b5508b3b9833f53cf7 > @@ +2865,2 @@ > > > > + _openSignedJarFile: function(aOldApp, aZipFile) { > > nit: s/aOldApp/aApp Ok, fixed (on top of the fix described below). https://github.com/mykmelez/mozilla-central/commit/d59bec298d09201239807927698740d1046319ec > @@ +2889,4 @@ > > > > + _readPackage: function(aRv, aZipReader, aZipFile, aOldApp, aNewApp, > > + aIsLocalFileInstall, aIsUpdate, aManifest, > > + aRequestChannel, aHash) { > > 10 parameters.... I'd like to avoid that. Me too! Unfortunately, the alternatives are unpalatable. Our goal has been to break up the long, nested downloadPackage implementation into a set of smaller functions, each of which can be held in one's mind and reasoned about as a whole. That's hardest for downloadPackage itself, which is the longest of the bunch and still longer than it should be. And the only way I can see to avoid giving _readPackage a bunch of parameters is to make downloadPackage even longer by unrefactoring the code I factored out into _readPackage. > @@ +2899,5 @@ > > + throw "INVALID_SIGNATURE"; > > + } else { > > + isSigned = false; > > + aZipReader = Cc["@mozilla.org/libjar/zip-reader;1"] > > + .createInstance(Ci.nsIZipReader); > > so, we don't need aZipReader as a parameter? This is a different ZIP reader. We use it in place of the reader in the parameter if we discover that the JAR is unsigned. But your question led me to realize that we can move more package opening/reading out of downloadPackage, so I've moved the rest of that code into an _openAndReadPackage function (with _openPackage, _openSignedPackage, and _readPackage dependents for various parts of the process). This also reduces the number of parameters for _openAndReadPackage to eight, although _readPackage still takes nine. Nevertheless, it makes downloadPackage a lot shorter and easier to read, and _openAndReadPackage et. al. are also quite reasonable in size and quite a bit more readable. I think it's worth the parameter count. In the process, I also started using ES6 generators (function*) to improve readability further, because you can *return* at the end of them instead of having to throw a new Task.Result! https://github.com/mykmelez/mozilla-central/commit/9972ea7bb4bf9c58a251b7a8f25382dd94a368d2

(In reply to Myk Melez [:myk] [@mykmelez] from comment #17) > > (In reply to Fabrice Desré [:fabrice] from comment #16) > > > A did a first pass, but I'm far from being done. Note that > > /tests/dom/apps/tests/test_packaged_app_update.html in M2 is failing with > > this patch. > > It looks like intermittent failure bug 915879, which is Windows-specific. I > haven't tried reproducing on Windows yet, but I'll do so shortly. Ideally, > these changes will have made the intermittent failure be consistent, since > then I can figure it out and fix it! Actually, the problem was that we weren't closing the package file after reading it to compute its hash. This patch fixes that problem and uses a task to further simplify the code in downloadPackage and _computeFileHash. https://github.com/mykmelez/mozilla-central/commit/4fe66368efcb71d4450e5af52d8ba015d38e8225 Here's the try run: https://tbpl.mozilla.org/?tree=Try&rev=2885f1330000 I also tested locally (mochitests on Firefox, manual tests in B2G Desktop).

And here's the patch. I think this is ready for review.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #18) > Here's the try run: > > https://tbpl.mozilla.org/?tree=Try&rev=2885f1330000 The handful of failures in this try run all look like the usual intermittent ones.

Comment on attachment 8337120 [details] [diff] [review] 910815v4.diff Review of attachment 8337120 [details] [diff] [review]: ----------------------------------------------------------------- r=me once we agree on the debug part. Not sure if I'm excited or scared ;) ::: dom/apps/src/Webapps.jsm @@ +34,5 @@ > > function debug(aMsg) { > +#ifdef MOZ_DEBUG > + dump("-*- Webapps.jsm : " + aMsg + "\n"); > +#endif With this change we need a build wide change (like --enable-debug) to get debug info there? If so, I'd like something more flexible, like: #ifdef MOZ_DEBUG const WEBAPPS_DEBUG = true; #else const WEBAPPS_DEBUG = false; #endif function debug(aMsg) { if (WEBAPPS_DEBUG) { dump("-*- Webapps.jsm : " + aMsg + "\n"); } } @@ +2240,5 @@ > + let jsonManifest = aData.isPackage ? app.updateManifest : app.manifest; > + this._writeManifestFile(id, aData.isPackage, jsonManifest); > + > + debug("jsonManifest: " + JSON.stringify(jsonManifest)); > + debug("app.origin: " + app.origin); nit: stringify is expensive, so we could use this opportunity to do : if (WEBAPPS_DEBUG) { debug(...); debug(...); }

(In reply to Fabrice Desré [:fabrice] from comment #21) > ::: dom/apps/src/Webapps.jsm > @@ +34,5 @@ > > > > function debug(aMsg) { > > +#ifdef MOZ_DEBUG > > + dump("-*- Webapps.jsm : " + aMsg + "\n"); > > +#endif > > With this change we need a build wide change (like --enable-debug) to get > debug info there? Currently, you have to make this source code change to enable debug logging: function debug(aMsg) { - //dump("-*-*- Webapps.jsm : " + aMsg + "\n"); + dump("-*-*- Webapps.jsm : " + aMsg + "\n"); } But this is cumbersome, because it means you have to make that (unrelated) change to the source code every time you start developing a patch and then remove it before producing the patch. So I added the ability to enable logging via the same build flag that enables logging in a bunch of C++ code: ac_add_options --enable-debug Nevertheless, it's still possible to enable logging via a source code change, although it requires changing two lines instead of one: function debug(aMsg) { - #ifdef MOZ_DEBUG + //#ifdef MOZ_DEBUG dump("-*- Webapps.jsm : " + aMsg + "\n"); - #endif + //#endif } So I'm actually adding flexibility, not taking it away. And only slightly increasing code complexity. > If so, I'd like something more flexible, like: > > #ifdef MOZ_DEBUG > const WEBAPPS_DEBUG = true; > #else > const WEBAPPS_DEBUG = false; > #endif > > function debug(aMsg) { > if (WEBAPPS_DEBUG) { > dump("-*- Webapps.jsm : " + aMsg + "\n"); > } > } It's unclear how this adds flexibility, since it still requires either a build configuration change or a source code change to enable logging, and the source code change is just as complex, requiring you to comment out at least two lines: function debug(aMsg) { - if (WEBAPPS_DEBUG) { + //if (WEBAPPS_DEBUG) { dump("-*- Webapps.jsm : " + aMsg + "\n"); - } + //} } If you want to increase flexibility, then I could implement a runtime configuration flag (i.e. a hidden pref), so developers and users could enable logging on any build. Alternately, if you just want it to be as simple as possible to make the source code change, then I could do something like this: #ifdef MOZ_DEBUG const WEBAPPS_DEBUG = true; #else const WEBAPPS_DEBUG = false; #endif function debug(aMsg) { // Uncomment this to enable logging regardless of build-time configuration. //let WEBAPPS_DEBUG = true; if (WEBAPPS_DEBUG) { dump("-*- Webapps.jsm : " + aMsg + "\n"); } } Then it'll remain a one-line change instead of becoming a two-line change. I'm game for either (or both!). And I'm also happy to spin this off into a separate bug if you'd like to think about it more. Which would you prefer? > @@ +2240,5 @@ > > + let jsonManifest = aData.isPackage ? app.updateManifest : app.manifest; > > + this._writeManifestFile(id, aData.isPackage, jsonManifest); > > + > > + debug("jsonManifest: " + JSON.stringify(jsonManifest)); > > + debug("app.origin: " + app.origin); > > nit: stringify is expensive, so we could use this opportunity to do : > if (WEBAPPS_DEBUG) { > debug(...); > debug(...); > } Nice catch! But logging an entire JSON blob is probably not a good idea anyway, since it could be really long, and it won't be particularly easy to read. In the Android log, it'll also get cut off after a certain number of characters (which actually confused me mightily at one point in my debugging process). How about instead I simply remove that debug statement?

(In reply to Myk Melez [:myk] [@mykmelez] from comment #22) > Nevertheless, it's still possible to enable logging via a source code > change, although it requires changing two lines instead of one: > > function debug(aMsg) { > - #ifdef MOZ_DEBUG > + //#ifdef MOZ_DEBUG > dump("-*- Webapps.jsm : " + aMsg + "\n"); > - #endif > + //#endif > } > > So I'm actually adding flexibility, not taking it away. And only slightly > increasing code complexity. Ok, let's keep it this way. > Nice catch! But logging an entire JSON blob is probably not a good idea > anyway, since it could be really long, and it won't be particularly easy to > read. In the Android log, it'll also get cut off after a certain number of > characters (which actually confused me mightily at one point in my debugging > process). > > How about instead I simply remove that debug statement? I'm fine with removing it entirely, along with the other stringify used in debug().

(In reply to Fabrice Desré [:fabrice] from comment #23) > > Nice catch! But logging an entire JSON blob is probably not a good idea > > anyway, since it could be really long, and it won't be particularly easy to > > read. In the Android log, it'll also get cut off after a certain number of > > characters (which actually confused me mightily at one point in my debugging > > process). > > > > How about instead I simply remove that debug statement? > > I'm fine with removing it entirely, along with the other stringify used in > debug(). I found two other instances of JSON.stringify() in debug(), both of which predate these changes. I've removed them. Aside: I also see a number of JSON.stringify() calls passed to the "data" parameter of nsIOberverService.notifyObservers(). If they have performance implications, and the observers are JavaScript, then we could address them by passing the JSON object via the wrappedJSObject property of an nsISupports "subject", i.e. something like: Services.obs.notifyObservers({ wrappedJSObject: jsonObject }, "…", null); (The anonymous "subject" might need a QueryInterface method; unsure.) Carrying forward review, and this is the patch I'll land.

Fabrice mentioned to me in person to do some exploratory testing on this to check for any critical regressions, so I'll take a look into this.

Awesome, thanks Jason!