Closed Bug 1315407 Opened 8 years ago Closed 8 years ago

Refactor AddonInstall

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

Bug 1315407 Revise tracking of active AddonInstalls
58 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1315407 Refactor AddonInstall
58 bytes, text/x-review-board-request
rhelmer
: review+
Details
Bug 1315407 Fix sdk addon-install
58 bytes, text/x-review-board-request
mossop
: review+
Details 
AddonInstall handles both local file installs and download installs, plus it uses lots of coding style techniques that are not up to our current standards.  This bug is to address bringing AddonInstall up to our current standards.
Comment on attachment 8807793 [details]
Bug 1315407 Revise tracking of active AddonInstalls

Here are a couple of patches for initial feedback, I'll wait for bug 1204156 to land and then get this rebased onto that.  Tests are passing for me locally and of course I'll run it through try.
I've tried to preserve all the current behavior, so I didn't make any changes with regard to things like updates from local files for instance.  I did notice one quirk though: in the current implementation if a local file install is canceled and then resumed, it goes through startDownload() which ... works I guess?  But it makes an unnecessary copy of the file and I can't imagine that it was desired behavior.  Anyhow, the download logic is now all on the subclass of AddonInstall that handles installs that start life as downloads so that sequence can no longer occur.
Attachment #8807793 - Flags: feedback?(rhelmer)
Attachment #8807793 - Flags: feedback?(dtownsend)
Attachment #8807794 - Flags: feedback?(rhelmer)
Attachment #8807794 - Flags: feedback?(dtownsend)
I need some time to look more thoroughly but the overall direction looks great!

(In reply to Andrew Swan [:aswan] from comment #3)
> I've tried to preserve all the current behavior, so I didn't make any changes
> with regard to things like updates from local files for instance.

Do you mean how `createDownload()` can take both a URL and a nsIFileURL, and calls the local or download install methods?

I bet there are quite a few callers of `AddonInstall.createDownload` so I can understand why you don't want to have to deal with changing the behavior in all of them...

Anyway if I got that all right then I agree, and there should be a followup bug to move all the callers to `createDownloadInstall` and `createLocalInstall` as appropriate, move the former to promises instead of callback etc. but that seems like too much churn for this bug.

> I did notice one quirk though: in the current implementation if a local file
> install is canceled and then resumed, it goes through startDownload() which
> ... works I guess?  But it makes an unnecessary copy of the file and I can't
> imagine that it was desired behavior.  Anyhow, the download logic is now all
> on the subclass of AddonInstall that handles installs that start life as
> downloads so that sequence can no longer occur.

I can't imagine there's anything relying on the download sequence, except by accident... I guess there could be code that expects to see `onDownloadEnded` when cancel is resumed or something?

This feels similar to the first case - it's confusing that a "download" type of AddonInstall can be created, but in actuality be a "local file" type of AddonInstall and callers don't really need to differentiate, so they don't necessarily know what events to expect etc.

I still agree with straightening that out as a followup though.
(In reply to Robert Helmer [:rhelmer] from comment #4)
> (In reply to Andrew Swan [:aswan] from comment #3)
> > I've tried to preserve all the current behavior, so I didn't make any changes
> > with regard to things like updates from local files for instance.
> 
> Do you mean how `createDownload()` can take both a URL and a nsIFileURL, and
> calls the local or download install methods?

I actually meant how `createUpdate()` does what you describe.  I think that behavior in `createDownload()` is desirable and createDownload is just the wrong name -- that's what calls to `AddonManager.getInstallForURL()` eventually turn into and having that single entry point handle different types of urls and doing the switch internally for file: vs protocols that hit the network seems like a good thing...
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review90976

I didn't look too deep here but this approach looks fine to me.
Attachment #8807794 - Flags: review+
Attachment #8807794 - Flags: review+
Attachment #8807794 - Flags: feedback?(dtownsend)
Attachment #8807794 - Flags: feedback+
Attachment #8807793 - Flags: feedback?(dtownsend) → feedback+
Comment on attachment 8807793 [details]
Bug 1315407 Revise tracking of active AddonInstalls

https://reviewboard.mozilla.org/r/90818/#review91032

This is a nice improvement!

::: toolkit/mozapps/extensions/test/xpcshell/test_install.js:738
(Diff revision 1)
>  
>      do_check_eq(installs[5].state, AddonManager.STATE_DOWNLOAD_FAILED);
>      do_check_eq(installs[5].error, AddonManager.ERROR_CORRUPT_FILE);
>  
>      AddonManager.getAllInstalls(function(aInstalls) {
> +      dump(`states ${aInstalls.map(i => i.state)}\n`);

don't forget to remove :)
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review90558

I like your approach. I just have some comments on code that you moved around that is no fault of your own, except for the one about `this.browser` and `this.window`.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5873
(Diff revision 1)
> +      }
> +      else {
> +        // The install is completed so it should be removed from the active list
> +        XPIProvider.removeActiveInstall(this);
> +
> +        // TODO We can probably reduce the number of DB operations going on here

We should have a bug # if we're going to leave this TODO so it's actionable.

Also this seems to be totally different from the second sentence in the comment which does have its own bug # (which is resolved as it happens)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5875
(Diff revision 1)
> +        // The install is completed so it should be removed from the active list
> +        XPIProvider.removeActiveInstall(this);
> +
> +        // TODO We can probably reduce the number of DB operations going on here
> +        // We probably also want to support rolling back failed upgrades etc.
> +        // See bug 553015.

This bug is duped to bug 587088 which is resolved - I wonder if this part of the comment should be removed.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6129
(Diff revision 1)
> +        applyBlocklistChanges(addon, this.addon);
> +      this.addon.updateDate = Date.now();
> +      this.addon.installDate = addon ? addon.installDate : this.addon.updateDate;
> +
> +      if (!this.addon.isCompatible) {
> +        // TODO Should we send some event here?

If we want to do this we should file a bug, or if not then just remove this comment.

I guess the event would be something meaning "an update check was initiated because the addon was incompatible"?

If we don't have any ideas for listeners of such an event right now, maybe we should just log a message here instead.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6607
(Diff revision 1)
> +  }
>  
> -  getInterface: function(iid) {
> +  getInterface(iid) {
>      if (iid.equals(Ci.nsIAuthPrompt2)) {
> -      let win = this.window;
> -      if (!win && this.browser)
> +      let win = null;
> +      if (this.browser) {



::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6703
(Diff revision 1)
> -AddonInstall.createDownload = function(aCallback, aUri, aHash, aName, aIcons,
> +function createDownloadInstall(aCallback, aUri, aHash, aName, aIcons,
> -                                       aVersion, aBrowser) {
> +                               aVersion, aBrowser) {
>    let location = XPIProvider.installLocationsByName[KEY_APP_PROFILE];
>    let url = NetUtil.newURI(aUri);
>  
> -  let install = new AddonInstall(location, url, aHash, null, null, aBrowser);
> +  if (url instanceof Ci.nsIFileURL) {

Hm calling `createDownloadInstall` and passing it a `nsIFileURL` seems really odd, since there's a `createLocalInstall` method too... 

If you don't want to have to deal with that level of change for all calling code then that makes sense.
Attachment #8807793 - Flags: feedback?(rhelmer) → feedback+
Attachment #8807794 - Flags: feedback?(rhelmer) → feedback+
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review90558

> We should have a bug # if we're going to leave this TODO so it's actionable.
> 
> Also this seems to be totally different from the second sentence in the comment which does have its own bug # (which is resolved as it happens)

This code was just moved from one place to another though mozreview didn't figure that out.
But yeah, the reference to "DB operations" seems quite dated, everything that touches XPIDatabase these days just writes the whole thing as a big json blob, and that's what happens here, I don't think we can realistically do anything lighter (and even if we could I don't think it would be worth it)
And yep the second comment also seems obsolete.
I think the comment should just be removed...

> If we want to do this we should file a bug, or if not then just remove this comment.
> 
> I guess the event would be something meaning "an update check was initiated because the addon was incompatible"?
> 
> If we don't have any ideas for listeners of such an event right now, maybe we should just log a message here instead.

This is a guess, but I suspect that comment was about potentially putting something in the UI to indicate that the install has gone into this state.  We've lived without it for a while and nobody has complained so I'm inclined to just remove the comment.  If somebody wants to add that to the UI, it will be fairly obvious that they need to notify the UI when this transition happens, even without the comment there as a landmark.

> 

you also referenced this in your initial comment but i just see any empty comment here...?

> Hm calling `createDownloadInstall` and passing it a `nsIFileURL` seems really odd, since there's a `createLocalInstall` method too... 
> 
> If you don't want to have to deal with that level of change for all calling code then that makes sense.

yeah, I mentioned this over on the bug, but this is what `AddonManager.getInstallForURL()` turns into, and that method supports file: urls.  I could rename it to something like createInstallForURL?  (or FromURL?)
Comment on attachment 8807793 [details]
Bug 1315407 Revise tracking of active AddonInstalls

https://reviewboard.mozilla.org/r/90818/#review92066

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5463
(Diff revision 2)
>      this.addon = aManifest;
> +    this.addon.sourceURI = this.sourceURI;
>  
>      this.state = AddonManager.STATE_INSTALLED;
>  
>      XPIProvider.installs.push(this);

s/push/add/

Do we have any test coverage for `createStagedInstall` (which calls this)?

It's only during failure of `processPendingFileChanges` it looks like.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5500
(Diff revision 2)
>        }
>        catch (e) {
>          logger.warn("Unknown hash algorithm '" + this.hash.algorithm + "' for addon " + this.sourceURI.spec, e);
>          this.state = AddonManager.STATE_DOWNLOAD_FAILED;
>          this.error = AddonManager.ERROR_INCORRECT_HASH;
> +      XPIProvider.removeActiveInstall(this);

indent

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5515
(Diff revision 2)
>        if (calculatedHash != this.hash.data) {
>          logger.warn("File hash (" + calculatedHash + ") did not match provided hash (" +
>               this.hash.data + ")");
>          this.state = AddonManager.STATE_DOWNLOAD_FAILED;
>          this.error = AddonManager.ERROR_INCORRECT_HASH;
> +      XPIProvider.removeActiveInstall(this);

indent
Attachment #8807793 - Flags: review?(rhelmer)
Comment on attachment 8807793 [details]
Bug 1315407 Revise tracking of active AddonInstalls

https://reviewboard.mozilla.org/r/90818/#review92066

> s/push/add/
> 
> Do we have any test coverage for `createStagedInstall` (which calls this)?
> 
> It's only during failure of `processPendingFileChanges` it looks like.

Whoops, I overlooked that since it gets wiped out by the next patch :-)  still it should be fixed here, I'll push a change momentarily.
We talked about this on IRC last week and we don't have any tests for it since it is invoked only when applying a staged update fails.  I think that manually injecting a fault would be the only practical way to test this, that wouldn't be too difficult, but it wouldn't be completely trivial.
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review92088

A few nits on this one but otherwise lgtm.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6741
(Diff revision 2)
>    let location = XPIProvider.installLocationsByName[KEY_APP_PROFILE];
>    let url = NetUtil.newURI(aUri);
>  
> -  let install = new AddonInstall(location, url, aHash, null, null, aBrowser);
> -  if (url instanceof Ci.nsIFileURL)
> -    install.initLocalInstall(aCallback);
> +  if (url instanceof Ci.nsIFileURL) {
> +    let install = new LocalAddonInstall(location, url, aHash);
> +    install.init().then(() => { aCallback(install); });

Are the `{}` around `aCallback(install)` necessary?

Concise arrow function body implies return, but I don't think it is harmful in this case?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6760
(Diff revision 2)
>   * @param  aAddon
>   *         The add-on being updated
>   * @param  aUpdate
>   *         The metadata about the new version from the update manifest
>   */
> -AddonInstall.createUpdate = function(aCallback, aAddon, aUpdate) {
> +function createUpdate(aCallback, aAddon, aUpdate) {

Could this be a `static` method of `AddonInstall` rather than a top-level function?

I don't really have strong feelings, but that would be the equivalent of this before I think.

Same for `createLocalInstall` and `createDownloadInstall`.
Attachment #8807794 - Flags: review?(rhelmer) → review+
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review90558

> This code was just moved from one place to another though mozreview didn't figure that out.
> But yeah, the reference to "DB operations" seems quite dated, everything that touches XPIDatabase these days just writes the whole thing as a big json blob, and that's what happens here, I don't think we can realistically do anything lighter (and even if we could I don't think it would be worth it)
> And yep the second comment also seems obsolete.
> I think the comment should just be removed...

Removing the comment sgtm!

> This is a guess, but I suspect that comment was about potentially putting something in the UI to indicate that the install has gone into this state.  We've lived without it for a while and nobody has complained so I'm inclined to just remove the comment.  If somebody wants to add that to the UI, it will be fairly obvious that they need to notify the UI when this transition happens, even without the comment there as a landmark.

+1

> you also referenced this in your initial comment but i just see any empty comment here...?

Oops I actually meant to delete this comment, and forgot I put it in the intitial. You can drop it.

> yeah, I mentioned this over on the bug, but this is what `AddonManager.getInstallForURL()` turns into, and that method supports file: urls.  I could rename it to something like createInstallForURL?  (or FromURL?)

I am fine leaving it as-is if you don't want to deal with test fallout and potential use by addons etc, I just find it a bit confusing that you can can `getInstallForFile(nsIFile)` or `getInstallForURL(url, ...)`. Why purpose does having a separate `getInstallForFile()` serve?

If you have an `nsIFileURL` it seems pretty trivial to pass a `nsIFileURL`.

Feel free to drop this though, just wanting to make sure I understood :)
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review90558

> I am fine leaving it as-is if you don't want to deal with test fallout and potential use by addons etc, I just find it a bit confusing that you can can `getInstallForFile(nsIFile)` or `getInstallForURL(url, ...)`. Why purpose does having a separate `getInstallForFile()` serve?
> 
> If you have an `nsIFileURL` it seems pretty trivial to pass a `nsIFileURL`.
> 
> Feel free to drop this though, just wanting to make sure I understood :)

Good question, but those are methods on AddonManager which I think are widely used, possibly even by out-of-tree code so I steered clear of touching them (a quick search of addons on DXR turns up hundreds/thousands of uses of byFile and byURL respectively)
Comment on attachment 8807794 [details]
Bug 1315407 Refactor AddonInstall

https://reviewboard.mozilla.org/r/90820/#review92088

> Are the `{}` around `aCallback(install)` necessary?
> 
> Concise arrow function body implies return, but I don't think it is harmful in this case?

Its not harmful but I like to use the braces to underscore the fact that nothing is looking at the return value.
If you feel strongly, I can remove them.

> Could this be a `static` method of `AddonInstall` rather than a top-level function?
> 
> I don't really have strong feelings, but that would be the equivalent of this before I think.
> 
> Same for `createLocalInstall` and `createDownloadInstall`.

As discussed on IRC, I wanted to keep things ordered and while we're declaring AddonInstall, the subclasses obvoiusly haven't been declared yet.  The longer term goal is to turn on no-use-before-define in eslint.
Comment on attachment 8807793 [details]
Bug 1315407 Revise tracking of active AddonInstalls

https://reviewboard.mozilla.org/r/90818/#review92122
Attachment #8807793 - Flags: review?(rhelmer) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95ef280ccc1b
Revise tracking of active AddonInstalls r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/7f747c8f6c60
Refactor AddonInstall r=mossop,rhelmer
New try run, now with passing sdk tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a2143c32fc32230cc68826a5f160cca96b2931
Flags: needinfo?(aswan)
This was originally backed out for both JP failures AND bc7 failures. The JP tests are fixed, but bc7 is still failing. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/00132770eecba298426f1f8fad715e1f92738f94
Flags: needinfo?(aswan)
Comment on attachment 8809945 [details]
Bug 1315407 Fix sdk addon-install

https://reviewboard.mozilla.org/r/92410/#review92462
Attachment #8809945 - Flags: review?(dtownsend) → review+
Ack sorry about missing the bc failures.  I'm giving both fixes a full cycle through try...
Flags: needinfo?(aswan)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b778159922a3
Revise tracking of active AddonInstalls r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/1aea09315a92
Refactor AddonInstall r=mossop,rhelmer
https://hg.mozilla.org/integration/autoland/rev/bba6abab045c
Fix sdk addon-install r=mossop
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: