Last Comment Bug 521780 - extension upgrade with a moved location breaks extension manager
: extension upgrade with a moved location breaks extension manager
Status: VERIFIED FIXED
: fixed1.9.0.18
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: 1.9.0 Branch
: x86 Linux
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Alexander Sack
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-12 06:00 PDT by Alexander Sack
Modified: 2010-12-05 03:50 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5-fixed
.8-fixed


Attachments
catch GetTarget exception (1.08 KB, patch)
2009-10-12 06:16 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
oops (1.09 KB, patch)
2009-10-12 06:23 PDT, Alexander Sack
dtownsend: review-
Details | Diff | Splinter Review
fix type of newVersion property... (1016 bytes, patch)
2009-10-19 13:59 PDT, Alexander Sack
dtownsend: review+
mbeltzner: approval1.9.2+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Splinter Review

Description Alexander Sack 2009-10-12 06:00:59 PDT
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 2009-10-12 06:16:24 PDT
Created attachment 405845 [details] [diff] [review]
catch GetTarget exception
Comment 2 Mike Hommey [:glandium] 2009-10-12 06:18:58 PDT
You don't assign oldValue, this ain't gonna work ;)
Comment 3 Alexander Sack 2009-10-12 06:23:36 PDT
Created attachment 405846 [details] [diff] [review]
oops
Comment 4 Dave Townsend [:mossop] 2009-10-12 10:22:13 PDT
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 2009-10-13 02:28:59 PDT
(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] 2009-10-13 09:16:18 PDT
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 2009-10-14 18:49:08 PDT
Ubuntu bug:
https://bugs.launchpad.net/bugs/441552
Comment 8 Alexander Sack 2009-10-19 13:59:57 PDT
Created attachment 407108 [details] [diff] [review]
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] (use needinfo?) 2009-11-09 09:33:48 PST
http://hg.mozilla.org/mozilla-central/rev/a1749e78c2be
Comment 10 Reed Loden [:reed] (use needinfo?) 2009-11-09 09:42:09 PST
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] 2009-12-02 08:13:30 PST
Comment on attachment 407108 [details] [diff] [review]
fix type of newVersion property...

a192=beltzner
Comment 12 Marco Bonardo [::mak] 2009-12-02 08:56:02 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/760b6fefebcc
Comment 13 Daniel Veditz [:dveditz] 2009-12-14 14:25:30 PST
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] (use needinfo?) 2009-12-14 23:16:55 PST
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) [away 09/30 - 10/06] 2010-12-05 03:50:36 PST
The nsExtensionManager.js.in module is gone on trunk. Just marking it as verified fixed.

Note You need to log in before you can comment on or make changes to this bug.