307279 - [FIX]Classinfo doesn't protect newly-wrapped stuff from GC

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

nsDOMClassInfo::WrapNative just returns the jsval, not the
nsIXPConnectJSObjectHolder.  This means that if the caller happens to GC the
jsval could die.  And JS_DefineUCProperty can GC (if it atomizes the prop name).

So I think that nsDOMClassInfo::WrapNative should just return the
nsIXPConnectJSObjectHolder too.  Or we could make it an optional arg and callers
who know they don't need it (getter hooks) can pass nsnull for the
nsIXPConnectJSObjectHolder** if that would be better... but I think having it
explicitly there reduces the chance of mistakes

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

The nsWindowSH::NewResolve change is not strictly for this bug, but it's just a
silly (regression) mistake with a code line ending up as part of a comment when
it shouldn't be.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 195077 [details] [diff] [review]
Patch

r=jst

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 195077 [details] [diff] [review]
Patch

Up to jst on whether to have a default null for aHolder, but I would -- matter
of taste, mostly, but it also reduces call side code bloat, from the look of
things.  I *trust* the few top hackers operating here to remember when to pass
a holder!

(Don't break my heard! ;-)

Approving for 1.8b4 -- this is a safe fix, and we want that *objp = obj; line
that was badly joined into the preceding comment line!

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

Fixed trunk and branch

Comment 5

[Scott MacGregor]

did this break the mac build on the trunk?

Normally I wouldn't care so much but it just landed on the branch too and I'm
busy trying to fix the Mac bustage due to enabling official branding on the
release machines and that will be made more difficult if the mac build starts
breaking here :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

I forgot to check in the .h part of the patch.  It's in now.