521780 - extension upgrade with a moved location breaks extension manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alexander Sack]

We moved our system langpacks from one location to another - previously not used one - for karmic; this seemed to have cause extension manager to go into a bad state (at least for me).

The symptoms are that during the upgrade, the uninstallItem function fails here:

http://hg.mozilla.org/mozilla-central/file/dbebe1771118/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#l4591

with an exception like:

[Exception... "Could not convert JavaScript argument arg 1 [nsIRDFDataSource.GetTarget]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///usr/lib/xulrunner-1.9.1.5pre/components/nsExtensionManager.js :: EMDS__setProperty :: line 7250"  data: no]

... which breaks the whole extension update somewhat.

I looked a bit around thinking that maybe the UPGRADE op is wrong, but i think thats not the problem. It seems that the _setProperty code her:

http://hg.mozilla.org/mozilla-central/file/dbebe1771118/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#l6939

is supposed to deal with this situation as it explicitly tests "if (oldValue)" ... however, GetTarget does not return null but throws and exception, so a potential fix would be to just catch it there.

Comment 1

[Alexander Sack]

Attachment: catch GetTarget exception

Comment 2

[Mike Hommey [:glandium]]

You don't assign oldValue, this ain't gonna work ;)

Comment 3

[Alexander Sack]

Attachment: oops

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 405846 [details] [diff] [review]
oops

GetTarget only throws when it is passed invalid arguments. We should fix the caller to not do that instead.

Comment 5

[Alexander Sack]

(In reply to comment #4)
> (From update of attachment 405846 [details] [diff] [review])
> GetTarget only throws when it is passed invalid arguments. We should fix the
> caller to not do that instead.

invalid in what way? I always thought that GetTarget also throws if "source" resource does not exist for example.

Comment 6

[Dave Townsend [:mossop]]

GetTarget only throws if source or property are null, however the error message you're getting isn't from GetTarget throwing, it is xpconnect throwing because source or property are not null or JS objects that it can pretend are nsIRDFResources.

Comment 7

[Micah Gersten]

Ubuntu bug:
https://bugs.launchpad.net/bugs/441552

Comment 8

[Alexander Sack]

Attachment: fix type of newVersion property...

ok newVersion was just a string while setItemProperty/setProperty requires a nsIRDFResource ... this patch fixes it by using EM_R("newVersion")

That said I noticed that setItemProperty assumes a nsIRDFResource as "property", while getItemProperty seems to EM_R "property" all the time. Might be worth looking into making the behaviour more consistent.

Comment 9

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/a1749e78c2be

Comment 10

[Reed Loden [:reed]]

Comment on attachment 407108 [details] [diff] [review]
fix type of newVersion property...

Ubuntu is already using this patch on their 1.9.1 branch, so would like to get this backported.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 407108 [details] [diff] [review]
fix type of newVersion property...

a192=beltzner

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/760b6fefebcc

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 407108 [details] [diff] [review]
fix type of newVersion property...

Approved for 1.9.1.7 and 1.9.0.17, a=dveditz for release-drivers

Comment 14

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b12d53eed85

Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.290; previous revision: 1.289
done

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The nsExtensionManager.js.in module is gone on trunk. Just marking it as verified fixed.