I can download a file once perfectly, but after the first download I can no longer download any more files. The "action" dialog comes up, but when I select "save" it just goes away and no save dialog appears. Build is is 2000100215. OS distro is Mandrake 7.1 Asa (#mozillazine) has confirmed this bug as well with 100306 trunk linux build on RedHat 6.2

by coicidence I was looking into this the other night as well. Turns out it looks like an xpconnect problem which effects the file picker component (which is implemented in JS). jband is looking into the cause today...

Confirming for Asa, who told me he saw this today.

It turns out that the JS Component Loader is fouling up the JSObject parenting. There are two main things wrong: 1) Before the fix to bug 52936 the loader was using the same JSContext (of its own manufacture) regardless of thread. The fix there uses the thread context stack to get a good context to run on. Unfortunately, it is trying to 'peek' on the stack to find a current context before falling back on getSafeContext. This is unfortunate because the stack could have a 'running' context and when the loader calls JS_ExecuteScript it looks like a function call to the engine and we end up linking in the scope of the frame on the JSContext from the stack. So, we would end up with some active window in the scope chain of the objects created in the component. When that window later closes xpconnect's scheme for finding *its* scope via a lookup of the Components object is whacked because the closed window's global object has no more Components object and data conversion in calls to the JS component fail with the infamous: "No 'Components' in scope!" assertion. My fix here is to use only getSafeContext and not try to peek on the stack at all. By using getSafeContext we are assured of getting either: the JSContext of the hidden window or a plain vanilla JSContext that xpconnect will make for us. 2) The loader's superglobal stuff was whacked. JSObjects in JS components were getting the superglobal as their parent rather than the wrapped 'backstagepass' global object that was intended. (shaver added this wrapped backstagepass object as a way to extend security principals to scripts created in the scope of the component). The parenting was not working as expected. Having a Components object hanging off the superglobal was messy and the fact that there was a shared component manager wrapper would cause wrong parenting of any wrappers built through calls to that wrapped componenent manager. My fix for this part is to just get rid of the superglobal. I use nsIXPConnect::InitClassesWithNewWrappedGlobal instead. This allows us to have a wrapped native as a global object without a superglobal. I changed it to wrap the component manager once for each scope. The cost of that is not important. This fixes the problem for me in my test case. nsSidebar continues to work also. I'm going to attach a few items: a test case, a diagnostic patch, and the proposed fix. The test case... Since this xp filepicker is being used on 'nix only and I prefer NT, I made a page to launch the file picker from NT. The file picker is in /xpfe/components/filepicker/src/nsFilePicker.js and is not copied to the components directory by default on other platforms. To run my test... 1) copy nsFilePicker.js to the components directory. (the filename can be changed if you want) 2) change contractid in nsFilePicker.js (to avoid conflict)... "@mozilla.org/filepicker;1"; -> "@mozilla.org/filepicker;2"; 3) save the attached test html file to local file system (c:\temp\picker.html) 4) start browser and load file:///c|/temp/picker.html (use file: url to avoid security hassles) 5) the picker should come up - it might not do this on first run after modifying the JS component (this is another fun bug). Shutdown the browser and rerun if the picker does not come up. 6) close the picker 7) open a new browser window (in the same process) 8) close the first window 9) load file:///c|/temp/picker.html With the current build the picker will not launch in the second window after closing the first, and you should see the no 'Components' in scope assertion. If you apply the proposed fix to js/src/xpconnect/loader and rebuild there then the test above should work and you should be able to run the picker over and over again. The other cases where the unmodified picker does not run twice should, of course, also be fixed. The diagnostic patch... I'm attaching a patch to xpconnect/src to add a debugger callable "DumpJSObject" function. This can be called in MSDEV (at least) using a watch. You can inspect a variable to get the JSObject* address (say 0x133f7f8) and then watch "DumpJSObject((JSObject*)0x133f7f8)" If you set the 'current frame' in the debugger's callstack window to a frame in the xpconnect module then the code will dump to stdout something like the following. (this is like the stack dumper explained in http://www.mozilla.org/scriptable/javascript-stack-dumper.html) Debugging reminders... class: (JSClass*)(obj->slots[2]-1) parent: (JSObject*)(obj->slots[1]) proto: (JSObject*)(obj->slots[0]) 0x133f7f8 'native' <Object> parent: 0x1234550 'native' <global> parent: 0x12331c8 'native' <Window> parent: null proto: 0x1233368 'native' <Object> parent: 0x12331c8 'native' <Window> (SEE ABOVE) proto: null proto: 0x1233368 'native' <Object> (SEE ABOVE) proto: 0x12345c0 'native' <Object> parent: 0x1234550 'native' <global> (SEE ABOVE) proto: 0x1233368 'native' <Object> (SEE ABOVE) (The reminders are for me - I can never rememeber this stuff) This gives address, 'native' or 'host', and object classname. It then shows the parent and proto of the object. Objects previously seen say: (SEE ABOVE) and don't list parent and proto. ................................... For the real hardcore here are the parent and proto graphs before and after my changes when loading the nsFilePicker.js component... BEFORE... notes: The superglobal is of classname "global"! The wrapped backstagepass global is of classname XPCWrappedNativeWithCall The 'window' is the current main window - not the hidden window. Dump of the global object: 0x133f268 'native' <XPCWrappedNativeWithCall> parent: null proto: 0x1234550 'native' <global> parent: 0x12331c8 'native' <Window> parent: null proto: 0x1233368 'native' <Object> parent: 0x12331c8 'native' <Window> (SEE ABOVE) proto: null proto: 0x1233368 'native' <Object> (SEE ABOVE) This is pretty reasonable. Except that we had no plan that the Window would be the parent of the superglobal! dump of the 'module' returned from NSGetModule() (a plain JSObject): http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/src/nsFilePic ker.js#183 0x133f7f8 'native' <Object> parent: 0x1234550 'native' <global> parent: 0x12331c8 'native' <Window> parent: null proto: 0x1233368 'native' <Object> parent: 0x12331c8 'native' <Window> (SEE ABOVE) proto: null proto: 0x1233368 'native' <Object> (SEE ABOVE) proto: 0x12345c0 'native' <Object> parent: 0x1234550 'native' <global> (SEE ABOVE) proto: 0x1233368 'native' <Object> (SEE ABOVE) This is horked - the superglobal is the parent and its parent is the window!?! The *real* global is nowhere to be found. AFTER... notes: No longer using superglobal. 'window' is hidden window. Dump of the global object: 0x1346230 'native' <XPCWrappedNativeWithCall> parent: null proto: 0x1346278 'native' <Object> parent: 0x1346230 'native' <XPCWrappedNativeWithCall> (SEE ABOVE) proto: 0x116bcd8 'native' <Object> parent: 0x116b720 'native' <Window> parent: null proto: 0x116bcd8 'native' <Object> (SEE ABOVE) proto: null looks ok to me. dump of the 'module' returned from NSGetModule() (a plain JSObject): 0x1346ee8 'native' <Object> parent: 0x1346230 'native' <XPCWrappedNativeWithCall> parent: null proto: 0x1346278 'native' <Object> parent: 0x1346230 'native' <XPCWrappedNativeWithCall> (SEE ABOVE) proto: 0x116bcd8 'native' <Object> parent: 0x116b720 'native' <Window> parent: null proto: 0x116bcd8 'native' <Object> (SEE ABOVE) proto: null proto: 0x1346278 'native' <Object> (SEE ABOVE) also looks good to me. Also, I'll look for a bug on the 'first run' problem. We used to think this kind of thing was mostly a matter of the component getting created early to do its RegisterSelf and getting stuck with a non-DOM JSContext as the safeContext. In my test case I'm seeing the problem that "netscape.security.PrivilegeManager.enablePrivilege" silently fails because (I think) the security manager got initialized too early and didn't properly setup this namespace and so *no* DOM windows can process this call - it is not even getting to really calling the JS component. Anyway... I could stand some more testing of these patches. I'd like to get them both in to at least the trunk sometime soon. comments? reviews? approvals? Thanks for reading! John.

adding the rtm keyword. PDT: because of this bug, you can only invoke the file picker from the helper dialog ONCE per session. i.e. click on a link that results in the helper app dialog coming up. Click on Save. The file picker asks you for a location to save the file to. Now try it again. the file picker doesn't come up. jband, thanks so much for looking into this tonight. I'm trying your patch out right now.

your patch definetly fixes my problem. So far things are running good.

JSCLAutoContext::~JSCLAutoContext() { - if (mContext && mContextThread) { - JS_EndRequest(mContext); + if (mContext) { + if (mContextThread) { + JS_EndRequest(mContext); + } } if (mPopNeeded) { What's up with that change? Is it purely cosmetic (the file wasn't long enough before?), or amy I missing some subtlety? Other than that (and the use of ``glob'' to name a global: ``glob'' means something else, especially in code that deals with file paths!), r=shaver and thanks for the cleanup.

shaver, that change is an artifact of an experiment I did with having JSCLAutoContext swap out the global object of the context from the stack: @@ -1141,6 +1143,11 @@ if (mContextThread) { JS_BeginRequest(mContext); } + mGlobal = JS_GetGlobalObject(mContext); + if (mGlobal) { + JS_AddNamedRoot(mContext, &mGlobal, "JSCLAutoContext::mGlobal"); + } + JS_SetGlobalObject(mContext, nsnull); } else { if (NS_SUCCEEDED(mError)) { mError = NS_ERROR_FAILURE; @@ -1150,8 +1157,14 @@ JSCLAutoContext::~JSCLAutoContext() { - if (mContext && mContextThread) { - JS_EndRequest(mContext); + if (mContext) { + JS_SetGlobalObject(mContext, mGlobal); + if (mGlobal) { + JS_RemoveRoot(mContext, &mGlobal); + } + if (mContextThread) { + JS_EndRequest(mContext); + } } if (mPopNeeded) { It turned out that the object apparently added to the scopechain was from cx->fp and not cx->globalObject. I left in the insignificant change. on glob: 'obj' was too generic and I was getting confused. js.c uses 'glob'. Thanks.

over to xpconnect qa owner...

jband: a= on those patches, get shaver to give an r= by renaming glob to global or some such. Thanks for fixing this. That DumpJSObject rules (there is also printObj over in jsobj.c, #ifdef DEBUG, but it's old and pretty wimpy). /be

I dealt with shaver's concerns and checked this into the trunk. What do I have to do to get permission for the branch?

to get it in on the branch, we need to do a couple of things: 1) we need a manager to mark the bug rtm+ 2) we need to list a sr= and r= in the bug report 3) ping PDT and they will turn it into a rtm++ assuming they agree with the severity. If not, then I'm willing to go talk to them to try to convince 'em....

mscott: I gave a= (same as sr=, too many ways to abbreviate). shaver should give an explicit r=, but he's already reviewed. I think we just need an [rtm+] in the status whiteboard, which I'm taking the liberty of doing for warren. /be

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

PDT marking [rtm++]

fix checked into branch too.

Verified Fixed on WinNT, Linux, and Mac on trunk and branch. Using 20001105xx trunk and commercial builds. I used the test procedure John described above at 2000-10-04 00:30, with one extra step: since release builds do not run component AutoRegistration at startup, I had to delete the component.reg file ("Component Registry" on the Mac) to make that happen . On Linux, where the XP filepicker runs by default, I simply invoked it via File|Open File instead of of running the local testcase John created.