1) do first inital load of a JS component from a worker thread. This will cause mozJSComponentLoader to allocate a JS context from the worker thread. 2) Shut down XPCOM from main thread. This will cause an assert in js_GetSlotWhileLocked. #ifndef NSPR_LOCK, JS_ASSERT(me == CurrentThreadId()); The problem is that the context was created on the worker thread and then destroyed from the main thread.

Changing bug bugzilla "Component" to XPConnect (because the JS loader does not have its own Component in bugzilla). and reassigning to shaver. Shaver, do you still own the JS loader or what? Do you want me to own it? I sometimes wonder if we shouldn't just merge it into the xpconnect module.

mozJSComponentLoader.cpp line 241 I needed to change to the threadsafe version NS_IMPL_THREADSAFE_ISUPPORTS(mozJSComponentLoader, NS_GET_IID (nsIComponentLoader));

I tried three different work arounds without success. Component loader needs to support per thread contexts if this is ever going to work.

Shaver's in Washington, D.C., should be back in a day or two. /be

This is a temporary patch to get around the problem. It does not fix the complete problem. A lock is used to completely serialize JS component creation. Current thread id is set into the shared JS context before it is used. cvs diff mozJSComponentLoader.h (in directory d:\cvs\mozilla\js\src\xpconnect\loader\) Index: mozJSComponentLoader.h =================================================================== RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.h,v retrieving revision 1.12 diff -r1.12 mozJSComponentLoader.h 70a71 > PRLock* mSerialize; *****CVS exited normally with code 1***** cvs diff mozJSComponentLoader.cpp (in directory d:\cvs\mozilla\js\src\xpconnect\loader\) Index: mozJSComponentLoader.cpp =================================================================== RCS file: /cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v retrieving revision 1.48 diff -r1.48 mozJSComponentLoader.cpp 191a192 > mSerialize = PR_NewLock(); 226a228 > JS_SetContextThread(mContext); 238a241 > PR_DestroyLock(mSerialize); 241c244 < NS_IMPL_ISUPPORTS(mozJSComponentLoader, NS_GET_IID(nsIComponentLoader)); --- > NS_IMPL_THREADSAFE_ISUPPORTS(mozJSComponentLoader, NS_GET_IID (nsIComponentLoader)); 258a262 > PR_Lock(mSerialize); 259a264,265 > PR_Unlock(mSerialize); > 886a893 > JS_SetContextThread(mContext); 973a981 > JS_SetContextThread(mContext); 1105a1114 > JS_SetContextThread(mContext); *****CVS exited normally with code 1*****

Jon, diff -u format patches attached to the bug, not inserted in comments, are the way to go. I haven't had a chance to study your patch, but it may just be papering over those assertbotches -- JS_SetContextThread is not enough, you must use a JSContext on at most one thread at a time. You can share a JSContext among threads only by serializing batches of JS API calls using that context. Is that what the lock you added does, in all cases (including destruction)? /be

The context in the js loader is only used for compiling the script. During method calls XPConnect provides me with the safe context on each call. I don't think there is a need for per thread contexts in the js loader if the jsloader has been serialized to allow only a single createInstance to happen at a time. That's what my patch did. My patch didn't deal with things like registration. Note that thread safe issues in createInstance are broader than just the JS loader. http://bugzilla.mozilla.org/show_bug.cgi?id=52718

I don't much like the idea of holding a lock while the JS executes. I think the thing to do is to not even create a JSContext in this code. We can get one from the js thread context service. It should be the component manager's job to ensure that calls to the loader are serialized (and don't nest on the same thread!). We ought to fix that problem in the component manager, not in each loader. The JS loader *should* protect its mutable state, however. It also needs to use Begin/End Request. I'll do some work on this and attach a patch later.

Why would we want to avoid nesting on the same thread? Does that rule exist for the native loader as well? I guess it's part of only doing a minimal amount of infallible work in the constructor, but I'm not sure the codebase is up to the challenge. And what should the component mgr do if it detects that it's about to nest? Return failure?

I forgot about nesting. What if the JS code being compiled loads another JS component? Obviously my lock won't work. These issues may also be the source of my double GC problem which does a nested createInstance. I was very surprised to discover that CreateInstance was not thread safe for creating any components, JS or otherwise. This is probably worth a code review and some test cases.

Shaver, if loading 'a' calls 'b' which asks that we load 'a' then we're likely to go into a stack blowing cycle. This is a coding error that the component manager ought to catch and return an error.

Assuming that the component manager gets fixed to serialize and detect nested calls to GetFactory, then I think this will pretty much do it. I assume that other calls into the loader are only going to happen on the main thread. I did not add any explicit locking here. This is still playing things loosly. There is also more JS loader stuff to fix: - I still have bug 50611 regarding shutdown errors that I thought were fixed. - grinda has changes for his subscript loader. - I'm wondering why we don't do any exception cleanup when we call directly into JS from the loader. - And, I was serious about the idea of just moving the whole thing into xpconnect. Thoughts?

I'm going to try the new patches right now. Has the case of a JS component createInstancing another JS component from createInstance be allowed for? I'm thinking of just a normal case, not a pair of creates causing infinite recursion. When doing createInstance I hit the threadsafe assert in these files too. This implies that there are five more components that need to be checked for thread safety. nsComponentManager.cpp lines: 403, 513 nsRegistry.cpp lines: 341, 1946 nsCategoryManager.cpp line: 152 I'm hitting the shut down assert that you mention too.

WTF is that PRUnichar thing, indeed? Must predate the decision to use the normal XPCOM naming stuff. Just lose it, don't preserve the (my?) shame. Cute autoclasses -- I blame brendan for my goto affliction. r=shaver I guess we might as well move the loader into XPConnect. I just liked the idea of having it be its own, independently-replaceable component, but I can get past my childish dreams in the name of fewer libraries and build directories. Rename to nsJSComponentLoader when you relocate it to xpconnect/src? Lack of exception cleanup is a bug, and another source of shame. You want to get it, or should I try to find time?

Compiler errors from xpcom standalone environment on Windows. My CVS is a couple days old so I'm pulling and building a new one. D:\CVS\mozilla\js\src\xpconnect\shell>..\..\..\..\config\makecopy.exe .\WIN32_D. OBJ\xpcshell.exe ..\..\..\..\dist\WIN32_D.OBJ\bin w95make: Leaving Directory d:\cvs\mozilla\js\src\xpconnect\shell with target ins tall w95make: Entering Directory d:\cvs\mozilla\js\src\xpconnect\loader with target i nstall mozJSComponentLoader.cpp d:\cvs\mozilla\js\src\xpconnect\loader\mozJSComponentLoader.cpp(72) : error C206 5: 'NS_SCRIPTSECURITYMANAGER_CONTRACTID' : undeclared identifier d:\cvs\mozilla\js\src\xpconnect\loader\mozJSComponentLoader.cpp(72) : error C244 0: 'initializing' : cannot convert from 'int' to 'const char []' There are no conversions to array types, although there are conversions to references or pointers to arrays d:\cvs\mozilla\js\src\xpconnect\loader\mozJSComponentLoader.cpp(73) : error C206 5: 'NS_OBSERVERSERVICE_CONTRACTID' : undeclared identifier d:\cvs\mozilla\js\src\xpconnect\loader\mozJSComponentLoader.cpp(73) : error C244 0: 'initializing' : cannot convert from 'int' to 'const char []' There are no conversions to array types, although there are conversions to references or pointers to arrays d:\cvs\mozilla\js\src\xpconnect\loader\mozJSComponentLoader.cpp(1186) : error C2 065: 'NS_CATEGORYMANAGER_CONTRACTID' : undeclared identifier NMAKE : fatal error U1077: 'D:\PROGRA~1\MICROS~2\VC98\BIN\cl.exe' : return code '0x2' Stop. w95make: nmake failed in directory loader with error code 2 NMAKE : fatal error U1077: '..\..\..\config\w95make.exe' : return code '0x2' Stop. w95make: nmake failed in directory xpconnect with error code 2 NMAKE : fatal error U1077: '..\..\config\w95make.exe' : return code '0x2' Stop. NMAKE : fatal error U1077: '"D:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\VC98\BIN\N MAKE.exe"' : return code '0x2' Stop. NMAKE : fatal error U1077: '"D:\PROGRAM FILES\MICROSOFT VISUAL STUDIO\VC98\BIN\N MAKE.exe"' : return code '0x2' Stop. D:\CVS\mozilla>

How embarrassing to miss that on review. I rescind my r=shaver until this atrocity is repaired!

Shaver, You're joking right? Jon's tree clearly predates the progid->contractid renaming.

Accepting the bug. I'll fix the remaining stuff mentioned.

Your right, I just had 500 new files come down. That why it is taking me so long to build.

New attachment is the same as before except that I moved one line inside a #ifndef XPCONNECT_STANDALONE block to correctly deal with #defined string from scriptsecuritymanager.

I have it built and installed now. It will take me a couple of hours to verify that it is working. I'm still worried about the other components referenced in http://bugzilla.mozilla.org/show_bug.cgi?id=52718. Ray's comment leads me to believe this is not being worked on. Using XPCOM in a server environment requires that createInstance be threadsafe.

I'm getting an assert at shutdown. It only happens when a JS component is loaded. I get the same assert if I register a JS component with regxpcom. Other that this assert I seem to be functioning. I'm only walking it through with the debugger so I'm not really stressing threading issues. Apache/1.3.12 (Win32) PX7/0.9 running... Type Manifest File: d:\testdll\components\xpti.dat ###!!! ASSERTION: Component Manager being held past XPCOM shutdown.: 'cnt == 0', file d:\cvs\mozilla\xpcom\build\nsXPComInit.cpp, line 667 Press any key to continue Stack trace: nsDebug::Assertion(const char * 0x00c80180, const char * 0x00c80174, const char * 0x00c80148, int 0x0000029b) line 215 + 22 bytes nsDebug::WarnIfFalse(const char * 0x00c80180, const char * 0x00c80174, const char * 0x00c80148, int 0x0000029b) line 358 + 21 bytes NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 667 + 31 bytes PX7Servlet::childTerm() line 81 + 10 bytes ChildTerm(server_rec * 0x008762dc, pool * 0x008762b4) line 40 ap_child_exit_modules(pool * 0x008762b4, server_rec * 0x008762dc) line 1537 + 14 bytes worker_main() line 6040 + 18 bytes apache_main(int 0x00000007, char * * 0x00981f50) line 6874 main(int 0x00000007, char * * 0x00981f50) line 15 + 13 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! bff89349() KERNEL32! bff891fb() KERNEL32! bff87c38()

That's been around forever. It seems harmless. I can't find a bug number. I dug in once and did not see a clear way to makeit go away.

marking fixed. My patches when in the on Sept 21.