52936 - JS Component loader is not threadsafe

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P3)

Description

[Jon Smirl]

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.

Comment 1

[John Bandhauer]

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.

Comment 2

[Jon Smirl]

mozJSComponentLoader.cpp line 241

I needed to change to the threadsafe version
NS_IMPL_THREADSAFE_ISUPPORTS(mozJSComponentLoader, NS_GET_IID
(nsIComponentLoader));

Comment 3

[Jon Smirl]

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

Comment 4

[Brendan Eich [:brendan]]

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

/be

Comment 5

[Jon Smirl]

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*****

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Jon Smirl]

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

Comment 8

[Jon Smirl]

Attachment: Same patch in diff -u format

Comment 9

[John Bandhauer]

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.
 

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 11

[Jon Smirl]

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.

Comment 12

[John Bandhauer]

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.

Comment 13

[John Bandhauer]

Attachment: Proposed patch

Comment 14

[John Bandhauer]

Attachment: proposed cvs checkin comment

Comment 15

[John Bandhauer]

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?

Comment 16

[Jon Smirl]

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.

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 18

[Jon Smirl]

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>

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 20

[John Bandhauer]

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

Comment 21

[John Bandhauer]

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

Comment 22

[Jon Smirl]

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

Comment 23

[John Bandhauer]

Attachment: new proposed fix with XPCONNECT_STANDALONE fix

Comment 24

[John Bandhauer]

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.

Comment 25

[Jon Smirl]

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.

Comment 26

[Jon Smirl]

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

Comment 27

[John Bandhauer]

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.

Comment 28

[John Bandhauer]

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