875 - Bogus "property is not defined" errors

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[rainer]

The lookup code for the JavaScript property cache is wrong. False cache hits are possible when a property is cached as PROP_NOT_FOUND. To the user, the bug manifests itself as bogus "property is not defined" errors, in perfectly valid JavaScript programs.



This is the buggy code, file jsinterp.h:



#define PROPERTY_CACHE_TEST(cache, obj, id, prop)                        \

    PR_BEGIN_MACRO                                                       \

        uintN _hashIndex = (uintN)PROPERTY_CACHE_HASH(obj, id);          \

        JSPropertyCacheEntry *_pce = &(cache)->table[_hashIndex];        \

        (cache)->tests++;                                                \

        if (_pce->property &&                                            \

            (_pce->property == PROP_NOT_FOUND ||

			/* *** BUG! obj has to be compared every time! *** */

             _pce->property->object == obj) &&                           \

            _pce->symbolid == id) {                                      \

            prop = _pce->property;                                       \

        } else {                                                         \

            (cache)->misses++;                                           \

            prop = NULL;                                                 \

        }                                                                \

    PR_END_MACRO



The simplest fix is putting an extra field containing the object into the JSPropertyCacheEntry structure. That's what I did and it works. Here is the fixed section of jsinterp.h:



typedef struct JSPropertyCacheEntry {

    JSObject    *obj;           /* weak link to object */

    JSProperty  *property;      /* weak link to property */

    jsval       symbolid;       /* strong link to name atom, or index jsval */

} JSPropertyCacheEntry;



typedef struct JSPropertyCache {

    JSPropertyCacheEntry table[PROPERTY_CACHE_SIZE];

    JSBool               empty;

    uint32               fills;

    uint32               recycles;

    uint32               tests;

    uint32               misses;

    uint32               flushes;

    uint32               pflushes;

} JSPropertyCache;



#define PROP_NOT_FOUND   ((JSProperty *)1)

#define PROP_FOUND(prop) ((prword)(prop) & (prword)-2)



#define PROPERTY_CACHE_FILL(cx, cache, obj, id, prop)                         \

    PR_BEGIN_MACRO                                                            \

        uintN _hashIndex = (uintN)PROPERTY_CACHE_HASH(obj, id);               \

        JSPropertyCacheEntry *_pce = &(cache)->table[_hashIndex];             \

        if (_pce->property) {                                                 \

            (cache)->recycles++;                                              \

            if (!JSVAL_IS_INT(_pce->symbolid))                                \

                js_DropAtom(cx, (JSAtom *)_pce->symbolid);                    \

        }                                                                     \

        _pce->obj = obj;                                                      \

        _pce->property = prop;                                                \

        _pce->symbolid = id;                                                  \

        if (!JSVAL_IS_INT(id))                                                \

            js_HoldAtom(cx, (JSAtom *)id);                                    \

        (cache)->empty = JS_FALSE;                                            \

        (cache)->fills++;                                                     \

    PR_END_MACRO



#define PROPERTY_CACHE_TEST(cache, obj, id, prop)                             \

    PR_BEGIN_MACRO                                                            \

        uintN _hashIndex = (uintN)PROPERTY_CACHE_HASH(obj, id);               \

        JSPropertyCacheEntry *_pce = &(cache)->table[_hashIndex];             \

        (cache)->tests++;                                                     \

        if (_pce->obj == obj && _pce->symbolid == id) {                       \

            prop = _pce->property;                                            \

        } else {                                                              \

            (cache)->misses++;                                                \

            prop = NULL;                                                      \

        }                                                                     \

    PR_END_MACRO

Comment 1

[Mike McCabe]

Thought I closed this one...

It turns out that this bug was against 3/31 code, and the bug has since been
fixed.

It's about as specific a bug as I've ever recieved from the net - I wish more
were like this.

Comment 2

[rubydoo123]

bug against 3/31 -- marking as verified

Comment 3

[leger]

Changing component to "Javascript Engine".  "Javascript" component is being
retired.