607818 - Bootstrapped add-ons are being given the wrong reason code in some cases

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Dave Townsend [:mossop]]

Once bug 553017 is fixed the following problems will still exist:

Uninstalling a profile add-on and revealing another add-on should sent ADDON_DOWNGRADE/ADDON_UPGRADE to the shutdown, uninstall, install, startup methods of the two add-ons.

Detecting a removed profile add-on on startup and activating a previously hidden add-on should send ADDON_DOWNGRADE/ADDON_UPGRADE to the install method of the add-on.

Both cases currently still send reasonably sane reason codes so this shouldn't block the release

Comment 1

[Hernán Rodriguez Colmeiro (:peregrino)]

I'm experiencing this when upgrading a traditional addon with an Addon-SDK based one. After the needed restart, the bootstrapped version of the addon doesn't know it's an upgrade of a previous one as it receives the startup flag, not upgrade nor install.

Comment 3

[Ben Basson]

Please see attachments / comments in bug 1199743 as a testcase for this bug. Thanks.

Comment 5

[Andrew Swan [:aswan]]

Attachment: Bug 607818 Fix bootstrap reasons during addon upgrade/downgrade operations

When the same addon is installed in multiple locations and the copy in
the highest priority location is removed, we reveal the next-highest priority
copy.  Since we're just changing addon versions, this sequence should look
like an upgrade or downgrade.  This patch fixes the reason parameter passed
to both instance's bootstrap methods to reflect that.

Review commit: https://reviewboard.mozilla.org/r/192206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192206/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8921205 [details]
Bug 607818 Fix bootstrap reasons during addon upgrade/downgrade operations

https://reviewboard.mozilla.org/r/192206/#review197418

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1681
(Diff revision 1)
> +   * of an add-on by ID and return the location and the XPIState.
> +   * @param {string} aId       The add-on ID
> +   * @param {string} aLocaname The name of the location to skip
> +   * @return {XPIState?}
> +   */
> +  findAddonSkipLocation(aId, aLocname) {

This is super specific... Can we just pass a location filter function to `findAddon` and have it default to `() => true`?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8921205 [details]
Bug 607818 Fix bootstrap reasons during addon upgrade/downgrade operations

https://reviewboard.mozilla.org/r/192206/#review197418

> This is super specific... Can we just pass a location filter function to `findAddon` and have it default to `() => true`?

yeah, good idea

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8921205 [details]
Bug 607818 Fix bootstrap reasons during addon upgrade/downgrade operations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192206/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca99e68b3b36
Fix bootstrap reasons during addon upgrade/downgrade operations r=kmag

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ca99e68b3b36

Comment 11

[Madalin Cotetiu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!