54307 - jsdtoa.c - InitDtoa does not destroy locks

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jon Smirl]

These two locks are created but never destroyed.

#ifdef JS_THREADSAFE
static JSBool initialized = JS_FALSE;

/* hacked replica of nspr _PR_InitDtoa */
static void InitDtoa(void)
{
    freelist_lock = PR_NewLock();
        p5s_lock = PR_NewLock();
    initialized = JS_TRUE;
}
#endif

Following patch adds static function TermDtoa() to jsdtoa.c. js_CleanupLocks is 
modified to call TermDtoa and cleanup jsdtoa.

Comment 1

[Jon Smirl]

Attachment: Adds term function to destroy locks

Comment 2

[Jon Smirl]

patch is not quite right. It needs to test to make sure the static have been 
initialized.

void TermDtoa(void)
{
    if (initialized == JS_TRUE)
    {
        PR_DestroyLock(freelist_lock);
        PR_DestroyLock(p5s_lock);
    }
    initialized = JS_FALSE;
}

Comment 3

[Phil Schwartau]

cc'ing Brendan and mccabe

Comment 4

[Brendan Eich [:brendan]]

The patch looks ok, but could you please prefix TermDtoa with js_ to avoid 
polluting the global C/C++ namespace?  Thanks.

/be

Comment 5

[Jon Smirl]

Attachment: adds js_ prefix and test for initialization

Comment 6

[Brendan Eich [:brendan]]

Jon, the extern js_TermDtoa(void); prototype is missing a void return type after 
the extern keyword.  Fix that, an a=[e-mail] (mccabe or rogerl can r= 
and check into the trunk).

/be

Comment 7

[Mike McCabe]

Maybe it's just me, but js_TermDtoa seems like an ambiguous name. 
js_FinishDtoa?  Also, I wonder if it makes sense to put the extern declaration
in jsdtoa.h.  It's part of the API requirement now...

The PR_DestroyLock calls in js_TermDtoa should be within a #ifdef JS_THREADSAFE,
as they refer to locks that are only defined for JS_THREADSAFE builds.

I'd leave the rest of js_TermDtoa outside of JS_THREADSAFE, (even though it
doesn't do much) so that the API stays the same, threadsafe or no.

Comment 8

[Brendan Eich [:brendan]]

mccabe: good point about js_Finish vs. js_Term -- I promulgated the Init : 
Finish :: New : Destroy long ago in SpiderMonkey's naming conventions, how could 
I forget?  I think I dimly recalled a Term vs. Init antonym and thought maybe 
that meme was in jsdtoa.c (code not original to SpiderMonkey), but I should've 
checked.

Also, jsdtoa.h hosting the extern sounds fine.  Sometimes, with one-off externs 
it pays to put the extern in the "importing" .c file and avoid over-including 
the .h file.  If jslock.c doesn't really need anything else from jsdtoa.h (as I 
believe is the case), then I'm ok with a "one-off extern in the importer" here.

/be

Comment 9

[Jon Smirl]

Attachment: add void, rename to finish

Comment 10

[Brendan Eich [:brendan]]

Ok, I can a=[e-mail] that last patch, without conditions.  Thanks 
Jon.  mccabe, you ok with the one-off extern?

/be

Comment 11

[Jon Smirl]

Attachment: also move threadsafe ifdef

Comment 12

[Jon Smirl]

Attachment: try again with the right file

Comment 13

[Jon Smirl]

Attachment: and add the extern void

Comment 14

[Mike McCabe]

Fix checked in.

Comment 15

[Phil Schwartau]

Marking Verified -