User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060417 Firefox/3.0a1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060417 Firefox/3.0a1 see #334616 comment 19 and comment 20. Reproducible: Always Steps to Reproduce: 1. start firefox (debug build) Actual Results: Assertion nsUUIDGenerator not thread-safe: '_mOwningThread.GetThread() Expected Results: no assertion
Regis, do you have a debugger available? If so, could you paste in the stack to that assert?
12 years ago
What extensions do you have installed?
(In reply to comment #3) > What extensions do you have installed? Adblock 0.7 Session Saver 0.4 Nightly tester Tool 1.0
Created attachment 219411 [details] stdout+stderr messages stdout and stderr output (gzipped) if it can help
Created attachment 219412 [details] stdouts+stderr messages (2) Corrected problem with mime type
Attachment #219411 - Attachment is obsolete: true
Hmm... Getting the safe JSContext from off the main thread is a reasonable thing to be doing. Vlad, what would it take to make the uuid generator threadsafe? If anything? The only state it has is mInitialized/mRBytes/mState, which are all set in Init() and never set again by nsUUIDGenerator itself. It doesn't use any statics. So as long as Init() is not reentered on multiple threads at once and as long as random() and the other OS calls we make are threadsafe, we should be ok, right?
I believe so yeah, it should be threadsafe; I'd still like to get away from using random() there anyway, but I don't know of any alternatives.
Actually, random() might not be thread-safe -- see http://gcc.gnu.org/ml/fortran/2004-04/msg00173.html for some discussion.... I suppose we could lock and unlock in UUID creation (and Init(), presumably)... I doubt that would add much overhead to actually creating the UUID, right?
I suspect that the locking overhead is minor compared to the other operations performed by UUID generation. We also have to be concerned about threadsafety of the Mac OS X API call.
Created attachment 220349 [details] [diff] [review] Make it thread-safe
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Summary: ASSERTION: nsUUIDGenerator not thread-safe: '_mOwningThread.GetThread() → [FIX]ASSERTION: nsUUIDGenerator not thread-safe: '_mOwningThread.GetThread()
Comment on attachment 220349 [details] [diff] [review] Make it thread-safe >Index: xpcom/base/nsUUIDGenerator.h > public: > nsUUIDGenerator(); >+ virtual ~nsUUIDGenerator(); Can't this be a private non-virtual dtor? >Index: xpcom/base/nsUUIDGenerator.cpp > If we can't >+ // depend on reasonable locking around Init we're screwed with the >+ // assign-and-check above, anyway. I think we should verify that this is okay. What happens if two threads call CreateInstance on this guy's class factory at the same time? (If this were a singleton managed by the service manager then you would not need to worry.)
> Can't this be a private non-virtual dtor? Sure. I'll make that change. > What happens if two threads call CreateInstance on this guy's class factory at > the same time? They get different instances, no? And yes, the UUIDGenerator is, generally speaking, a service....
> the UUIDGenerator is, generally speaking, a service.... Right, of course. No worries then. I think you should fix the comment accordingly.
Comment on attachment 220349 [details] [diff] [review] Make it thread-safe + virtual ~nsUUIDGenerator(); does this have any subclasses?
(In reply to comment #16) > (From update of attachment 220349 [details] [diff] [review] ) > + virtual ~nsUUIDGenerator(); > > does this have any subclasses? ah, darin commented on that already, nevermind :) I want to note that this lock doesn't necessarily make the code threadsafe, because the system call could attempt to use shared resources that are also used by other syscalls.
There's really not much I can do about the OS being generally not threadsafe.... :(
Hmm... we'll crash on some systems if people call gethostbyname. They have to call PR_GetHostByName instead so that all calls get serialized. Hopefully, we don't have to go there for this.
Well, in this case I'm basically serializing calls to the OS, unless code elsewhere calls CFUUIDCreate, CoCreateGuid, or random and company. CFUUIDCreate is only called by PDECore.c (whatever that is). No one calls CoCreateGuid There are various callers to random()... we really do need an NSPR random number generator (which would then be threadsafe etc).
So, those functions may all be called from extensions. I don't think it needs to block this bug, but I think we should have a follow-up bug to investigate whether or not those APIs are threadsafe and if they are not threadsafe, then we should provide threadsafe alternatives for extensions and us to use.
(In reply to comment #22) > Sounds reasonable. Boris's patch fix the issue here, no more assertions for nsUUIDGenerator however, I have two new similar ones: ###!!! ASSERTION: nsGlobalWindow not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla/mozilla/dom/src/base/nsGlobalWindow.cpp, line 582 ###!!! ASSERTION: PrincipalHolder not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file c:/mozilla/mozilla/js/src/xpconnect/src/xpccomponents.cpp, line 2070 (Custom Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060511 Firefox/3.0a1 ID:2006051101 [cairo] debug build)
Please file separate bugs. And check whether some particular extension causes them? Note that nsGlobalWindow is NOT threadsafe. So if some extension accesses it from off-thread it really needs to be fixed.
(In reply to comment #24) > Please file separate bugs. And check whether some particular extension causes > them? Done. See bug 337609 (nsGlobalWindow) and bug 337611 (PrincipalHolder). They seems both due to Google Toolbar 2.0.20060404. (NB: I'm not sure I did set the right Product/Component for the for bug 337609) Thanks Darin.
Comment on attachment 220349 [details] [diff] [review] Make it thread-safe Sorry, thought I already got to this earlier. r=me
Attachment #220349 - Flags: review?(vladimir) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Looks like bug 279521 got landed on the branch, so we need this there too.
Flags: blocking1.9a1? → blocking1.8.1?
Attachment #220349 - Flags: approval1.8.1?
Comment on attachment 220349 [details] [diff] [review] Make it thread-safe a=schrep for drivers for
Attachment #220349 - Flags: approval1.8.1? → approval1.8.1+
Fixed on 1.8 branch.
You need to log in before you can comment on or make changes to this bug.