73761 - Triggering PROPERTY_CACHE_FILL during gc can cause later crashes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[John Bandhauer]

While testing my xpconnect flattening changes against the full browser I'm
running into crashes in the JS property cache. The object and property in an
entry are non-null garbage. I worked out that the cause of the problem is a call
to js_GetProperty from my gccallback. This loads up an entry in the property
cache that soon becomes garbage.

It looks to me like we should add a 'disabled' flag to JSPropertyCache and set
it when we flush the cache on entry to gc and clear it on the way out of gc.
Acivity in PROPERTY_CACHE_FILL would be skipped if the disabled flag was set.
I'm thinking that short-circuiting PROPERTY_CACHE_TEST would not be worth the
cost, however.

How does this sound?

Also, as I was looking for the cause of my problem I noted that the manipulation
of the 'empty' flag in js_FlushPropertyCacheByProp does not look safe to me. You
could race with another thread and end up setting empty when the cache was not
really empty. This *could* have been the cause of the sort of crash I saw
because js_FlushPropertyCache is a nop when the flag is set. So, there is
potential for leaving dangling pointers in the cache here. I'd recomend removing
the setting of 'empty' from js_FlushPropertyCacheByProp - it can't really be
helping much.

FWIW... I currently need to call js_GetProperty in my gccallback to find and
delete a private data struct that I need to attach to my shared function
objects. Brendan, you'll recall we had an exchange about possible
'multiple-privates' for use by creators of function objects.

I'll attach a patch that works for me.

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Brendan Eich [:brendan]]

D'oh!  I am filled with shame.

This looks good to me.  I was thinking you might have used rt->gcRunning, but
this cache-level flag is better.  js_FlushPropertyCache is not thread-safe in
any case -- it can be called only by the GC, while requests are suspended or
ended.  js_FlushPropertyCacheByProp OTOH must be thread-safe, because of delete.

shaver, can you r=?  jband, I'll sr= if you'll check in.  Or do you want me to
do the deed?

/be

Comment 3

[John Bandhauer]

Attachment: better patch

Comment 4

[John Bandhauer]

Attached a slightly better patch:

- Avoid testing pce_prop twice in js_FlushPropertyCacheByProp.
- Don't call ASSERT_CACHE_IS_EMPTY in js_FlushPropertyCacheByProp - that would 
not be a correct thing to assert without locking.

brendan: I 90% expected a tweak or two from you. I can check this in given r= 
and sr=.

Thanks.

Comment 5

[Brendan Eich [:brendan]]

If I missed that redundant test in ...ByProp, I'm not gonna pick nits in the
second patch.  sr=[e-mail].

/be

Comment 6

[Brendan Eich [:brendan]]

jband: I have a patch that allows a JSClass to reserve up to 255 slots, should I
attach it here?  Or hit me with a new bug.

/be

Comment 7

[John Bandhauer]

brendan: I just filed: http://bugzilla.mozilla.org/show_bug.cgi?id=73843

Letting the JSClass specifiy more slots is fine. But how are you hooking this 
into function objects - where the function object creator does not control the 
function object's JSClass? Are you doing something per object? or setting asside 
x slots for objects of the function class? I suppose you can answer this 
question in the other bug :) Thanks!

Comment 8

[John Bandhauer]

fix checked in.

Comment 9

[Phil Schwartau]

Marking Verified -