520164 - Crash [@ JS_Enumerate] or "Assertion failure: obj->map->ops->defineProperty == js_DefineProperty" with eval

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

eval("var a;", <x/>)

Assertion failure: obj->map->ops->defineProperty == js_DefineProperty, at ../jsops.cpp:2938

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to TM changeset eec5815b761f or bug 519129:

The first bad revision is:
changeset:   33294:eec5815b761f
user:        Blake Kaplan
date:        Tue Sep 29 10:03:42 2009 -0700
summary:     Fix Zimbra indirect eval crash. r=brendan

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

eval("var a;", [])

asserts too. E4X isn't needed.

Tested on http://hg.mozilla.org/tracemonkey/rev/07e524d2a503

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

$ cat w6664-cmpin.js 
try { (function() { "" > x })() } catch(e) {}
gc()
try {
    eval("\
         for (var a = 0; a < 1; a++) {\
    for (var b = 0; b < 1; b++) 0\
    }", x = [])
} catch(e) {}
uneval(this)
$ ./js-opt-tm-darwin w6664-cmpin.js 
Segmentation fault

(sometimes Illegal Instruction, exit code 132?)

crashes js opt shell on TM tip without -j at JS_Enumerate near null and asserts js debug shell without -j at Assertion failure: obj->map->ops->defineProperty == js_DefineProperty, at ../jsops.cpp:2934


Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000028
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-darwin              	0x00011405 JS_Enumerate + 69
1   js-opt-tm-darwin              	0x00067b9a __ZL16MarkSharpObjectsP9JSContextP8JSObjectPP9JSIdArray + 250
2   js-opt-tm-darwin              	0x00067d09 __ZL16MarkSharpObjectsP9JSContextP8JSObjectPP9JSIdArray + 617
3   js-opt-tm-darwin              	0x00067d09 __ZL16MarkSharpObjectsP9JSContextP8JSObjectPP9JSIdArray + 617
4   js-opt-tm-darwin              	0x00067ef7 js_EnterSharpObject + 311
5   js-opt-tm-darwin              	0x0006946c __ZL12obj_toSourceP9JSContextjPl + 220
6   js-opt-tm-darwin              	0x0005e986 js_Invoke + 1606
7   js-opt-tm-darwin              	0x0005f0fb js_InternalInvoke + 139
8   js-opt-tm-darwin              	0x0006e46d js_TryMethod + 285
9   js-opt-tm-darwin              	0x000c848c js_ValueToSource + 380
10  js-opt-tm-darwin              	0x000c84e8 __ZL10str_unevalP9JSContextjPl + 40
11  js-opt-tm-darwin              	0x00057e80 js_Interpret + 39904
12  js-opt-tm-darwin              	0x0005e03a js_Execute + 362
13  js-opt-tm-darwin              	0x0000d65c JS_ExecuteScript + 60
14  js-opt-tm-darwin              	0x000041e8 __ZL7ProcessP9JSContextP8JSObjectPci + 1336
15  js-opt-tm-darwin              	0x000082a4 main + 2212
16  js-opt-tm-darwin              	0x0000217b _start + 209
17  js-opt-tm-darwin              	0x000020a9 start + 41

Comment 4

[Jesse Ruderman]

jsfunfuzz hits this frequently, even on ARM.

Comment 5

[Robert Sayre]

mrbkap, you catch this with other patches?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

ML objects are so special they make me want to scream... If this is an indirect call, then the scope object is necessarily already a global object.

Comment 7

[Igor Bukanov]

Comment on attachment 409071 [details] [diff] [review]
Proposed fix

>@@ -1393,16 +1393,25 @@ obj_eval(JSContext *cx, JSObject *obj, u
> 
>+        if (!OBJ_IS_NATIVE(scopeobj) || OBJECT_IS_XML(cx, scopeobj)) {

The "if" should check if scopeobj->map->ops->defineProperty is js_DefaultProperty to cover all cases like, for example, liveConnect objects that are still alive on 1.9.2. 

I also think that we should create the with object whenever scopeobj.parent is not null to avoid more surprises like the future.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that

Comment 9

[Igor Bukanov]

Comment on attachment 409129 [details] [diff] [review]
With that

>+        if (scopeobj->getParent() ||
>+            scopeobj->map->ops->defineProperty != js_DefineProperty) {

Final nit: add a comments that refers to this bug that we do not want to have an arbitrary object on the scope chain.

Comment 10

[Igor Bukanov]

(In reply to comment #7)
> I also think that we should create the with object whenever scopeobj.parent is
> not null to avoid more surprises like the future.

To be specific: currently only global, Call, With, Block and DOM event handlers can present on the scope chain. My worry is that if allow arbitrary objects on the scope chain, that may expose hidden bugs or lead to undesirable semantics. Hence the idea to avoid that and boxing surprises using With.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Gary, thanks for requesting checkin-needed, but in this case the patch in the bug is *not* ready for checkin yet (and I don't think the people who do checkin-needed triage are responsible for addressing review comments.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/b10d4f42ce96 with the comment. I noticed that the explicit test for the map->ops->defineProperty hook was unnecessary because any object with a null parent is a global object (and therefore must be a native object).

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #11)
> Gary, thanks for requesting checkin-needed, but in this case the patch in the
> bug is *not* ready for checkin yet (and I don't think the people who do
> checkin-needed triage are responsible for addressing review comments.

Apologies - thanks for the clarification!

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b10d4f42ce96

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

sayrer: this shouldn't block 1.9.2: the regressing patch never landed there (and won't land before final for sure).