156354 - Crash on propertyIsEnumerable()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[zack-weg]

propertyIsEnumerable() crashs if the property does not exist.
Build 2002070813, Linux.

Comment 1

[zack-weg]

Attachment: testcase

Comment 2

[Olivier Cahagne]

confirming using build 2002070804 on Win2k (trunk): TB8117842Y.

Comment 3

[Phil Schwartau]

Reassigning to Brendan and adding js1.5 keyword. The following 
line causes the crash in the JS shell:

js> this.propertyIsEnumerable('XYZ');


WINNT STACK TRACE:
obj_propertyIsEnumerable(JSContext * 0x00301d60, JSObject * 0x002fb340, unsigned 
int 1, long * 0x0042106c, long * 0x0012e30c) line 1187 + 3 bytes
js_Invoke(JSContext * 0x00301d60, unsigned int 1, unsigned int 0) line 788 + 23 
bytes
js_Interpret(JSContext * 0x00301d60, long * 0x0012fed8) line 2743 + 15 bytes
js_Execute(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript * 0x00306250, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 968 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript * 
0x00306250, long * 0x0012fed8) line 3274 + 25 bytes
Process(JSContext * 0x00301d60, JSObject * 0x002fb340, char * 0x00000000) line 
371 + 22 bytes
ProcessArgs(JSContext * 0x00301d60, JSObject * 0x002fb340, char * * 0x00300054, 
int 0) line 529 + 17 bytes
main(int 0, char * * 0x00300054) line 2129 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1b9ea()


Note: I looked up Olivier's Talkback incident TB8117842Y,
and found the stack trace there to be pretty similar -

Comment 4

[Brendan Eich [:brendan]]

Attachment: trivial fix

My fault, clumsy fix to old mccabe code that leaked a ref-counted property.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 90577 [details] [diff] [review]
trivial fix

I received tachyon transmissions from 20 minutes from now saying r=khanson,
sr=shaver.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed in the trunk.  Going for drivers' approval for the branch.

/be

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 90577 [details] [diff] [review]
trivial fix

Please land this on MOZILLA_1_0_BRANCH.  Once there, replace the
"mozilla1.0.1+" keyword with the "fixed1.0.1" keyword.

Comment 8

[Brendan Eich [:brendan]]

Fixed on branch, too.

/be

Comment 9

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-156354.js

Comment 10

[Phil Schwartau]

Reopening bug -

I do not crash on the given HTML testcase, but I get the same
crash as in Comment #3 if I do this in the JS shell:

js> this.propertyIsEnumerable('XYZ');


WINNT STACK TRACE:
obj_propertyIsEnumerable(JSContext * 0x00301d60, JSObject * 0x002fb340, unsigned
int 1, long * 0x0042106c, long * 0x0012e308) line 1196 + 23 bytes
js_Invoke(JSContext * 0x00301d60, unsigned int 1, unsigned int 0) line 839 + 23
bytes
js_Interpret(JSContext * 0x00301d60, long * 0x0012fed8) line 2803 + 15 bytes
js_Execute(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript * 0x003051a0,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 1020 + 13 bytes
JS_ExecuteScript(JSContext * 0x00301d60, JSObject * 0x002fb340, JSScript *
0x003051a0, long * 0x0012fed8) line 3277 + 25 bytes
Process(JSContext * 0x00301d60, JSObject * 0x002fb340, char * 0x00000000) line
369 + 22 bytes
ProcessArgs(JSContext * 0x00301d60, JSObject * 0x002fb340, char * * 0x00300054,
int 0) line 508 + 17 bytes
main(int 0, char * * 0x00300054) line 2108 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1b9ea()


Similarly, the newly-added JS shell testcase is crashing. 
Here is the crashpoint in the top frame of the stack trace:

   if (prop &&
        obj2 != obj &&
        !(OBJ_IS_NATIVE(obj2) &&
          SPROP_IS_SHARED_PERMANENT((JSScopeProperty *)prop))) {
        OBJ_DROP_PROPERTY(cx, obj2, prop);
        *rval = JSVAL_FALSE;
        return JS_TRUE;
    }

    ok = OBJ_GET_ATTRIBUTES(cx, obj2, id, prop, &attrs);  <<<----- CRASHES HERE

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix

Brainfart, sorry.  This is a diff -wu, BTW.

/be

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 99439 [details] [diff] [review]
proposed fix

r=shaver.

Comment 13

[Brendan Eich [:brendan]]

Fixed, for good.

/be

Comment 14

[Phil Schwartau]

Verified FIXED.

The new testcase passes, and the JS testsuite passes. 
I checked this in the debug/optimized JS shell on WinNT -

Comment 15

[Phil Schwartau]

I should add, this is fixed on the trunk only, not on MOZILLA_1_0_BRANCH -

Comment 16

[Brendan Eich [:brendan]]

No bugs since mid-July (IIRC) have been fixed on the 1.0 branch.  I proposed in
some number of bugs that we land the 1.5 engine from the trunk onto the branch
after doing another Release Candidate.

/be