302206 - Need to restart Firefox twice after updating an extension

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[elfguy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050726 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050726 Firefox/1.0+

When updating an extension from within the extension manager, Firefox says it
will be installed on next restart. However it still says that after a restart.
It takes 2 restarts to have the extension working.

Reproducible: Always

Steps to Reproduce:
1. Update an extension
2. Restart Firefox
3. See EM

Comment 1

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

With an upgrade the metadata for the original item is removed and the item is
then set to needs-install. It appears that during an upgrade PendingOperations
no longer gets the needs-install for the item that is being upgraded though it
is written to the extensions.cache. Then when a restart is performed
PendingOperations is populated with the new data and the install finishes. I
should be able to come up with a patch shortly.

Comment 2

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

Attachment: patch

Benjamin - I've gone over the PendingOperations changes and can't find a reason
not to use the original code. I have also tested new installs, upgrades,
changing the app path, etc. and it all worked. Am I missing something?

Comment 3

[Benjamin Smedberg]

Comment on attachment 190680 [details] [diff] [review]
patch

The codepath that breaks is
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensio
nManager.js.in#4410
which calls
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensio
nManager.js.in#4348

Where "addItem(OP_NONE)" is called. AFAICT this is meant to remove all the
pending operations for that item ID, but perhaps I'm wrong. Anyway, it caused
infinite loops in the _finishOperations while-loop because there was nothing to
deal with OP_NONE in that loop. I think I encountered this when testing
upgrades from 1.0 to 1.1.

Comment 4

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

I see where that may have happened previously but the code has changed in
regards to this since bug 264750 landed. I suspect when going from 1.0 to 1.0+
the incompatibility check calling enableItem or disableItem could have caused
the loop you saw. To summarize:

The loop was in _finishOperations is only called during startup.
_appEnableItem and _appDisableItem are now used for the incompatibility checks
during startup. Neither of these ever set the op to OP_NONE.
enableItem and disableItem which can set the op to OP_NONE are only called via
the ui after startup with one exception in _upgradeFromV10. In this instance it
is a newly created datasource so it should always set the op to OP_NEEDS_DISABLE.

Comment 5

[Benjamin Smedberg]

Comment on attachment 190680 [details] [diff] [review]
patch

I'm still worried, but ok.

Comment 6

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

Attachment: more safe gaurding

I changed the migration code so it just sets userDisabled and appDisabled
directly since this only runs during migration and that is all that needs to
happen at that point. Now the only call site for either of the two functions
that set OP_NONE are in extensions.js.

Comment 7

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

Attachment: doh! fixed comment

Sorry about that. I forgot to fix a comment.

Comment 8

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

Fixed on trunk