Mark Smith [:mcs] Reporter
	Description • 15 years ago

When upgrading from Firefox 3.5.7 to 3.6, I am not prompted to update
my incompatible extensions even when a new Firefox 3.6 compatible version is available.  I have observed this problem with 4 different extensions, some on MacOS and some on WinXP, so I assume it is a general issue; in this bug report I am using Flagfox to provide a reproducible test case.

Steps to reproduce:

1) Start Firefox 3.5.7.

2) Install Flagfox Version 3.3.13 from:
https://addons.mozilla.org/en-US/firefox/addons/versions/5791#version-3.3.13

3) Restart Firefox 3.5.7.  Verify that Flagfox is installed and working.

4) Check for updates via Help | Check for Updates.  Click "Get the New Version"
to start downloading Firefox 3.6.  Note that the "some extensions are incompatible" warning is not displayed because a compatible version of Flagfox is available on AMO.

5) When the Software Update window displays the "Update Ready to Install" message, click "Restart Firefox".

Results:  Flagfox is not updated and is disabled because it is not compatible with Firefox 3.6.  You will not be prompted to update it.

Expected results:  You should be prompted to check for an update, as was done in Firefox 3.5.

Note that if I then open the Add-ons manager and click "Find Updates" I am
told that version 3.3.20 is available.  Updating works fine.

Is this an intentional change to 3.6?

	Robert Strong (they/them - no direct email)
	Comment 1 • 15 years ago

disabledAddons.length is 0 here... I'll see if I can figure out what is going on here bug I highly suspect Bug 489941

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsExtensionManager.js#3303

	Robert Strong (they/them - no direct email)
	Comment 2 • 15 years ago

I verified bug 489941 caused this. It tries to create an array (disabledAddons) of new appDisabled add-ons but this is done after _finishOperations is called which disables those add-ons and hence are no longer seen as new appDisabled.

	Mark Smith [:mcs] Reporter
	Comment 3 • 15 years ago

(In reply to comment #2)
> I verified bug 489941 caused this. It tries to create an array (disabledAddons)
> of new appDisabled add-ons but this is done after _finishOperations is called
> which disables those add-ons and hence are no longer seen as new appDisabled.

Thanks for looking at this so quickly.  Restating what you said, the new code added to checkForMismatches() as part of bug 489941 assumes that it knows when extensions transition to the appDisabled state, but the _finishOperations() code has already disabled the incompatible extensions.

Should the disabledAddons.length > 0 check be removed?  As things stand now, for people who do not update their add-ons before installing Firefox 3.6, the upgrade experience is a jarring (and confusing) one!

I requested blocking1.9.2 status, because I don't see a blocking1.9.2.x flag.

	Dave Townsend [:mossop] Assignee
	Comment 4 • 15 years ago

Where in _finishOperations are we transitioning newly incompatible add-ons to be appdisabled?

	Dave Townsend [:mossop] Assignee
	Comment 5 • 15 years ago

Oh never mind, I see it. Damn

	Dave Townsend [:mossop] Assignee
	Comment 6 • 15 years ago

To explain fully what is happening, at the start of checkForMismatches we checkForFileChanges. Something has changed (almost certainly the default theme only), this causes us to finishOperations and since something changed we update the appDisabled state of all add-ons in there since dependencies requires us to. This leaves the loop in checkForMismatches for disabling add-ons for incompatibility redundant in this case.

On Fennec this probably all works as expected. They don't have the default theme so upgrading the app wouldn't normally cause checkForFileChanges to find anything causing us to enter finishOperations, so the disabledAddons list should get properly filled and they should see a notification on startup as expected.

Upshot is that yes I think if we just remove, or move the disabledAddons.length check then this should fix Firefox and keep Fennec working.

	Dave Townsend [:mossop] Assignee
	Comment 7 • 15 years ago

(In reply to comment #6)
> Upshot is that yes I think if we just remove, or move the disabledAddons.length
> check then this should fix Firefox and keep Fennec working.

And yes it is kind of flakey to rely on no installed add-ons changing for disabledAddons to flow through to the pref, but I think it is reasonably safe to rely on that on mobile devices on branch for now. We can make this much more robust on trunk at a later point.

	Robert Strong (they/them - no direct email)
	Comment 8 • 15 years ago

Dave, though it is true that chances are the default theme will cause this as well prior to that happening the following is called before checkForMismatches
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2382

Which will cause isDirty to be true for _ensureDatasetIntegrity which will cause _finishOperations to be called.

	Robert Strong (they/them - no direct email)
	Comment 9 • 15 years ago

(In reply to comment #3)
>...
> Should the disabledAddons.length > 0 check be removed?  As things stand now,
> for people who do not update their add-ons before installing Firefox 3.6, the
> upgrade experience is a jarring (and confusing) one!
> 
> I requested blocking1.9.2 status, because I don't see a blocking1.9.2.x flag.
I'm not sure what the correct fix should / will be and will let Dave decide that though I agree and expect that this should be fixed in a 1.9.2.x update relatively soon... hopefully before a major update is offered.

	Dave Townsend [:mossop] Assignee
	Comment 10 • 15 years ago

Yeah I want to see this fixed on 1.9.2.x a.s.a.p, I just need to do some manual testing this afternoon to check my understanding is right here before deciding for sure on the way forward.

As a side note we do have this covered in litmus tests but the extended tests weren't run for the 3.6 major update: https://litmus.mozilla.org/show_test.cgi?id=8756

	Dave Townsend [:mossop] Assignee
	Comment 11 • 15 years ago

This is the safest way I can think to resolve this correctly. In the first pass through the extensions in checkForMismatches we gather a list of all the extensions that are not appDisabled. This is before finishOperations has run so we can be sure that the list is correct for the previous application version. Then in the final run through for every item that is no longer usable and used to be app enabled according to the list we know that it must have been disabled due to the application upgrade, and that is what we should pass through for apps like Fennec to use.

We're ending up in a place where we're doing basically the same sort of thing for both desktop and mobile here but in slightly different ways and different places, but I'll consolidate that later rather than take the additional risk here.

The automated tests can't really be run for multiple app versions so I'm don't have an easy way to test this right now.

	juan becerra [:juanb]
	Comment 12 • 15 years ago

This also affects the major update offer to 3.0.17 builds.

	Dave Townsend [:mossop] Assignee
	Comment 13 • 15 years ago

(In reply to comment #12)
> This also affects the major update offer to 3.0.17 builds.

The patch that broke this never landed on the 1.9.0 branch so you are probably seeing a different issue (albeit with the same symptoms). Please can you file a fresh bug

	juan becerra [:juanb]
	Comment 14 • 15 years ago

I think the extension "check for update" happens on launching Fx 3.6 for the first time after the major update, so it might be the same code.

	Robert Strong (they/them - no direct email)
	Comment 15 • 15 years ago

I'll likely get to the review of the patch tonight. I spoke with Dave regarding tests and believe that a fairly simple xpcshell test is possible by calling the checkForMismatches method and using a window watcher to intercept the opening of update.xul.

	Robert Strong (they/them - no direct email)
	Comment 16 • 15 years ago

Comment on attachment 424316 [details] [diff] [review]
patch rev 1

Looks fine. Can mfinkle verify this does the right thing on Fennec? If a test is possible it should probably verify the value of extensions.disabledAddons as well.

	Dave Townsend [:mossop] Assignee
	Comment 17 • 15 years ago

This adds unit tests for both the Firefox and Fennec cases. There is one minor change to nsExtensionManager.js that wasn't present in the previous patch and that is resetting gFirstRun at the end of nsIExtensionManager.start. This is purely necessary for the tests. Without this no attempt to notify the user about changes is ever made because gFirstRun is permanently true.

	Robert Strong (they/them - no direct email)
	Comment 18 • 15 years ago

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

Looks good!

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 19 • 15 years ago

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

The tests look good for Fennec. Thanks Dave.

	Dave Townsend [:mossop] Assignee
	Comment 20 • 15 years ago

Landed: http://hg.mozilla.org/mozilla-central/rev/361978b7ce3d

	Dave Townsend [:mossop] Assignee
	Comment 21 • 15 years ago

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

We should take this on the branch before we do a major update. The changes here are well tested and there have been no reported problems since it landed on trunk.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 22 • 15 years ago

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

a1922=beltzner

	Dave Townsend [:mossop] Assignee
	Comment 23 • 15 years ago

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7afc9557c8f0

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 24 • 15 years ago

Here's a question:

Let's say that a user has Firefox 3.6, and has already hit this bug. When they upgrade to Firefox 3.6.2, will they see the prompt, or once the damage is done must they always employ the workaround from comment 0 (manually check for updates)?

	Dave Townsend [:mossop] Assignee
	Comment 25 • 15 years ago

(In reply to comment #24)
> Here's a question:
> 
> Let's say that a user has Firefox 3.6, and has already hit this bug. When they
> upgrade to Firefox 3.6.2, will they see the prompt, or once the damage is done
> must they always employ the workaround from comment 0 (manually check for
> updates)?

This is purely about the prompt shown on startup of a new version of Firefox that has disabled some extensions. Background update checks will still be happening once a day and users will get updated like that.

	Mike Hommey [:glandium]
	Comment 26 • 15 years ago

The commit for this bug created a toolkit/mozapps/extensions/test/unit/head_extensionmanager.js.orig file in the mercurial tree.