198851 - JSD leaks jsdcontext on shutdown

Status: RESOLVED INCOMPLETE

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[timeless]

I know there's a comment indicating that jsd is concerned about its code being
left alive when it shuts down, but in xpcshell i don't have that problem and the
actual leak introduces noise which i don't need into valgrind output.

Comment 1

[timeless]

Attachment: introduce lock release apis and conditionally (timeless) call them

Comment 2

[timeless]

Comment on attachment 118209 [details] [diff] [review]
introduce lock release apis and conditionally (timeless) call them

I'm willing to wrap the release code in whatever magic you want, and i'll
follow whatever style you like.

Comment 3

[Brendan Eich [:brendan]]

Why can't jsd clean up its resources without #ifdef DEBUG_timeless etc.?

/be

Comment 4

[Boying Lu]

I found the same leak on solaris sparc platform. I have two questions:
1. why user #ifdef ... #endif?
2. I think there is no need to put these operations in sparatered functions,
since  onone else will call it.

Comment 5

[Boying Lu]

I think we can put all these free operations in one function maybe called
jsd_DestroyLock(JSDStaticLock*) and move 
jsdc->scriptsLock = NULL;
jsdc->sourceTextLock = NULL;
jsdc->atomsLock = NULL;
jsdc->objectsLock = NULL;
jsdc->threadStatesLock = NULL;
into it

Comment 6

[timeless]

i was slightly afraid about unconditionally cleaning the resources, see the 
comment about intentionally leaking, and hoped that it'd have been easier for 
me to get it committed ifdefed. i'm certainly not tied to the ifdef.

[e-mail]: your suggestion doesn't work at all, the locks are void*, so 
the function would have to take a void** and do evil casting which is not a 
good idea.

i've been using this patch for over a year without issue (unlike some things 
in jsd, such as the unsafe lock asserts).

Comment 7

[Boying Lu]

Attachment: another proposal

Maybe I didn't say clear. So I made this patch. I have tested this patch it
works.

Comment 8

[Boying Lu]

Comment on attachment 162158 [details] [diff] [review]
another proposal

Sorry I found a bug in my  patch

Comment 9

[Boying Lu]

Attachment: a new proposal

Comment 10

[timeless]

Comment on attachment 162163 [details] [diff] [review]
a new proposal

>+static void jsd_ReleaseLock(JSDContext* jsdc)

This thing isn't exported, so it probably shouldn't have jsd_ prefixed.
Second it doesn't take a Lock as a parameter, if you were going to have a
function that did what this one does, then it would probably be called
ReleaseLocks.
Lastly, why bother? how is this code any better than my patch?
you have a block of code which is only ever going to be used in a single place,
which is long and ugly.
Oh, and you're also passing a void* to a function which takes a JSDStaticLock*
which is not very nice.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 118209 [details] [diff] [review]
introduce lock release apis and conditionally (timeless) call them

Over three years without r+ so I'm removing the sr? to me and the r?rginda -- try another would-be owner.

/be

Comment 12

[BMO Automation]

Component is obsolete so resolving bugs as INCOMPLETE