131275 - JS_AddRoot could provide a default root name if you define NAME_ALL_GC_ROOTS

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Ginda]

If JS_AddRoot were a #define to JS_AddNamedRoot in debug builds, leaked roots
could be easier to track.  I'm not sure how to turn __LINE__ into a string in
preprocessor speak, so the upcoming patch only uses __FILE__ as the name.

What do you all think?

Comment 1

[Robert Ginda]

Attachment: proposed patch

Comment 2

[timeless]

Attachment: like 74404 but with line number

http://www.jaggersoft.com/pubs/CVu10_1.html by [e-mail] describes how
to convert __LINE__ into a string.

Comment 3

[Robert Ginda]

I'd like to hear from brendan or some else closer to the engine than I am before
I r= anything.

Comment 4

[Brendan Eich [:brendan]]

I don't think we should change the ABI (exported symbols) even for DEBUG
versions of js3250.dll, e.g. (DSO, whatever).  So I'd prefer a different test
than DEBUG.  How about JS_NAME_ALL_ROOTS or some such?

rginda, did this come up in a real-world situation?  Who was using JS_AddRoot
and not JS_AddNamedRoot?

/be

Comment 5

[Robert Ginda]

Yes, this came from a real world situation, but it was a while ago so I don't
recall the specifics.  I may have been trying to get more information in the
GC_MARK_DEBUG output.

Comment 6

[timeless]

not that my say counts for anything, but that define seems fine with me, if
people like it then I can try to write up that patch...

Comment 7

[Brendan Eich [:brendan]]

If you test GC_MARK_DEBUG with that #ifdef, embedders will have to recompile the
world when enabling GC_MARK_DEBUG.  That didn't used to be the case.  I'd say
make the test

#if defined GC_MARK_DEBUG && defined NAME_ALL_GC_ROOTS

/be

Comment 8

[Robert Ginda]

I happened to use this in conjunction with GC_MARK_DEBUG, but I could imagine it
being useful in other situations.  How about just NAME_ALL_GC_ROOTS?

Comment 9

[Brendan Eich [:brendan]]

Ok, NAME_ALL_GC_ROOTS it is.  Timeless, can you repatch?

/be

Comment 10

[timeless]

Attachment: like 113687 but dependent on NAME_ALL_GC_ROOTS

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 115400 [details] [diff] [review]
like 113687 but dependent on NAME_ALL_GC_ROOTS

Sure, why not?

/be

Comment 12

[timeless]

.

Comment 13

[timeless]

checked in

Comment 14

[Phil Schwartau]

Checkin verified -