341362 - Extensions installation (.XPI) broken in trunk

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ria Klaassen (not reading all bugmail)]

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

Comment 1

[Ria Klaassen (not reading all bugmail)]

Attachment: Smoothwheel extension

Comment 2

[Ria Klaassen (not reading all bugmail)]

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

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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

Comment 4

[Ria Klaassen (not reading all bugmail)]

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

Comment 5

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot of the problem (watch the grayed-out Restart Minefield button)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Patch that should convert nsISupportsArray again

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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));

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated patch with dont_AddRef and a couple of linebreak changes.

> (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...

Comment 11

[Tobias Fischer]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments, really

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

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.