Closed Bug 925389 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: Irving, Assigned: Irving)

References

Details

Attachments

(1 file, 2 obsolete files)

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 addon5@tests.mozilla.org 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?
This doesn't break the existing xpcshell or browser-chrome tests, but doesn't have explicit tests for the new behaviour.
Attachment #815870 - Flags: feedback?(bmcbride)
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)
Attachment #815870 - Flags: feedback?(bmcbride) → feedback+
(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.
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.
Attachment #815870 - Attachment is obsolete: true
Attachment #8337493 - Flags: review?(bmcbride)
Attachment #8337493 - Flags: feedback?(dtownsend+bugmail)
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.
Attachment #8337493 - Flags: review?(bmcbride) → review-
Mocking out AsyncShutdown made a bunch of the changes in the previous version unnecessary.
Attachment #8337493 - Attachment is obsolete: true
Attachment #8337493 - Flags: feedback?(dtownsend+bugmail)
Attachment #8338265 - Flags: review?(bmcbride)
Attachment #8338265 - Flags: feedback?(dtownsend+bugmail)
Attachment #8338265 - Flags: review?(bmcbride) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
Attachment #8338265 - Flags: feedback?(dtownsend+bugmail)
Keywords: verifyme
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.
Flags: needinfo?(irving)
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.
Flags: needinfo?(irving)
(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?
Keywords: verifyme
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: