Closed Bug 526586 Opened 13 years ago Closed 13 years ago

Crash in [@ nsThreadManager::Init()] when accessing TLS variable


(Core :: XPCOM, defect)

Windows XP
Not set



Tracking Status
status1.9.2 --- beta3-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed
fennec 1.0+ ---


(Reporter: mfinkle, Assigned: benjamin)



(Keywords: crash, regression, topcrash)

Crash Data


(2 files, 4 obsolete files)

Bug 521750 added a TLS global variable to nsThreadManager. On Windows XP (debug and release) builds of Fennec + XULRunner, we are crashing when the TLS variable is accessed in nsThreadManager::Init

Call stack:

>	xul.dll!nsThreadManager::Init()  Line 110 + 0xc bytes	C++
 	xul.dll!NS_InitXPCOM3_P(nsIServiceManager * * result=0x0012ec5c, nsIFile * binDirectory=0x009abf00, nsIDirectoryServiceProvider * appFileLocationProvider=0x0012ed74, const nsStaticModuleInfo * staticComponents=0x023266f8, unsigned int componentCount=50)  Line 477 + 0xc bytes	C++
 	xul.dll!ScopedXPCOMStartup::Initialize()  Line 1073 + 0x29 bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00908b10, const nsXREAppData * aAppData=0x009abe08)  Line 3278 + 0xb bytes	C++
 	fennec.exe!NS_internal_main(int argc=1, char * * argv=0x00908b10)  Line 502 + 0x1a bytes	C++
 	fennec.exe!wmain(int argc=1, wchar_t * * argv=0x00904f60)  Line 110 + 0xd bytes	C++
 	fennec.exe!__tmainCRTStartup()  Line 327 + 0x19 bytes	C
 	fennec.exe!wmainCRTStartup()  Line 196	C


#ifdef NS_TLS
  gTLSIsMainThread = true;
01D8FEA8  mov         edx,dword ptr [__tls_index (2B2E75Ch)] 
01D8FEAE  mov         eax,dword ptr fs:[0000002Ch] 
01D8FEB4  mov         ecx,dword ptr [eax+edx*4] <=== CRASH
01D8FEB7  mov         byte ptr [ecx+101h],1 

eax and edx are both 0

I checked with a release bug from here:

The debug build was from my own m-c tree.
__declspec(thread) doesn't work in a DLL loaded dynamically with
LoadLibrary (such as, for example, a COM DLL). The only way around is to
drop __declspec(thread) and use thread-local strorage APIs (TlsAlloc et
al) directly.

This is a pretty unfortunate regression!
We need to fix this for, but won't block on it since it only affects XULRunner/embedded.

This probably needs to block 1.9.2 as well.
blocking1.9.1: --- → .6+
Flags: blocking1.9.2?
Releng is going to push XR out as official. We were going to skip and got straight to, but we can change that plan.
Keywords: regression
dbaron: Can you own this? This blocks which freezes on November 10 at 11:59pm.
Whiteboard: [needs owner]
I know almost nothing about this stuff; I think Benjamin would be a better owner.
Assigning this to Benjamin for now then.
Assignee: nobody → benjamin
Whiteboard: [needs owner]
Attached patch untested (obsolete) — Splinter Review
so, this is the lazy patch. i think dbaron should be able to review it, since it's written using gecko (nspr) style code.

one more advanced approach would involve distinct functions and a function pointer to deal with the differences between w5 and w6.
Comment on attachment 410463 [details] [diff] [review]

The whole point of the fast check was to avoid the extra function all overhead of the NSPR functions. I don't know yet whether direct calls to TlsAlloc/TlsGetValue will have smaller overhead.
Attachment #410463 - Flags: review-
Attached patch untested using winapi (obsolete) — Splinter Review
Attachment #410463 - Attachment is obsolete: true
Attachment #410489 - Flags: review?(benjamin)
This compiles... going to run through tryserver momentarily.
Attachment #410489 - Attachment is obsolete: true
Attachment #411230 - Flags: review?
Attachment #410489 - Flags: review?(benjamin)
Attachment #411230 - Flags: review? → review?(dbaron)
this is holding the beta 5 release due to desktop win32 build being dead.
tracking-fennec: --- → 1.0+
Flags: blocking1.9.2? → blocking1.9.2+
Again, with #undef ERROR for treewide sanity.
Attachment #411230 - Attachment is obsolete: true
Attachment #411269 - Flags: review?(dbaron)
Attachment #411230 - Flags: review?(dbaron)
Attachment #411269 - Attachment is obsolete: true
Attachment #411269 - Flags: review?(dbaron)
I think stuart's objection to bsmedberg's latest patch was:

[2009-11-09 15:38:51] <stuart> yes, his patch doesn't work
[2009-11-09 15:39:11] <stuart> (you can't include windows.h in a core headerfile and hope for a real build)

Maybe, to fix that, we should just not bother with the inline on Windows?
My latest version, does exactly that, but I'm worried about the performance costs of the non-inline version, since this is a hot path and the inline can make it almost disappear but the called version can't so much. If m-c is relatively clear somebody is welcome to try it today, or I can do it in the morning tomorrow which is likely to be quieter.
Closed: 13 years ago
Resolution: --- → FIXED
dbaron: I hear Benjamin isn't fully around today. Can you port this over to 1.9.1 before code freeze tonight?
Comment on attachment 411440 [details] [diff] [review]
Final patch as checked in

We should get this onto 1.9.2 and 1.9.2... and also reland the other patches on 1.9.2.
Attachment #411440 - Flags: review+
Attachment #411440 - Flags: approval1.9.1.6?
Comment on attachment 411440 [details] [diff] [review]
Final patch as checked in

Approved for a=ss
Attachment #411440 - Flags: approval1.9.1.6? → approval1.9.1.6+
Summary: Crash in nsThreadManager::Init when accessing TLS variable → Crash in [@ nsThreadManager::Init()] when accessing TLS variable
Severity: normal → critical
Keywords: crash, topcrash
Crash Signature: [@ nsThreadManager::Init()]
You need to log in before you can comment on or make changes to this bug.