PL_ArenaFinish should zero the static variable "PRCallOnceType once".

RESOLVED FIXED in 4.7.2

Status

NSPR
NSPR
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.19 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
PL_ArenaFinish destroys arenaLock, but arenaLock is created
on first use with PR_CallOnce.  So PL_ArenaFinish should also
return the static variable "PRCallOnceType once" to the initial
state so that arenaLock can be re-created if necessary.
(Assignee)

Comment 1

10 years ago
Created attachment 317815 [details] [diff] [review]
Proposed patch

With the proliferation of these static pristineCallOnce objects, I wonder
if we should add a PR_CallOnceTypeInitializer function:

static const PRCallOnceType pristineCallOnce;
const PRCallOnceType *PR_CallOnceTypeInitializer()
{
    return &pristineCallOnce;
}

or just officially declare that it's OK to zero PRCallOnceType variables.
Attachment #317815 - Flags: review?(nelson)
Comment on attachment 317815 [details] [diff] [review]
Proposed patch

This patch is fine.  

You suggested:

static const PRCallOnceType pristineCallOnce;
const PRCallOnceType *PR_CallOnceTypeInitializer()
{
    return &pristineCallOnce;
}

I think a function to facilitate initializing these structs 
is a great idea, but I think I'd prefer 

static const PRCallOnceType pristineCallOnce;
void PR_CallOnceTypeInitializer(PRCallOnceType *zapme)
{
    *zapme = pristineCallOnce;
}

Because this way, if we ever wanted to initialize them with
some other value, that function signature would not need to
change.
Attachment #317815 - Flags: review?(nelson) → review+
(Assignee)

Comment 3

10 years ago
I like the PR_InitializeCallOnceType idea.  We can't
initialize the struct with anything other than zero
because that'll break the current source code that
declare static variables with no initializers:

static PRCallOnceType once;

If it were not for the subtlety that a null pointer is
not guaranteed to be zero-valued in all C implementations,
we could just use memset to reinitialize PRCallOnceType.
(PRCallOnceType doesn't contain any pointer, but it
could be defined to contain a pointer for a new platform.)

I checked in the patch on the NSPR trunk (NSPR 4.7.2).

Checking in plarena.c;
/cvsroot/mozilla/nsprpub/lib/ds/plarena.c,v  <--  plarena.c
new revision: 3.15; previous revision: 3.14
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7.2
In reply to comment 3:
There are MANY places in NSS that use memset to initialize structs containing 
pointers, expecting that doing so will ensure that the pointers are NULL. 
So, I think there's no point in trying to write code that does not make that
assumption, given that we already assume it in so many places. 

There is a data structure size, below which it is faster to initialize the 
block using a structure copy rather than calling memset.  On some systems,
that size is quite large.  That is why I prefer to use structure copies to
memset calls for small structs like this one.  
You need to log in before you can comment on or make changes to this bug.