148884 - NS_NAMED_LITERAL_STRING depends on static global initialization order

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brian Ryner (not reading)]

If you have a global NS_NAMED_LITERAL_STRING, the app may crash at startup
depending on the order that global intializers are run in.  In particular, when
nsString allocates its storage via nsMemory::Alloc, that may call
SetupGlobalMemory which then calls NS_RegisterXPCOMExitRoutine, which depends on
the static nsVoidArray gExitRoutines having been initialized.

This impacts the static build; it doesn't hit the non-static build because we
don't use static strings in any libraries that mozilla-bin directly links with.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We shouldn't have global |NS_NAMED_LITERAL_STRING|s.

Comment 2

[Brian Ryner (not reading)]

If that's true, someone needs to go clean up extensions/xmlextras/soap/src.

Comment 3

[Doug Turner (:dougt)]

file a new bug. ;-)

Comment 4

[Brian Ryner (not reading)]

reopening... talked to dbaron and he says that we can fix this particular
problem fairly easy by allocating gExitRoutines at run time.

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 86141 [details] [diff] [review]
patch

add an assertion if gExitRoutines is null after the allocation.  

Also lets bullet proof this:  Any time we access this array, check for null.

Comment 7

[Brendan Eich [:brendan]]

Rayw, see comment #2.

/be

Comment 8

[Doug Turner (:dougt)]

brendan see comment #4.  

Comment 9

[Brendan Eich [:brendan]]

dougt, saw that -- but are static ctors and dtors ok now?  See
http://www.mozilla.org/hacking/portable-cpp.html#dont_use_static_constructors.

/be

Comment 10

[Doug Turner (:dougt)]

Attachment: something like this

Comment 11

[Doug Turner (:dougt)]

brendan: static ctors and dtors are still not okay.  As the document points out
they are problematic.  I think that this is a case and point.

We should make xpcom startup a bit more friendly by remove its own static
nsVoidArray. 

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 86161 [details] [diff] [review]
something like this

Use PR_Once for thread-safe at-most-once initialization.

Why call EnsureExitRoutines from CallExitRoutines?  Better to return early if
(!gExitRoutines), no?

/be

Comment 13

[Doug Turner (:dougt)]

yeah.  CallExitRoutines should just check gExitRoutines.

Now what is this business with PR_Once?
http://lxr.mozilla.org/seamonkey/search?string=PR_Once yields nothing

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 15

[Brendan Eich [:brendan]]

Er, PR_CallOnce.

/be

Comment 16

[Brian Ryner (not reading)]

Attachment: patch v.4

This avoids the function call overhead, since we now only need to allocate
gExitRoutines in one place.

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 86165 [details] [diff] [review]
patch v.4

r=dougt

Comment 18

[Doug Turner (:dougt)]

brendan, we don't care about threadsafety here.  This "exit" methods are private
between the xpcom library and the xpcom glue library.  They will always occur on
the primordal thread.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 86165 [details] [diff] [review]
patch v.4

That's the ticket.

dougt, a comment in nsXPCOMPrivate.h about the single-threaded restriction on
exit routines would nail it.  Any time, rs=me.

/be

Comment 20

[Brian Ryner (not reading)]

doug, i'll let you go ahead and check this in.

Comment 21

[Doug Turner (:dougt)]

Checking in nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.136; previous revision: 1.135
done
Checking in nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
new revision: 1.2; previous revision: 1.1
done

thanks all.

Comment 22

[Brendan Eich [:brendan]]

Argh -- what about a bug for the static NS_NAMED_LITERAL_STRINGS in soap code?

/be

Comment 23

[Doug Turner (:dougt)]

http://bugzilla.mozilla.org/show_bug.cgi?id=148888

Comment 24

[Darin Fisher]

another solution to this bug would have been to make the string code use
malloc/free directly :-/