Even when no arguments are passed to openDialog "arguments" in window evaluates to true with recent trunk. http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#6080 http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.js#544 Reproduced on WinXP and Mac OS X

I suspect the patch from bug 341362?

btw: the ui this affects is in Firefox -> Tools -> Add-ons

Why wouldn't it be true? arguments isn't an array object, but it is an object. Only six values evaluate to false in JS: false, undefined, 0, null, [], and "".

(In reply to comment #3) > Why wouldn't it be true? arguments isn't an array object, but it is an object. This is about the statement |"arguments" in window|, not |arguments| itself.

(though I'm surprised that that check ever worked)

In any case, I was wrong: Boolean(window.arguments) evaluates to false for Firefox 1.5.0.4 on this bug page.

(In reply to comment #6) > In any case, I was wrong: Boolean(window.arguments) evaluates to false for > Firefox 1.5.0.4 on this bug page. The arguments object is only attached to chrome windows.

A quick scan of the tree for areas that are likely to be affected by this bug. http://lxr.mozilla.org/seamonkey/source/security/manager/pki/resources/content/resetpassword.js#44 http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#662 http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/viewsource.js#112 http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/addBookmark.js#123 http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#560 http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/plugins/content/pluginInstallerWizard.js#68 http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/addBookmark.js#107 http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/places.js#64 http://lxr.mozilla.org/seamonkey/source/mail/base/content/msgMail3PaneWindow.js#820 http://lxr.mozilla.org/seamonkey/source/mail/components/migration/content/migration.js#60 http://lxr.mozilla.org/seamonkey/source/suite/common/pref/pref-languages.js#81 I also verified that this worked in Firefox 1.0.

After reading the comments in bug 341362 I am quite sure it is the cause of this bug.

(In reply to comment #9) > After reading the comments in bug 341362 I am quite sure it is the cause of > this bug. Hmm, I'm not so sure. From debugging, it appears that we manage to run down an entirely different code path (nsWindowWatcher::OpenWindowJS instead of nsWindowWatcher::OpenWindow). On the other hand, it seems that not imitating the same sort of change in nsWindowWatcher::OpenWindow stands a much higher chance of breaking people than previously thought, so it's probably worth restoring that compatibility.

(Noting here since it's late): A further problem is that nsJSArgArray doesn't implement nsISupportsArray.

hmmm... I think you ate correct that this isn't caused by the patch for bug 341362. It looks like the patch from bug 255942 is more likely the cause.

So, uh, I have this patch here, but don't really know how to see if it's actually made a difference. Are there any steps that I can follow that will let me check to make sure that my patch really works?

I can compile / test it if you like, if you have Firefox compiled you can check if the only button displayed along the bottom when opening the Add-ons Mgr is the "Find Updates" button, or you can check if arguments is in window when using openDialog as is done as follows: http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#6080 http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.js#544

Okay, I *think* this passes those instructions. Sadly, the code in nsWindowWatcher::OpenWindow got pretty ugly.

Sure: open minefield, choose Tools > Add-Ons If there is something listed, your patch works

Comment on attachment 227273 [details] [diff] [review] patch, v1 >Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp > // aArguments is allowed to be either an nsISupportsArray (in which case > // it is treated as argv) or any other COM object (in which case it > // becomes argv[0]). Fix this comment to reflect the new reality? >+ if (!array) { ... >+ } else { Just use aArguments for the args array; no reason to make a copy. I realize argsArray is an nsIMutableArray, but now that we allocate it in different places, maybe we should make it be nsIArray, and just have local nsIMutableArray ptrs with assignment to argsArray as needed. With those changes, looks good.

This should be all updated to comments.

Sorry, forgot the comment change.

Comment on attachment 227276 [details] [diff] [review] Really updated Looks good.

Fix checked into trunk.

Hi guys - sorry for letting some of these slide, and thanks for getting them sorted - this change also fixes the Python demos that got broken in the fallout from the initial landing.