The add-ons manager remains empty and there is no extension installed after a restart. Regression between 1.9a1_2006061216 and 1.9a1_2006061220. http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-12+16%3A00&maxdate=2006-06-12+20%3A00

Regression range a bit wider: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-12+15%3A00&maxdate=2006-06-12+21%3A00

I've created two test builds from today, one without bug 255942 and one without both bug 255942 and bug 176182. Could you test these and see if either of them works? http://gavinsharp.com/ff/firefox-3.0a1-2006-06-13-07-no255942.zip http://gavinsharp.com/ff/firefox-3.0a1-2006-06-13-07-no255942-no176182.zip

Also this bug is not reproducible with the first build. Thanks for the build!

The problem is that nsWindowWatcher::OpenWindow (the version that takes an nsISupports arguments array) is broken. The API docs say: 87 @param aArguments extra argument(s) to the new window, to be attached 88 as the |arguments| property. An nsISupportsArray will be 89 unwound into multiple arguments (but not recursively!). 90 can be null. but it doesn't do that.

This patch should get things working again. It does break the Python bindings, but that's less of a concern than the trunk :)

Comment on attachment 225541 [details] [diff] [review] Patch that should convert nsISupportsArray again >+ nsCOMPtr<nsISupports> elt; >+ elt = supArray->ElementAt(i); nsISupportsArray::ElementAt returns a reference count to the caller, so you need elt = dont_AddRef(supArray->ElementAt(i));

(Does leaving the initial QI to nsIMutableArray not break the python bindings?)

> (Does leaving the initial QI to nsIMutableArray not break > the python bindings?) Yes, supporting nsIArray as another "special case" would get Python working again. In the early days of this patch I was pointed away from nsISupportsArray towards nsIArray due to it being deprecated and "more scriptable", but didn't realize this subtle distinction. This isn't a trivial change for Python to make. Python and JS have their own custom nsIArray implementations, which allows the original language values to be retrieved (ie, the native Python dict object, or whatever). These arrays are used for Window arguments and for event arguments. JS bypasses nsIWindowWatcher::Open, but Python can't - it is the only scriptable function available. bz correctly notes the interface is frozen and that this has the potential to be a breaking change. However, lxr doesn't show anyone relies on passing nsIArray to this function, so giving nsIArray that special treatment may still be a possibility. If not, I guess I can just add a basic skeleton implementation of nsISupportsArray to my special array object - but all languages will need to also implement that under-specified skeleton which would be good to avoid...

*** Bug 341423 has been marked as a duplicate of this bug. ***

Comment on attachment 225550 [details] [diff] [review] Updated patch with dont_AddRef and a couple of linebreak changes. >Index: src/nsWindowWatcher.cpp >@@ -356,27 +357,50 @@ nsWindowWatcher::OpenWindow(nsIDOMWindow >+ nsCOMPtr<nsISupportsArray> supArray; >+ supArray = do_QueryInterface(aArguments); nsCOMPtr<nsISupportsArray> supArray = do_QueryInterface(aArguments); (as in, no need for the two lines). Also, you need to allocate an nsIMutableArray no matter what, right? Why not just allocate it outside the if/else completely and not have the duplicated code? >+ if (supArray == nsnull) { if (!supArray) { > tempArray->AppendElement(aArguments, PR_FALSE); For what it's worth, you should be checking the rv of that... > argsArray = do_QueryInterface(tempArray); This could just be an assignment -- nsIMutableArray inherits from nsIArray. In fact, why even have a separate variably? Just make argsArray an nsIMutableArray and alloc directly into it. Note that the old behavior was to not attach an arguments array if a zero-length nsISupportsArray was passed in. You probably want to continue doing that. >+ for (i=0;i<count;i++) { Spaces around the '=' and '<', and after the ';', please. >+ nsCOMPtr<nsISupports> elt(dont_AddRef(supArray->ElementAt(i))); >+ if (elt == nsnull) >+ return NS_ERROR_UNEXPECTED; Why? I see nothing prohibiting a null in the middle of the array -- we used to convert it to JSVAL_NULL. Just put the null into then nsIArray and be done with it. >+ if (argsArray == nsnull) >+ return NS_ERROR_UNEXPECTED; This can go away if you allocate outside the if/then.

Oops, we already decided to not do the "no arguments if passed a 0-length nsISupportsArray" thing.

Comment on attachment 225626 [details] [diff] [review] Updated to comments, really >Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp >+ if (!supArray) { Why not just set argc to 1 in this case? >+ } else { And use argc instead of |count| here. > argsArray->GetLength(&argc); And nuke this line. r+sr=bzbarsky with that.

Fix checked into trunk.

I was able to install (and use) the Smoothwheel extension XPI just fine with build 2006-06-14-20 of SeaMonkey trunk on Windows XP. Verified FIXED.