[FIX]ASSERTION: nsUUIDGenerator not thread-safe: '_mOwningThread.GetThread()

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Régis Caspar, Assigned: bz)

Tracking

({fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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?
(Reporter)

Comment 2

12 years ago
Created attachment 219339 [details]
call stack
What extensions do you have installed?
(Reporter)

Comment 4

12 years ago
(In reply to comment #3)
> What extensions do you have installed?
Adblock 0.7
Session Saver 0.4
Nightly tester Tool 1.0
(Reporter)

Comment 5

12 years ago
Created attachment 219411 [details]
stdout+stderr messages

stdout and stderr output (gzipped) if it can help
(Reporter)

Comment 6

12 years ago
Created attachment 219412 [details]
stdouts+stderr messages (2)

Corrected problem with mime type
Attachment #219411 - Attachment is obsolete: true

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: 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?
Flags: blocking1.9a1?
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?

Comment 10

12 years ago
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
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #220349 - Flags: superreview?(darin)
Attachment #220349 - Flags: review?(vladimir)
Blocks: 326506
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 12

12 years ago
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....

Comment 14

12 years ago
> 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] [edit])
> +    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.... :(

Comment 19

12 years ago
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).

Comment 21

12 years ago
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.
Sounds reasonable.

Updated

12 years ago
Attachment #220349 - Flags: superreview?(darin) → superreview+
(Reporter)

Comment 23

12 years ago
(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.
(Reporter)

Comment 25

12 years ago
(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+
Fixed.
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.
Blocks: 279521
Flags: blocking1.9a1? → blocking1.8.1?
Attachment #220349 - Flags: approval1.8.1?

Comment 29

12 years ago
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.
Keywords: fixed1.8.1
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.