380833 - Crash during GC after uneval (involves E4X, mysterious sharp variable)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> www = <x><y/></x>; print(uneval(this) + "\n"); gc();
({www:#1=<x>
  <y/>
</x>})


Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdddddc00

Thread 0 Crashed:
0   js 	0x00053264 js_GetGCThingFlags + 26 (jsgc.c:512)
1   js 	0x00055663 JS_CallTracer + 495 (jsgc.c:2146)
2   js 	0x0008696b js_TraceObject + 811 (jsobj.c:4898)
3   js 	0x000548ec JS_TraceChildren + 140 (jsgc.c:1812)
4   js 	0x0005577d JS_CallTracer + 777 (jsgc.c:2172)
5   js 	0x0008696b js_TraceObject + 811 (jsobj.c:4898)
6   js 	0x000548ec JS_TraceChildren + 140 (jsgc.c:1812)
7   js 	0x0005577d JS_CallTracer + 777 (jsgc.c:2172)
8   js 	0x00055ccb js_TraceStackFrame + 355 (jsgc.c:2333)
9   js 	0x0005639a js_TraceContext + 124 (jsgc.c:2445)
10  js 	0x000568d5 js_TraceRuntime + 179 (jsgc.c:2519)
11  js 	0x00056b3b js_GC + 530 (jsgc.c:2747)
12  js 	0x000161a7 JS_GC + 96 (jsapi.c:2372)
13  js 	0x000037f6 GC + 45 (js.c:742)
14  js 	0x00059941 js_Invoke + 2945 (jsinterp.c:1332)
15  js 	0x0006a376 js_Interpret + 62065 (jsinterp.c:4025)
16  js 	0x0005a295 js_Execute + 715 (jsinterp.c:1591)
17  js 	0x0001b02f JS_ExecuteScript + 54 (jsapi.c:4692)
18  js 	0x00002692 Process + 912 (js.c:268)
19  js 	0x0000309b ProcessArgs + 2045 (js.c:519)
20  js 	0x00007fb0 main + 612 (js.c:3256)
21  js 	0x000021a6 _start + 216
22  js 	0x000020cd start + 41

Comment 1

[Jesse Ruderman]

With less contamination:

js> o = {}; o.a = <x><y/></x>; print(uneval(o)); gc();
({a:#1=<x>
  <y/>
</x>})
Segmentation fault

Comment 2

[Igor Bukanov]

The problem is caused by the fact that during uneval(obj) the runtime will call
JS_Enumerate of the xml object and that eventually will invoke
xml_lookupProperty with id == 0 corresponding to the first child of the xml.
That in turn calls js_AddNativeProperty and that locks the object and calls
js_AddScopeProperty.

In this particular case js_AddScopeProperty will allocate a new slot via
calling js_AllocSlot. That slot is explicitly left uninitialized under the
assumption that somebody will eventually set it before unlocking the scope. But
this is never done in js_AddNativeProperty leaving the junk value (or explicit
0xddddddd8 marker with a debug build) in the slot and eventually crashing
during tracing the object graph during GC.

So I guess js_AddNativeProperty has to be patched to initialize the slot to
something, but I am not sure under which conditions.

Comment 3

[Igor Bukanov]

Here is almost trivial case to expose the same problem with uninitialized slot after js_AddNativeProperty:

js> var x = <x><y/></x>; 0 in x; gc();
Segmentation fault

Comment 4

[Jesse Ruderman]

Is the "mysterious" (seemingly unnecessary) use of a sharp variable here related to that problem, or is it a separate bug?

Comment 5

[Jesse Ruderman]

Igor, do you know which patch from the last few days led to this regression?

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Igor, do you know which patch from the last few days led to this regression?

I think this is an old problem, will test it later today.

Comment 7

[Igor Bukanov]

(In reply to comment #4)
> Is the "mysterious" (seemingly unnecessary) use of a sharp variable here
> related to that problem, or is it a separate bug?

This is a separated and unrelated bug.

Comment 8

[Igor Bukanov]

This is a recent regression, see bug 308429. I was wrong to advise to set the slot to 0xddddddd8 there. If for some reason under way-too-much GC it was necessary to set the slot to void in AllocSlot, then there was bug somewhere else. At that point the free slot must be void as either ReallocSlot or js_Clear/js_FreeSlot sets the available slots to void.

Comment 9

[Igor Bukanov]

Attachment: fix v1

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 265031 [details] [diff] [review]
fix v1

Whew, glad this was caught quickly.

/be

Comment 11

[Igor Bukanov]

I committed the patch from comment 9 to the trunk:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.337; previous revision: 3.336
done

Comment 12

[Jesse Ruderman]

I filed bug 380946 on the unnecessary sharping.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-380833.js

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

/cvsroot/mozilla/js/tests/e4x/Regress/regress-380833.js,v  <--  regress-380833.js
initial revision: 1.1

Comment 15

[Daniel Veditz [:dveditz]]

Wanted on the 1.8 branch if we take bug 308429

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1.15 linux/mac/win