Closed
Bug 98755
Opened 23 years ago
Closed 22 years ago
nsNativeComponentLoader not thread-safe error?
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: leif, Assigned: dougt)
References
Details
Attachments
(2 files, 2 obsolete files)
5.50 KB,
patch
|
brendan
:
review+
jband_mozilla
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
3.35 KB,
patch
|
waterson
:
review+
jband_mozilla
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Hi,
I'm not sure what happend, but I'm getting assertions now, like
###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread ==
NS_CurrentThread()', file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp,
line 528
###!!! Break: at file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line
528
###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread ==
NS_CurrentThread()', file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp,
line 528
###!!! Break: at file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line
528
###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread ==
NS_CurrentThread()', file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp,
line 528
###!!! Break: at file /export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line
528
I thought this was something I had done in my code. But, I backed out all my
changes, and it's still happening. I had a bried discussion with Shaver last
night on IRC, which I'm attaching here. Dmose also says he's seeing the same
problem, and he'll post more information as well.
Thanks,
-- leif
From IRC w. shaver:
<leif> quick question, just need a hint where to start looking. I get this
error, apparently I'm running code on the "wrong" thread:
<leif> ###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread
== NS_CurrentThread()', file
/export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line 528
<leif> ###!!! Break: at file
/export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line 528
<leif> ###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread
== NS_CurrentThread()', file
/export/disk1/opensrc/mozilla/xpcom/base/nsDebug.cpp, line 528
<leif> how can I debug this?
<shaver> that sucks
<leif> hehe
<leif> I'm screwed? :)
<shaver> it looks like you're creating an instance (the first one from a new
component) on a non-main thread
<shaver> that should be fine
<shaver> can you file a bug on that, against me?
<shaver> cc: dougt and dp
<leif> ehhh, but, this must be something I'm doing wrong...
<leif> I rearranged some code, and it suddenly started happening
<shaver> well, it looks like you're calling into the component manager on a
non-main thread
<shaver> and it has to load a new component
<shaver> so it's touching the native component loader on that Other thread, and
the assertions are bitching
<leif> ok, makes sense I think, not sure which component I'm loading. But, I do
use the Proxy objects heavily here, to cause the callbacks to happen on a main
thread
Reporter | ||
Updated•23 years ago
|
Summary: nsNativeComponentLoader not thread-safe → nsNativeComponentLoader not thread-safe error?
Comment 1•23 years ago
|
||
This actually is unrelated to leif's patch; I started seeing these assertions on
the trunk about a week and a half ago after some component manager checkins by
dp; I'll go try and figure out which ones in a bit.
Note that this is actually being triggered because for some reason, the LDAP
addressbook code (see http://abzilla.mozdev.org) is being called at autocomplete
time, which it shouldn't be.
I run under gdb with the XPCOM_DEBUG_BREAK set to stack, and here's what the
first assertion's stack looks like:
(gdb) where
#0 0x40190a20 in nsDebug::Break (
aFile=0x401f20e0
"/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp", aLine=528) at
/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp:344
#1 0x40190895 in nsDebug::Assertion (
aStr=0x401ed8a0 "nsNativeComponentLoader not thread-safe",
aExpr=0x401f2120 "owningThread == NS_CurrentThread()",
aFile=0x401f20e0
"/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp", aLine=528) at
/home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp:290
#2 0x40190ee9 in NS_CheckThreadSafe (owningThread=0x8061f58,
msg=0x401ed8a0 "nsNativeComponentLoader not thread-safe")
at /home/dmose/s/main-browser/mozilla/xpcom/base/nsDebug.cpp:528
#3 0x401725df in nsNativeComponentLoader::AddRef (this=0x806b1d8)
at
/home/dmose/s/main-browser/mozilla/xpcom/components/nsNativeComponentLoader.cpp:87
#4 0x4016e3ce in nsComponentManagerImpl::GetLoaderForType (this=0x806a758,
aType=0, aLoader=0x432473ac)
at
/home/dmose/s/main-browser/mozilla/xpcom/components/nsComponentManager.cpp:2383
#5 0x4016b1b9 in nsComponentManagerImpl::FindFactory (this=0x806a758,
aClass=@0x4324744c, aFactory=0x432474dc)
at /home/dmose/s/main-browser/mozilla/xpcom/base/nsCOMPtr.h:1069
#6 0x4017085a in nsComponentManager::FindFactory (aClass=@0x4324744c,
aFactory=0x432474dc)
at
/home/dmose/s/main-browser/mozilla/xpcom/components/nsComponentManager.cpp:3021
#7 0x40964fde in RDFServiceImpl::GetResource (this=0x80f4c58,
aURI=0x89892c0
"moz-abldapcard://nsdirectory.netscape.com:-1/ou=People,dc=netscape,dc=com",
aResource=0x4324756c)
at ../../../dist/include/xpcom/nsCOMPtr.h:1069
#8 0x42bb743e in nsAbLDAPDirectory::CreateCard (this=0x8943a38,
uri=0x8964d58, dn=0x43247694 "ou=People,dc=netscape,dc=com",
card=0x4324766c) at ../../../dist/include/xpcom/nsCOMPtr.h:1069
#9 0x42bbbfaf in nsAbQueryLDAPMessageListener::OnLDAPMessageSearchEntry (
this=0x845ea20, aMessage=0x8432148, result=0x4324787c)
at ../../../dist/include/xpcom/nsCOMPtr.h:642
#10 0x42bba1d1 in nsAbQueryLDAPMessageListener::OnLDAPMessage (this=0x845ea20,
aMessage=0x8432148) at ../../../dist/include/xpcom/nsCOMPtr.h:1069
#11 0x42bfc399 in nsLDAPConnection::InvokeMessageCallback (this=0x893d658,
aMsgHandle=0x89887f8, aMsg=0x8432148, aRemoveOpFromConnQ=0)
at ../../../../dist/include/xpcom/nsCOMPtr.h:649
#12 0x42bfd4a5 in nsLDAPConnectionLoop::Run (this=0x8342c80)
at ../../../../dist/include/xpcom/nsCOMPtr.h:642
#13 0x40182cb9 in nsThread::Main (arg=0x83426d8)
at ../../dist/include/xpcom/nsCOMPtr.h:649
#14 0x40251339 in _pt_root (arg=0x89366e0)
at /home/dmose/s/main-browser/mozilla/nsprpub/pr/src/pthreads/ptthread.c:214
#15 0x40280d8c in pthread_start_thread (arg=0x43247be0) at manager.c:274
#16 0x40280e6f in pthread_start_thread_event (arg=0x43247be0) at manager.c:298
(gdb)
Comment 2•23 years ago
|
||
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=dp%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=08%2F30%2F2001+00%3A00&maxdate=08%2F30%2F2001+12%3A00&cvsroot=%2Fcvsroot
I believe I started seeing it shortly after the above checkin.
Comment 3•23 years ago
|
||
None of my changes should cause thread control flow changes. They were purely
reducing allocations of strings.
Let me do a debug build and see the assertions too.
Comment 4•23 years ago
|
||
Also the attached stack trace looks wierd:
- FindFactory() doesn't call GetLoaderForType()
- GetLoaderForType() is addrefffing native component loader ?
Comment 5•23 years ago
|
||
nsComponentManager::FindFactory
-> nsFactoryEntry::GetFactory (inline)
-> nsComponentManager::GetLoaderForType
GetLoaderForType addrefs the loader it hands back -- for a (most common) type of
0, that'll always be the native component loader.
Stack is fine, I think. I also think it's fine, or should be, to call the
component loader on a thread that is different from that which initialized XPCOM
(and thereby created the native component loader). The problem is really that
we haven't made the component manager and loaders be threadsafe, or at least
haven't verified that they are and used NS_IMPL_QUERYINTERFACE_THREADSAFE to
indicate as much.
Comment 6•23 years ago
|
||
I see. ComponentManager did go through one round of head scratching and bug
fixing for thread safeness early on (2 years ago). Native loader wasn't around
then. FYI
timeless@timeless-bsd:~/mozilla/obj-gtk-i386-unknown-freebsd4.4/dist/bin:
./run-mozilla.sh ./TestProtocols
"jar:resource:/chrome/classic.jar\!/skin/classic/global/scrollbars.css"
Type Manifest File:
/home/timeless/mozilla/obj-gtk-i386-unknown-freebsd4.4/dist/bin/components/xpti
.dat
Trying to load:
jar:resource:/chrome/classic.jar!/skin/classic/global/scrollbars.css
###!!! ASSERTION: nsNativeComponentLoader not thread-safe: 'owningThread ==
NS_CurrentThread()', file /home/timeless/mozilla/xpcom/base/nsDebug.cpp, line
528
[repeated 8 times]
shaver: what are your plans for this bug, will we proxy to the main thread,
mark thread safe, remove the warning, make the code work on other threads?
Assignee | ||
Comment 8•23 years ago
|
||
mike I have similar bugs for 1.01/1.1. If you want to reassign, feel free.
Assignee | ||
Updated•23 years ago
|
Target Milestone: --- → mozilla1.1
Assignee | ||
Comment 10•23 years ago
|
||
I spend some time looking at the nsNativeComponentLoader and I do not think
there is much to do to make this loader threadsafe.
There are three objects which are of concern. First the nsISupport impl must be
made threadsafe. The second object is the mDLLStore, which is a
nsObjectHashtable class, is already created threadsafe. Lastly, there is
mDeferredComponents, a nsVoidArray. This object is only accessed during
registration, but should be protected anyways.
bringing this baby in. lets see if I can get it pre-approved.
Keywords: mozilla1.0
Target Milestone: mozilla1.1alpha → mozilla1.0
Assignee | ||
Comment 11•23 years ago
|
||
Checking in nsIComponentRegistrar.idl;
/cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v <--
nsIComponentRegistrar.idl
new revision: 1.4; previous revision: 1.3
done
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•23 years ago
|
||
wrong bug. sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•23 years ago
|
||
protects mRefcnt by using the threadsafe macros.
protects mDeferredComponents with mMon a PRMonitor.
Comment 15•23 years ago
|
||
+ PR_DestroyMonitor(mMon);
+ mMon = PR_NewMonitor();
*Always* use nsAutoMonitor::NewMonitor nsAutoMonitor::DestroyMonitor if you are
going to use nsAutoMonitor.
Does this really need to be a monitor rather than a lock? Does it really ever
reenter?
Assignee | ||
Comment 16•23 years ago
|
||
Why? Our examples in nsAutoLock.h show that this is correct. There are many
other places where we create a monitor through nspr and pass it to
nsAutoMonitor. Why is this bad?
I will double check and see if I can use a lock instead. An previous draft of
this patch used to also protect the dllstore hash table. In that case, I really
needed a monitor.
Comment 17•23 years ago
|
||
Then the examples are broken. The deadlock detection code in nsAutoLock.cpp
depends on the monitors and locks being created using that static member. At one
point there was talk of adding a type so that the compiler could help enforce
this rule.
Comment 18•23 years ago
|
||
Bug 58904 asks for strong types for locks and monitors in XPCOM that trivially
wrap the NSPR ones, adding lifetime tracking value for the deadlock detection
code in nsAutoLock.cpp (which has saved several peoples' @$$es in the last few
years, lemmetellya).
dougt, I don't see any example comment in nsAutoLock.h showing a PR_NewMonitor
call, or saying you can use PR_NewMonitor directly. OTOH, the comment I wrote
saying to use nsAutoMonitor::NewMonitor instead is buried in nsAutoLock.cpp,
dagnabit. Some modified for public consumption version of that comment should
go in nsAutoLock.h.
But I don't see why a monitor is needed here, when a lock should suffice. What
control flow must nest, reentering the monitor that's already entered?
/be
Assignee | ||
Comment 19•23 years ago
|
||
after double checking, I can use a lock.
brendan, take a look at nsAutoLock.h line 53. This example is using a lock. I
will move your comment to the right place and get us a new patch. Coming soon.
Assignee | ||
Comment 20•23 years ago
|
||
oh geez, that sucks. it is okay to use PLLock* allocated directly from nspr,
but not a PRMonitor? Not very consistent.
Comment 21•23 years ago
|
||
Yes, monitors and locks are different: there is no nsAutoLock::NewLock static
method, but there is an nsAutoMonitor::NewMonitor -- confusing, as you imply,
and comments will help -- thanks. The right thing are flyweight NSPR-wrapping
XPCOM lock and monitor types with ctors and dtors, but that won't make 1.0.
/be
Assignee | ||
Comment 22•23 years ago
|
||
Attachment #77352 -
Attachment is obsolete: true
Assignee | ||
Comment 23•23 years ago
|
||
Updated•23 years ago
|
Attachment #77398 -
Flags: superreview+
Comment 24•23 years ago
|
||
Comment on attachment 77398 [details] [diff] [review]
some comment to stop others from making my same mistakes
sr=jband comments are good
Comment 25•23 years ago
|
||
dougt: I'm not comfortable with the assertion that mDLLStore is protected by
virtue of being created with the threadsafe flag set. That protection seems
fairly useless to me given that this code also does an enumeration of the
hashtable - a very unprotected activity.
It would help it you layed out which entry points the component manager *might*
call simultaneously. Which are only called on one thread at shutdown? It is
guarenteed that all other calls will be finished when shutdown happens?
I'm curious why this code allows for multiple calls to Init. Does that really
happen? If so is it protected from simultaneous calls?
A map of how the component manager uses the loaders would help - and might allow
you to avoid locking down at this level if there are cases where it is really
not needed.
Is the goal to make this bulletproof in a free threading environment or just
sort of safe?
Assignee | ||
Comment 26•23 years ago
|
||
Luckly unloading is never called :-/ We can get into trouble if two threads
both call Unload(). This is a problem with any threadsafe enumerator which
needs to unlock before calling out, right? Assuming that we do expose Unloading
to clients, it would be a required thing to document that this call should only
be made on the primordial thread, right?
> It would help it you layed out which entry points the component manager
*might* call simultaneously.
I traced though the paths from the component manager to the loader(s) and try to
bucket the calls into sets startup-only, running, shutdown. However, there is a
great amount of overlap. One example is the loader's init method. Looking at
it, you might thing that that would only be called at startup. however,
createInstance could wind up calling it. Since CI is allowed to be called on
multiple thread, it is plausible that init be called simultaneously. If the
loader has not be created and both CI's need that loader, we must have a sync point.
> I'm curious why this code allows for multiple calls to Init. Does that really
happen? If so is it protected from simultaneous calls?
I am not happy with this at all. The component manager never reinits the
loader. It does it once, then caches the loader. It should be removed. I will
attack this with these changes.
> A map of how the component manager uses the loaders would help - and might
allow you to avoid locking down at this level if there are cases where it is
really not needed.
Is there a tool that does this? I started on a pad and paper and it just
started getting messy.
> Is the goal to make this bulletproof in a free threading environment or just
sort of safe?
This is a loaded question. This patch certianly moves us in the direction of a
more bulletproof implementation.
Comment 27•23 years ago
|
||
Comment on attachment 77398 [details] [diff] [review]
some comment to stop others from making my same mistakes
>+ /**
>+ * Constructor
>+ * The constructor locks the given lock. During destruction
>+ * the lock will be unlocked.
Just a thought: "acquires the given lock... the lock will be released" avoids
the "lock the lock" and "unlock the lock" semi-stutter.
Might avoid passive voice in the second sentence, as the first does: "The
destructor releases the lock."
>+ /**
>+ * lock
>+ * Client may call this to reenter the given lock. Take special
>+ * note that locking a locked lock has undefined results.
s/reenter/reacquire/
"Reenter" should be restricted to monitors and reentrancy. Locks are not
"reentrant" as the second sentence notes (but "you'll hang or crash" might be a
better warning than "undefined results" :-).
>+ /**
>+ * unlock
>+ * Client may call this to unlock the given lock. Take special
>+ * note unlocking an unlocked lock has undefined results.
"release the given lock."
Thanks for doing this, r=brendan@mozilla.org with a bit more love for the
comments as proposed above.
/be
Attachment #77398 -
Flags: review+
Comment 28•23 years ago
|
||
Comment on attachment 77393 [details] [diff] [review]
uses a lock to protect mDeferredComponents
r=jband. Doug has convinced me that this is 'good enough'.
Attachment #77393 -
Flags: review+
Comment 29•23 years ago
|
||
> One example is the loader's init method. Looking at
> it, you might thing that that would only be called at startup. however,
> createInstance could wind up calling it.
When I broke loaders out, I fought with myself over this lazy-creation decision.
On the one hand, I didn't want people to pay to spin up Python or Java just
because they had a component registered that might someday be created.
On the other hand, by not doing that initialization at startup when we're
"guaranteed" to be single-theaded, we impose this locking overhead. Though
we can still avoid the locking during the initial startup phase, if we need to.)
Given that the dominant case is that there are only the JS and native component
loaders, maybe we should eagerly create them during single-threaded spinup and
avoid the locks.
Back when I was first trying to wedge PLDHash into the component manager, I
dreamed up a lock-free scheme that took advantage of the fact that changes to
the table (component registrations) are very rare compared to reads from it,
after the initial startup phase, by letting readers race freely, and requiring
writers to update a copy of the table and only swap it in after they'd
synchronized with all threads via a posted XPCOM event (to ensure that they were
no longer in the critical section where they might be holding references to the
original table store). Turns out it's similar to the read-copy-update stuff
that they're starting to use for Linux, so I don't need to write too much about
it. =) (http://lse.sourceforge.net/locking/rclock.html) We can't do this now,
by any means, but I wanted to blurb the bug before I forgot again, in case we
want to revisit this code to reduce locking costs.
I'll look at the patch now: curious to see why we need to lock around
mDeferredComponents, since I thought that was only accessed during the
single-threaded initial registration process. (That's why I didn't lock it in
the first place, maybe a mistake!)
Comment 30•23 years ago
|
||
Comment on attachment 77393 [details] [diff] [review]
uses a lock to protect mDeferredComponents
From above:
> I'm curious why this code allows for multiple calls to Init. Does that really
happen? If so is it protected from simultaneous calls?
I am not happy with this at all. The component manager never reinits the
loader. It does it once, then caches the loader. It should be removed. I
will
attack this with these changes.
-----
I don't see that attack here: we allocate a lock in Init, but don't acquire
it, so it would seem that we could end up racing pretty freely through there.
What's the plan for this? As the patch stands, the only thing the lock
protects is mDeferredComponents, which really seems like it should only be
touched during single-threaded autoreg. Nay?
Assignee | ||
Comment 31•23 years ago
|
||
okay, i didn't remove the reinit code. I will post a new patch will which
remove the check before creating the mLock and mDllStore.
nsNativeComponentLoader::Init() code does not have a race condition as it is
only called during PlatformInit() from the nsComponentManager. This is run only
when XPCOM startups.
WRT the mDeferredComponents lock. I would be happy to remove the lock if we
only supported XPCOM registeration from a single thread. Is that where we want
to be?
Comment 32•23 years ago
|
||
I think we absolutely want to only permit autoregistration on the main thread.
Do we use the deferral mechanism for directed registration, such as XPInstall?
Either way, I'm more than happy requiring people to proxy such registration to
the main thread. Thrilled, even.
Assignee | ||
Comment 33•23 years ago
|
||
I have no problems with that. I will make a comment in the IDL and attach a new
patch. Stay tuned.
Assignee | ||
Comment 34•23 years ago
|
||
thread restriction of AutoReg allows us not protect mDeferredComponents.
Attachment #77393 -
Attachment is obsolete: true
Comment 35•23 years ago
|
||
Comment on attachment 78396 [details] [diff] [review]
patch 3
sr=jband. I'm obviously becoming a push over :)
I'd slap an NS_ASSERTION(!mDllStore,...) in there to catch
unexpected cheaters.
Attachment #78396 -
Flags: superreview+
Assignee | ||
Comment 36•23 years ago
|
||
right. done.
Comment 37•22 years ago
|
||
Comment on attachment 78396 [details] [diff] [review]
patch 3
r=waterson, assuming jband's assertion is there.
Attachment #78396 -
Flags: review+
Assignee | ||
Comment 38•22 years ago
|
||
Checking in nsIComponentRegistrar.idl;
/cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v <--
nsIComponentRegistrar.idl
new revision: 1.5; previous revision: 1.4
done
Checking in nsNativeComponentLoader.cpp;
/cvsroot/mozilla/xpcom/components/nsNativeComponentLoader.cpp,v <--
nsNativeComponentLoader.cpp
new revision: 1.79; previous revision: 1.78
done
Comment 39•22 years ago
|
||
Comment on attachment 77398 [details] [diff] [review]
some comment to stop others from making my same mistakes
a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #77398 -
Flags: approval+
Comment 40•22 years ago
|
||
Comment on attachment 78396 [details] [diff] [review]
patch 3
a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #78396 -
Flags: approval+
Assignee | ||
Comment 41•22 years ago
|
||
landed on branch:
Checking in nsIComponentRegistrar.idl;
/cvsroot/mozilla/xpcom/components/nsIComponentRegistrar.idl,v <--
nsIComponentRegistrar.idl
new revision: 1.4.2.2; previous revision: 1.4.2.1
done
Checking in nsNativeComponentLoader.cpp;
/cvsroot/mozilla/xpcom/components/nsNativeComponentLoader.cpp,v <--
nsNativeComponentLoader.cpp
new revision: 1.78.10.2; previous revision: 1.78.10.1
done
Status: REOPENED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•