NS_NAMED_LITERAL_STRING depends on static global initialization order

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: bryner, Assigned: dougt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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.
We shouldn't have global |NS_NAMED_LITERAL_STRING|s.
(Reporter)

Comment 2

17 years ago
If that's true, someone needs to go clean up extensions/xmlextras/soap/src.
(Assignee)

Comment 3

17 years ago
file a new bug. ;-)
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID
(Reporter)

Comment 4

17 years ago
reopening... talked to dbaron and he says that we can fix this particular
problem fairly easy by allocating gExitRoutines at run time.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 6

17 years ago
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.
Attachment #86141 - Flags: needs-work+
Rayw, see comment #2.

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 8

17 years ago
brendan see comment #4.  
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
(Assignee)

Comment 10

17 years ago
Created attachment 86161 [details] [diff] [review]
something like this
Attachment #86141 - Attachment is obsolete: true
(Assignee)

Comment 11

17 years ago
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 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
Attachment #86161 - Flags: needs-work+
(Assignee)

Comment 13

17 years ago
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
(Assignee)

Comment 14

17 years ago
Created attachment 86163 [details] [diff] [review]
patch v.3
Attachment #86161 - Attachment is obsolete: true
Er, PR_CallOnce.

/be
(Reporter)

Comment 16

17 years ago
Created attachment 86165 [details] [diff] [review]
patch v.4

This avoids the function call overhead, since we now only need to allocate
gExitRoutines in one place.
Attachment #86163 - Attachment is obsolete: true
(Assignee)

Comment 17

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

r=dougt
Attachment #86165 - Flags: review+
(Assignee)

Comment 18

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

Updated

17 years ago
Attachment #86165 - Flags: superreview+
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
(Reporter)

Comment 20

17 years ago
doug, i'll let you go ahead and check this in.
(Assignee)

Comment 21

17 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
Argh -- what about a bug for the static NS_NAMED_LITERAL_STRINGS in soap code?

/be

Comment 24

15 years ago
another solution to this bug would have been to make the string code use
malloc/free directly :-/
You need to log in before you can comment on or make changes to this bug.