Closed Bug 394717 Opened 13 years ago Closed 13 years ago

NS_ERROR_ILLEGAL_VALUE calling nsIExtensionManager.update when profile updates


(Toolkit :: Add-ons Manager, defect)

Not set





(Reporter: WeirdAl, Assigned: WeirdAl)




(Keywords: regression)


(1 file)

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.
Flags: blocking-firefox3?
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 for details on how to package Venkman.
Keywords: hang
Severity: critical → major
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
JS stack trace:
([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
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
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?
Is there any reason not to remove the check for itemCount equaling 0 and updating the tests and idl accordingly?
Robert: seeing as I'm the one that added it, all I can say is "it seemed like a good idea at the time..."
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?
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
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.
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?
What if there really are zero extensions?

(Note:  Firefox probably doesn't qualify, as it has theme extensions.)
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.
Attached patch patch, v1Splinter Review
Assignee: nobody → ajvincent
Attachment #279693 - Flags: review?(robert.bugzilla)
Comment on attachment 279693 [details] [diff] [review]
patch, v1

Thanks Alex... r=me
Attachment #279693 - Flags: review?(robert.bugzilla) → review+
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
Checking in mozilla/toolkit/mozapps/extensions/src/;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/,v  <--
new revision: 1.251; previous revision: 1.250
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug394717.js,v
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
Closed: 13 years ago
Flags: blocking-firefox3?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M8
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.