256373 - globally installed extensions problem when they "expire"

Status: RESOLVED WORKSFORME

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Hommey [:glandium]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ja-JP; rv:1.7.2) Gecko/20040819 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ja-JP; rv:1.7.2) Gecko/20040819 Firefox/0.9.1+

When a globally installed extension expires (i.e. em:MaxVersion <
firefox_version), it breaks some things.
It looks like it totally stops the extension manager, so that it even don't
finish registering the other extensions, and running as normal user brings a
loop (only if the user doesn't have a profile already, though):
*** loading the extensions datasource
*** loading the extensions datasource
*** loading the extensions datasource
...


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Mike Hommey [:glandium]]

I managed to circumvent the issue by adding version checking in
ensurePreConfiguredItem in nsExtensionManager.js and only add to the datasource
if the version is fine.
In a perfect world, instead of moving installed-extensions.txt to
installed-extensions-processed.txt, the nsInstalledExtensionReader should write
which extensions were successfully installed.
A probably better thing would also be to tag the non matching extensions to be
uninstalled.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Basic patch to circumvent the issue

Note that the patch has been made against the installed .js file of a 0.9.3,
but might apply on the .js.in file in the aviary branch. Well, you get the
idea.
Also note that uncommenting the else clause brings the loop again...

Comment 3

[Mike Hommey [:glandium]]

Attachment: Additional patch to write an installed-extensions-processed.txt file

Comment 4

[Asa Dotzler [:asa]]

Ben, can you take a look at this?

Comment 5

[Mike Hommey [:glandium]]

Attachment: Cleaner patch

This new patch solves the issue by actually implementing the behaviour
described in https://bugzilla.mozilla.org/show_bug.cgi?id=247846#c14

Comment 6

[Mike Hommey [:glandium]]

Note that patch from Comment #5 doesn't register components in
bin/extensions/{guid}/components/

Comment 7

[Mike Hommey [:glandium]]

Addition to Comment #6: it seems to be due to the fact that
this._ds.isProfileItem(extension.id) always return true (even for global
extensions) in nsExtensionManager__writeProfileFile when it is called through
the nsExtensionManager_register. I haven't tested if it does the same in other
conditions.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Workaround to make components registration work

Since isProfileItem returns a wrong value when the register function of the EM
is called, i just inverted the test as a workaround, since only 2 values
(global and profile) are allowed for installLocation.

What happens without that workaround is the following:
in _writeProfileFile (called from register), the loop begins, the first
extension in grabbed then comes the
var isProfile = this._ds.isProfileItem(extension.id);
which always return true, due to
getItemProperty(extension.id,"installLocation") returning null, and then
var sourceDir = aGetDirFunc(isProfile, extension.id);
gets an exception from fileLocator.get in getDirInternal, which doesn't like
the ProfD key in this case. But since I didn't have any clue why the latter
happens, I just worked around that issue.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Workaround to make components registration work

Oops, the patch was reverted.

Comment 10

[Mike Hommey [:glandium]]

Attachment: New patch

Here is a merged patch which also writes the defaults manifest when running
-register, so that bin/extensions/{uid}/defaults/preferences also get
registered.
There's also a kinda workaround in the exception handling at extension
installation time, so that extensions don't get tagged toBeUninstalled, since
it won't be possible to do as user.

Comment 11

[Alexander Sack]

Looks good. This fixes the infinite loop and other issues with the extension
manager. The -register flag is now usable. 

Nevertheless it appears to be still necessary to trash the profiles compreg.dat
once and restart thunderbird for unprivileged users in order to get globally
installed components registered correctly to an existing profile.

This is of course not needed if firefox/thunderbird itself was upgraded at the
same time, because the extension manager detects it. Maybe the EM should handle
a changed global Extensions.rdf in a similar way?

Comment 12

[Mike Hommey [:glandium]]

Attachment: Some more changes.

Including the following:
- Avoid creation of directories when a file is not found in some cases
(especially when we only need to read these files ; that led to an infinite
loop when there is no uninstall directory in appdir/extensions/{uid} and the
appdir is read-only (e.g. a system dir and firefox run as user on a system with
proper rights set))
- If the manifest for an extension doesn't exist at -register time, just do
nothing, instead of trying to get the file from DIR_DEFAULTS_EXTENSIONS, which
is not what -register is expected to do.
- Only try to extract stuff from xpi if we are installing in user profile
directory, though i'm not sure plain firefox might want to do that, we
definitely do in debian.
- Lock global extensions registered through -register in any cases
- Don't disable extensions that are already disabled.
- Don't check for extensions update for locked extensions.
- Disable update button in the extensions manager of locked items.

There might still be problems with upgrade service for locked items, which are
solved in debian by the fact we disabled the upgrade service for global stuff,
since it is handled by the debian packaging system.

Anyways, I think the doing stuff in global directory when the app has the
rights is a *bad* *bad* thing that really needs to be fixed deeply in the
application. That kind of stuff is how most windows apps work, but it is
definitely a bad idea on systems with strong restrictions on file properties
and ownerships.
It also leed "power users" to be able to fuck up the packaging system or just
the software installation by using the software which is also a very bad thing.


I'll file another bug for the latter.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Fixed upgrade

Previous patches broke extension upgrade by user, it's now fixed.

Comment 14

[Steffen Wilberg]

-> Mike.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Patch against trunk with -up8 option

Following Steffen's advice on IRC, cvs diff -up8'ing against trunk.

Comment 16

[Mike Hommey [:glandium]]

(In reply to comment #12)
> - Only try to extract stuff from xpi if we are installing in user profile
> directory, though i'm not sure plain firefox might want to do that, we
> definitely do in debian.

Note this has been reverted in patch #167527 and #167532 and fixed another way.

Comment 17

[timeless]

Comment on attachment 167532 [details] [diff] [review]
Patch against trunk with -up8 option

mike, please don't use tabs in patches for mozilla.org, and please try to use
consistent alignment that matches the file you're patching. this means not
doing things like:

function confused()
{
  var indented;
  if (true) {
      really-indented;
  }
}

it--;
 also++;
means--;
  not+=doing;
 // stuff
  // like
// this.

Comment 18

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

Is this still a problem on the 1.8 branch or trunk?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 167532 [details] [diff] [review]
Patch against trunk with -up8 option

This patch no longer applies.

Comment 20

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

This is WFM with 1.5