570200 - Errors aren't passed sanely from getInstallForFile

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Currently if getInstallForFile encounters an error it throws an exception rather than calling the callback. This kind of doesn't make sense I think and it would be better to instead pass the callback the AddonInstall instance in the DOWNLOAD_FAILED state, exactly matching what you'd get from onDownloadFailed for the same XPI if you used getInstallForURL.

As part of this I'm also going to fix bug 553023 by exposing the error since it will not be exposed any other way for this case and I'm also going to make the error parameter on onInstallFailed a simple error code rather than an exception which you can't compare to anything.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Adds error handling and tests a bunch of cases. This fixes bug 553023, bug 562303, bug 567173 and bug 569194.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated from our discussion yesterday. I don't think it makes sense to call onNewInstall for AddonInstalls with errors so the only events that get installs in an error state are onDownloadFailed and onInstallFailed. It is perhaps a little redundant that we pass the error code as an argument to those while it is already available on the install object itself but I think for those states that code is more important so it makes some sense to include it. The alternative is removing it which would cause a certain amount of churn, probably doable at this stage but I'd still rather avoid it.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 450163 [details] [diff] [review]
patch rev 2

I would prefer consistency between them but understand wanting to avoid churn. How about a followup bug to make them consistent at some point?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (From update of attachment 450163 [details] [diff] [review])
> I would prefer consistency between them but understand wanting to avoid churn.
> How about a followup bug to make them consistent at some point?

If you think it is necessary to do at some point then we should just do it now to avoid changing the API on people later. I'll update the patch to do that.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Updated to remove those arguments.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 450419 [details] [diff] [review]
patch rev 3

Looks great

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0d404b63f71b

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking as verified fixed based on check-in and a green tinderbox in our area.