Closed Bug 872 Opened 26 years ago Closed 26 years ago

Bogus "property is not defined" errors

Categories

(Core :: JavaScript Engine, defect, P1)

All
Other
defect

Tracking

()

VERIFIED INVALID

People

(Reporter: rainer, Assigned: brendan)

Details

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
Assignee: mccabe → brendan
Status: NEW → RESOLVED
Closed: 26 years ago
Resolution: --- → INVALID
Turns out that this was based on the code from the original March drop.
Status: RESOLVED → VERIFIED
Changing component to "Javascript Engine". "Javascript" component is being retired.
You need to log in before you can comment on or make changes to this bug.