394717 - NS_ERROR_ILLEGAL_VALUE calling nsIExtensionManager.update when profile updates

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alex Vincent [:WeirdAl]]

I'm not sure what's causing it, but I only see it immediately after I try to start Firefox with a profile that was based on a slightly older version.  When the profile tries to update extensions, it hangs.

I don't know exactly how to test this one, but it's definitely happening.

Comment 1

[Alex Vincent [:WeirdAl]]

Note that it doesn't really hang; you can still cancel out of it.  But once you hit the error, the update is essentially dead.

This was with a custom Venkman build installed in the profile - see http://weblogs.mozillazine.org/weirdal/archives/018483.html for details on how to package Venkman.

Comment 2

[Dave Townsend [:mossop]]

So are the steps to reproduce:

1. Install a new version of firefox (new build or whole version change?)
2. Wait for background update to happen

Comment 3

[Alex Vincent [:WeirdAl]]

JS stack trace:
Error()@:0
([object Array],0,2,[object XPCWrappedNative_NoHelper])@file:///home/ajvincent/trunk/xu-debug/dist/bin/components/nsExtensionManager.js:4973
update([object Array],0,2,[object Object])@:0
()@chrome://mozapps/content/extensions/update.js:154
anonymous([object Event])@chrome://global/content/bindings/wizard.xml:410
_fireEvent([object XULElement],"pageshow")@chrome://global/content/bindings/wizard.xml:411
set_currentPage([object XULElement])@chrome://global/content/bindings/wizard.xml:94
()@chrome://mozapps/content/extensions/update.js:54
onload([object Event])@chrome://mozapps/content/extensions/update.xul:1

And what do you know:

em.update([], 0, nsIExtensionManager.UPDATE_SYNC_COMPATIBILITY, this);

That second argument triggers NS_ERROR_ILLEGAL_VALUE.  This makes me wonder - what's the correct thing to do here?

Comment 4

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

Is there any reason not to remove the check for itemCount equaling 0 and updating the tests and idl accordingly?

Comment 5

[Alex Vincent [:WeirdAl]]

Robert: seeing as I'm the one that added it, all I can say is "it seemed like a good idea at the time..."

Comment 6

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

I agree and being one of the reviewers it seemed fine at the time to me as well. ;)

Would you be willing to put together a patch and request review from me?

Comment 7

[Dave Townsend [:mossop]]

I think having 0 items passed is probably sensible enough to not throw, however if you're allowing that can we be very explicit in the comments about whether the listener's onUpdateStarted and onUpdateEnded will ever be called

Comment 8

[Alex Vincent [:WeirdAl]]

Dave, you raise an interesting point.  From my cursory reading of the code, onUpdateStarted gets called, but not onUpdateEnded.  However, there was a call to it, as a couple extensions tried to update.  Not sure what they were, and that worries me.

I have a patch without the docs changes.  I'll submit it tomorrow after I've had a bit of time to think about it.

Comment 9

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

It looks to me that when itemCount is 0 we get all addons into the items array and we always use items and items.length in the call to checkForUpdates. I also observed onUpdateStarted being called at the start, all of the onAddonUpdateStarted / onAddonUpdateEnded pairs, and it finished with onUpdateEnded being called. Seems reasonable to me. Where in the code are you concerned about this?

Comment 10

[Alex Vincent [:WeirdAl]]

What if there really are zero extensions?

(Note:  Firefox probably doesn't qualify, as it has theme extensions.)

Comment 11

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

Ahhh... now I see the concern. I think adding onUpdateEnded when we have a listener in checkForUpdates after onUpdateStarted when aItemCount is 0 should suffice.

Comment 12

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Comment 13

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

Comment on attachment 279693 [details] [diff] [review]
patch, v1

Thanks Alex... r=me

Comment 14

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

Checked in to trunk

Checking in mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl;
/cvsroot/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl,v  <--  nsIExtensionManager.idl
new revision: 1.54; previous revision: 1.53
done
Checking in mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.251; previous revision: 1.250
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug394717.js,v
done
Checking in mozilla/toolkit/mozapps/extensions/test/unit/test_bug394717.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug394717.js,v  <--  test_bug394717.js
initial revision: 1.1
done