279636 - js_Interpret JSOP_NAMEDFUNOBJ: parent object not protected from GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dylan Salisbury]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: 

In js_Interpret case JSOP_NAMEDFUNOBJ:

The new parent object (created around jsinterp.c:4446) is not protected from GC.
 The js_CloneFunctionObject call a few lines later can cause GC (by calling
js_NewObject, calling js_GetSlotThreadSafe / ClaimScope, suspending the
request).  parent is used again after that call.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Brendan Eich [:brendan]]

The newborn should protect parent until it is stored in the clone's parent slot,
if you use the patch to js_NewObject from bug 268535 (attachment 171823 [details] [diff] [review]).  Can
you try that patch and report whether it helps here?

/be

Comment 2

[Brendan Eich [:brendan]]

> The newborn should protect parent until it is stored in the clone's parent slot,

"The newborn object root in cx", I meant.

The patch in bug 268535 should fix things for you, I think, but the trunk engine
opens this hole back up again, by GC-allocating the slots vector.  D'oh!

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Simple enough to fix this case.  Any others like it?  Giving jband review duty.


/be

Comment 4

[John Bandhauer]

Brendan, The patch in bug 268535 is a wonderful patch, but it doesn't fix this
case. And you might care why... 

The sequence of js_NewObject -> js_GetSlotThreadSafe -> ClaimScope that is
killing us is kicked off by the OBJ_GET_CLASS of the passed in proto in
js_NewObject. This happens after the (recently relocated) js_AllocGCThing that
fills in a new newborn and before the slots are alloc'd and filled. So, in the
sequence we see, the passed in parent object is collected before it is copied
into a slot. 

It is the OBJ_GET_CLASS below that gets us. And yes, we do have a case where the
 proto's scope's ownercx != cx.

    /*
     * Share proto's map only if it has the same JSObjectOps, and only if
     * proto's class has the same private and reserved slots, as obj's map
     * and class have.
     */
    if (proto &&
        (map = proto->map)->ops == ops &&
        ((clasp->flags ^ OBJ_GET_CLASS(cx, proto)->flags) &
    ...

The patch you added here for jsinterp.c should fix our specific case (once we
take the rest of that patch to the JSOP_NAMEDFUNOBJ case in jsinterp.c. But
perhaps the general problem in js_NewObject will impact other callers?

Thanks!

Comment 5

[Brendan Eich [:brendan]]

jband: that's what I was afraid of, hence this bug's patch and non-DUP status. 
BTW, were you gonna stamp r+ on the patch?

So there may be other problems, but I can't find any, and I do not believe that
js_NewObject should temporarily root its parameters.  Rather, callers should
pass in only well-connected or -rooted objects.  So let's get this fix in and
see whether we can find any other bad (direct or indirect) js_NewObject calls.

/be

Comment 6

[John Bandhauer]

Agreed. I certainly wasn't intending to suggest that passed in params be locally
rooted. I just wanted to make sure this contraint was on your mind and give you
a chance to scan for any errant cases. We're still adjusting to the fact that we
aren't immune to the (so called) last ditch gc cases. I had completely lost
track of tha fact that gc could be let loose by the various paths that can get
into ClaimScope. I'm guessing we'll find some more that you hadn't known about
:) Thanks.

Comment 7

[Brendan Eich [:brendan]]

Fixed.

/be