Last Comment Bug 380833 - Crash during GC after uneval (involves E4X, mysterious sharp variable)
: Crash during GC after uneval (involves E4X, mysterious sharp variable)
Status: VERIFIED FIXED
[sg:critical] regression from 308429
: crash, regression, testcase, verified1.8.1.15
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz 308429 426628
  Show dependency treegraph
 
Reported: 2007-05-15 20:16 PDT by Jesse Ruderman
Modified: 2008-06-11 01:53 PDT (History)
6 users (show)
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x-
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (944 bytes, patch)
2007-05-16 12:46 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
e4x/Regress/regress-380833.js (2.03 KB, text/plain)
2007-05-27 16:36 PDT, Bob Clary [:bc:]
no flags Details

Description Jesse Ruderman 2007-05-15 20:16:01 PDT
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 2007-05-15 20:26:12 PDT
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 2007-05-16 01:25:29 PDT
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 2007-05-16 01:38:05 PDT
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 2007-05-16 02:29:01 PDT
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 2007-05-16 02:29:42 PDT
Igor, do you know which patch from the last few days led to this regression?
Comment 6 Igor Bukanov 2007-05-16 03:47:04 PDT
(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 2007-05-16 03:48:03 PDT
(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 2007-05-16 12:45:43 PDT
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 2007-05-16 12:46:54 PDT
Created attachment 265031 [details] [diff] [review]
fix v1
Comment 10 Brendan Eich [:brendan] 2007-05-16 15:26:13 PDT
Comment on attachment 265031 [details] [diff] [review]
fix v1

Whew, glad this was caught quickly.

/be
Comment 11 Igor Bukanov 2007-05-16 15:43:03 PDT
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 2007-05-16 16:09:25 PDT
I filed bug 380946 on the unnecessary sharping.
Comment 13 Bob Clary [:bc:] 2007-05-27 16:36:58 PDT
Created attachment 266301 [details]
e4x/Regress/regress-380833.js
Comment 14 Bob Clary [:bc:] 2007-09-18 12:49:48 PDT
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] 2008-04-02 15:04:07 PDT
Wanted on the 1.8 branch if we take bug 308429
Comment 16 Bob Clary [:bc:] 2008-06-11 01:53:33 PDT
verified fixed 1.8.1.15 linux/mac/win

Note You need to log in before you can comment on or make changes to this bug.