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

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Régis Caspar]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Regis, do you have a debugger available?  If so, could you paste in the stack to that assert?

Comment 2

[Régis Caspar]

Attachment: call stack

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

What extensions do you have installed?

Comment 4

[Régis Caspar]

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

Comment 5

[Régis Caspar]

Attachment: stdout+stderr messages

stdout and stderr output (gzipped) if it can help

Comment 6

[Régis Caspar]

Attachment: stdouts+stderr messages (2)

Corrected problem with mime type

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Make it thread-safe

Comment 12

[Darin Fisher]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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

[Darin Fisher]

> the UUIDGenerator is, generally speaking, a service....

Right, of course.  No worries then.  I think you should fix the comment accordingly.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Will do.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 220349 [details] [diff] [review]
Make it thread-safe

+    virtual ~nsUUIDGenerator();

does this have any subclasses?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

There's really not much I can do about the OS being generally not threadsafe.... :(

Comment 19

[Darin Fisher]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Sounds reasonable.

Comment 23

[Régis Caspar]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Régis Caspar]

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

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 220349 [details] [diff] [review]
Make it thread-safe

Sorry, thought I already got to this earlier. r=me

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Looks like bug 279521 got landed on the branch, so we need this there too.

Comment 29

[Mike Schroepfer]

Comment on attachment 220349 [details] [diff] [review]
Make it thread-safe

a=schrep for drivers for

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.