925389 - Add-on update check is not cancelled when add-on is uninstalled

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

While working on tests for bug 772484, I tripped over an odd failure condition.

The sequence of events triggering the failure is to call addon.findUpdate() for an add-on that has an available update and then, before the AddonUpdateChecker completes its HTTP transaction, uninstall the add-on.

When the HTTP result comes back, the update checker tries to apply the updated information to the (no longer installed) add-on, and throws an unhandled exception:

System JS : ERROR resource://gre/modules/XPIProvider.jsm:6858
                      Error: Unknown add-on ID [e-mail]

under DBA_applyCompatibilityUpdate -> XPI_updateAddonDisabledState -> DirInstallLocation_getLocationForID


We could either do some consistency checking in DBA_applyCompatibilityUpdate (or the function that calls it) to bail out if the results of the AddonUpdateChecker are not longer needed, or add a mechanism to cancel the update check if the add-on is uninstalled.

I've put together a WIP patch for cancelling the update check. Open issues:

0) Do we really care?
1) Are there places aside from uninstalling the add-on where we should cancel?
2) We should probably still add some exception handling to notify listeners if anything else throws in the completion callbacks.
3) Specific test case.
4) Would we rather move this update check into AddonInstall and use that existing framework to manage the life cycle?

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: Add support for cancelling in-progress add-on update checks

This doesn't break the existing xpcshell or browser-chrome tests, but doesn't have explicit tests for the new behaviour.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 815870 [details] [diff] [review]
Add support for cancelling in-progress add-on update checks

Review of attachment 815870 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Irving Reid from comment #0)
> 0) Do we really care?

Yes. We're going to have to be more careful about this to, with changing more things to async - like bug 562674. Might be worth talking to Mossop about introducing a more generic way to register/cancel async operations, see what strategy orks for that bug (which may be just ad-hoc special casing every instance of this kind of issue).

> 1) Are there places aside from uninstalling the add-on where we should
> cancel?

Shutdown?


> 2) We should probably still add some exception handling to notify listeners
> if anything else throws in the completion callbacks.

Yep.

> 3) Specific test case.

Yep.

> 4) Would we rather move this update check into AddonInstall and use that
> existing framework to manage the life cycle?

May actually need this in both AddonInternal and AddonInstall :\ Both can trigger this (in cases where the other doesn't exist), and both should be able to cancel it.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +456,2 @@
>     */
> +  startup: function AMI_startup(aTestStartup = false) {

How is this relevant? (Ditto with the head_addons.js changes)

Comment 3

[Dave Townsend [:mossop]]

(In reply to Blair McBride [:Unfocused] from comment #2)
> Comment on attachment 815870 [details] [diff] [review]
> Add support for cancelling in-progress add-on update checks
> 
> Review of attachment 815870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Irving Reid from comment #0)
> > 0) Do we really care?
> 
> Yes. We're going to have to be more careful about this to, with changing
> more things to async - like bug 562674. Might be worth talking to Mossop
> about introducing a more generic way to register/cancel async operations,
> see what strategy orks for that bug (which may be just ad-hoc special casing
> every instance of this kind of issue).

Some time ago I considered a plan where every async operation in the add-ons manager returned something like an nsICancelable. Now I wonder if it might not be a good idea to return a special promise with an additional cancel method to stop the action and reject the promise. That would make the callback arguments optional though which would mean passing a lot of nulls.

Comment 4

[:Irving Reid (No longer working on Firefox)]

Attachment: Cancel in-progress update checks, v2 (with tests & more catches)

Pushed to try, to make sure the tests don't mess up on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=8623d3639bce

(In reply to Blair McBride [:Unfocused] from comment #2)
> Might be worth talking to Mossop
> about introducing a more generic way to register/cancel async operations,
...
> > 1) Are there places aside from uninstalling the add-on where we should
> > cancel?
> 
> Shutdown?

I added a doing() / done() / cancelAll() combo to XPIProvider and hooked up the update check and shutdown - how does that look?

> > 2) We should probably still add some exception handling to notify listeners
> > if anything else throws in the completion callbacks.
> 
> Yep.

I couldn't see any more places where exceptions could be reported back to listeners, but I think I covered the rest of the places where we weren't protecting ourselves against broken callbacks.

> > 4) Would we rather move this update check into AddonInstall and use that
> > existing framework to manage the life cycle?
> 
> May actually need this in both AddonInternal and AddonInstall :\ Both can
> trigger this (in cases where the other doesn't exist), and both should be
> able to cancel it.

I was thinking that all the async things that happen around the add-on could be managed by AddonInstall, but after spending more time in the code that idea didn't make as much sense.

> ::: toolkit/mozapps/extensions/AddonManager.jsm
> @@ +456,2 @@
> >     */
> > +  startup: function AMI_startup(aTestStartup = false) {
> 
> How is this relevant? (Ditto with the head_addons.js changes)

This is a drive-by fix of a minor test regression from bug 911621, because the test harness shuts AddonManager down directly instead of through the Observer service. I can move it to a separate patch if you'd like.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8337493 [details] [diff] [review]
Cancel in-progress update checks, v2 (with tests & more catches)

Review of attachment 8337493 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +467,5 @@
>    /**
>     * Initializes the AddonManager, loading any known providers and initializing
>     * them.
> +   * @param aTestStartup If AddonManager is being initialized by the test harness,
> +   *                     it will not register its normal shutdown hook.

Why is this needed? We've explicitly tried to avoid doing this in the past.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3757,5 @@
>      this.installs.forEach(function(aInstall) {
>        if (!aTypes || aTypes.indexOf(aInstall.type) >= 0)
>          results.push(aInstall.wrapper);
>      });
> +    makeSafe(aCallback)(results);

The callback here is already guaranteed to be safe, by AddonManager's safeCall().

@@ +4676,5 @@
>     * @param  aCallback
>     *         The callback to pass the initialised AddonInstall to
>     */
>    initLocalInstall: function AI_initLocalInstall(aCallback) {
> +    let safeCallback = makeSafe(aCallback);

Should re-use the callback variable in each function, so we can't accidentally use the callback without the safe wrapper:

  aCallback = makeSafe(aCallback);

And as a bonus, that means a lot less changes are needed.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Attachment: Cancel in-progress update checks, v3

Mocking out AsyncShutdown made a bunch of the changes in the previous version unnecessary.

Comment 7

[:Irving Reid (No longer working on Firefox)]

https://hg.mozilla.org/integration/fx-team/rev/0409df389464

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0409df389464

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8338265 [details] [diff] [review]
Cancel in-progress update checks, v3

Looks like this already landed. If you still wanted some feedback from me just re-request it.

Comment 10

[Ada [:adalucinet]]

After installing Adblock 2.4 on Nightly from 2013-10-14, I get: 
WARN addons.xpi: Failed to install: Error: Unknown add-on ID {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} (resource://gre/modules/XPIProvider.jsm:6852) when I remove the add-on and the available update is not fully downloaded.

With the same steps on Firefox 28 beta 6 (Build ID: 20140224220227), I get: 
Policy is null contentPolicy.js:415
NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIWebNavigation.loadURI]

Is it the right result? Could you please point to another add-on to test this? (Ghostery and Firebug weren't helpful)
Thanks in advance.

Comment 11

[:Irving Reid (No longer working on Firefox)]

This problem was observed in the debug output from running the xpcshell unit test suite, so I didn't work out steps to reproduce from the UI.

Can you describe in more detail how you're testing? I can't find where the error message you're seeing from 28 beta 6 is coming from.

Comment 12

[Ada [:adalucinet]]

(In reply to :Irving Reid from comment #11)
> Can you describe in more detail how you're testing? I can't find where the
> error message you're seeing from 28 beta 6 is coming from.

With Firefox 28 beta 8 on Windows 7 64bit and the below STR:
1. In Add-ons Manager, uncheck Update Add-ons Automatically
2. Install older version of Adblock Plus (eg: 2.4)
3. Check for updates
4. Remove addon from Add-ons Manager/Extensions, go to Available Updates and click on Install updates button.
In Browser Console I get:
WARN addons.xpi: Failed to install C:\Users\ALEXAN~1.LUC\AppData\Local\Temp\tmp-rhf.xpi from https://addons.cdn.mozilla.net/storage/public-staging/1865/adblock_plus-2.5.1-sm+tb+an+fx.xpi: 
Error: Unknown add-on ID {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} (resource://gre/modules/XPIProvider.jsm:7221)

5. In Add-ons Manager/Available Updates, click on Enable this add-on. 
Result: "Adblock Plus will be enabled after you restart Firefox" message is displayed. After restart, Adblock Plus is not available in Add-ons Manager/Extensions and in Browser Console: "[object ErrorEvent]" is displayed.

When I re-install Adblock Plus 2.4, in Browser Console I get (although the add-on was successfully installed): 
"WARN addons.xpi: Failed to remove temporary file C:\Users\ALEXAN~1.LUC\AppData\Local\Temp\tmp-s1a.xpi for addon https://addons.mozilla.org/firefox/downloads/file/230225/adblock_plus-2.4-tb+fx+an+sm.xpi?src=version-history: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AI_removeTemporaryFile :: line 4929"  data: no]"

I couldn't reproduce the error from comment 10 with Firefox 28 beta 8 nor beta 6.
If the above steps to reproduce are not reliable, could you please add another ones so that this fix can be properly verified?

Comment 13

[:Irving Reid (No longer working on Firefox)]

The steps you describe in https://bugzilla.mozilla.org/show_bug.cgi?id=925389#c12 are a different bug - please file it and CC: me.

The error covered by this bug was discovered by looking at logs as the mochitest-browser tests were running, and depended on very tight timing overlaps between when the HTTP server responded to the add-on downloads, and when the automated test clicked the 'cancel' button. I don't think it's worth trying to manually verify this fix.