535930 - TM: Crash [@ UpvarArgTraits::interp_get] or [@ GetUpvarArgOnTrace]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary [:streetwolf52]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091218 Minefield/3.7a1pre Firefox/3.5.5
Build Identifier: 20091218043634

Clicked on URL provided, page appeared briefly then crashed to desktop with the FF crash menu.

Reproducible: Always

Steps to Reproduce:
1. Go to URL provided
2.
3.
Actual Results:  
Crash to desktop

Expected Results:  
No crash

Crash Report:

http://crash-stats.mozilla.com/report/index/9da734e0-629e-4e04-bb54-f2c8e2091218

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091218 Minefield/3.7a1pre.

But no crash with javascript.options.jit.content to false.
No crash with Namoroko so far.
This must have regressed in the last few days.

Comment 2

[Matthias Versen [:Matti]]

0  	mozjs.dll  	GetUpvarOnTrace<UpvarArgTraits>  	 js/src/jstracer.cpp:3179
1 	mozjs.dll 	GetUpvarArgOnTrace 	js/src/jstracer.cpp:3199
2 	mozjs.dll 	js_MonitorLoopEdge 	js/src/jstracer.cpp:7089

Comment 3

[Ria Klaassen (not reading all bugmail)]

Crash still present in the latest tracemonkey build.

Comment 4

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

I'm on this. After some investigation, it's actually crashing at:

http://www.evri.com/widget/javascripts/ZiffDavis.1.js

Reduced testcase coming right up.

Comment 5

[IU]

Regression window is hard to find.

The last working TM build (and without the crash) was:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091205 Minefield/3.7a1pre
http://hg.mozilla.org/tracemonkey/rev/21b06416fa71

Then all TM builds (up to and including the 20091215 build) were unusable, crashing immediately on startup.  This started with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091206 Minefield/3.7a1pre
http://hg.mozilla.org/tracemonkey/rev/ced680bb4404

Finally, TM build started working again and this crash now exists, but did it begin with the 20091206 build?  First TM build to load again without crashing is:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091216 Minefield/3.7a1pre
http://hg.mozilla.org/tracemonkey/rev/6687fe3cda87

Comment 6

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

(function () {
    p = function () {
        Set()
    };
    var Set = function () {};
    for (var x = 0; x < 5; x++) {
        Set = function (z) {
            return function () {
                [z]
            }
        } (x)
    }
})()

crashes js debug shell at UpvarArgTraits::interp_get near null and js opt shell at GetUpvarArgOnTrace near null on TM tip with -j.

Huge pain to reduce, but it's eventually done. Setting security-sensitive because of memory addresses on stack, hopefully more-experienced eyes will re-diagnose this.

Debug stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000028
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-dbg-32-tm-darwin           	0x0016bae5 UpvarArgTraits::interp_get(JSStackFrame*, int) + 9
1   js-dbg-32-tm-darwin           	0x001729cd JSTraceType_ GetUpvarOnTrace<UpvarArgTraits>(JSContext*, unsigned int, int, unsigned int, double*) + 507
2   js-dbg-32-tm-darwin           	0x0014786b GetUpvarArgOnTrace(JSContext*, unsigned int, int, unsigned int, double*) + 51
3   ???                           	0x002e6ea0 0 + 3042976
4   js-dbg-32-tm-darwin           	0x00135b63 ExecuteTrace(JSContext*, nanojit::Fragment*, InterpState&) + 89
5   js-dbg-32-tm-darwin           	0x00168f90 ExecuteTree(JSContext*, TreeFragment*, unsigned int&, VMSideExit**) + 629
6   js-dbg-32-tm-darwin           	0x0016a2df js_MonitorLoopEdge(JSContext*, unsigned int&, RecordReason) + 995
7   js-dbg-32-tm-darwin           	0x0007d30a js_Interpret + 42491
8   js-dbg-32-tm-darwin           	0x0009c0b8 js_Execute + 1169
9   js-dbg-32-tm-darwin           	0x000116d2 JS_ExecuteScript + 54
10  js-dbg-32-tm-darwin           	0x0000a617 Process(JSContext*, JSObject*, char*, int) + 1347 (js.cpp:532)
11  js-dbg-32-tm-darwin           	0x0000afe8 ProcessArgs(JSContext*, JSObject*, char**, int) + 2281 (js.cpp:849)
12  js-dbg-32-tm-darwin           	0x0000b3b5 main + 953 (js.cpp:4862)
13  js-dbg-32-tm-darwin           	0x0000246d _start + 208
14  js-dbg-32-tm-darwin           	0x0000239c start + 40

Opt stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000028
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x000f13cb GetUpvarArgOnTrace(JSContext*, unsigned int, int, unsigned int, double*) + 187
1   ???                           	0x003d2ea0 0 + 4009632
2   js-opt-32-tm-darwin           	0x000f9398 ExecuteTree(JSContext*, TreeFragment*, unsigned int&, VMSideExit**) + 744
3   js-opt-32-tm-darwin           	0x00114434 js_MonitorLoopEdge(JSContext*, unsigned int&, RecordReason) + 1172
4   js-opt-32-tm-darwin           	0x0005bb8c js_Interpret + 56972
5   js-opt-32-tm-darwin           	0x0005db2c js_Execute + 444
6   js-opt-32-tm-darwin           	0x0000d8dc JS_ExecuteScript + 60
7   js-opt-32-tm-darwin           	0x00004658 Process(JSContext*, JSObject*, char*, int) + 1336
8   js-opt-32-tm-darwin           	0x000086d6 main + 1734
9   js-opt-32-tm-darwin           	0x000025fd _start + 208
10  js-opt-32-tm-darwin           	0x0000252c start + 40

Comment 7

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

Was tested on 10.6.2 and assuming [sg:critical?] unless otherwise noted.

AutoBisecting now...

Comment 8

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

autoBisect shows this is probably related to bug 515749:

The first bad revision is:
changeset:   35427:538a07fdf3d2
user:        David Anderson
date:        Fri Dec 11 19:10:36 2009 -0800
summary:     Lazily import stack and global slots (bug 515749, original patch and r=gal).

Comment 9

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

dvander would like this assigned to him... (over IRC)

Comment 10

[Jesse Ruderman]

Slightly simpler:

(function () {
    var Set;
    var p = function () {
        Set;
    };
    for (var x = 0; x < 5; x++) {
        Set = (function (z) {
            return function () {
                z;
            };
        }) (x)
    }
})()

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

The meaning of TraceRecorder::known() is different now that we lazily import slots. I did not catch this case though, so it generates bad code.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/f7cff6dd16f1

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Backed out, get() asserts somewhere. http://hg.mozilla.org/tracemonkey/rev/28ba3d9a651f

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: actual fix

Brain fart, of course get() expects an on-trace value, so it will always return something. This version passes closure tests.

Comment 15

[David Mandelin [:dmandelin]]

Comment on attachment 418750 [details] [diff] [review]
actual fix

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -12126,16 +12126,23 @@ TraceRecorder::upvar(JSScript* script, J
>      */
>     uint32 cookie = uva->vector[index];
>     jsval& vr = js_GetUpvar(cx, script->staticLevel, cookie);
>     v = vr;
> 
>     if (known(&vr))
>         return get(&vr);
> 
>+    /* If the variable was not known, it could require a lazy import. */
>+    CountSlotsVisitor countVisitor(&vr);
>+    VisitStackSlots(countVisitor, cx, callDepth);
>+
>+    if (countVisitor.stopped() || size_t(&vr - cx->fp->slots) < cx->fp->script->nslots)
>+        return get(&vr);
>+

How about:

bool TraceRecorder::can_get(jsval *vp)
{
    if (known(vp))
        return true;

    /* If the variable was not known, it may be available for lazy import. */
    CountSlotsVisitor countVisitor(&vr);
    VisitStackSlots(countVisitor, cx, callDepth);
    return countVisitor.stopped() || size_t(&vr - cx->fp->slots) < cx->fp->script->nslots;
}

and then

    if (can_get(&vr))
        return get(&vr);

It seems good for confusion reduction to have an direct API that says whether |get| will succeed.

Comment 16

[Bob Clary [:bc] (inactive)]

also http://time.yandex.ru

Comment 17

[chris hofmann]

crash data also says

   2 http://www.washingtonpost.com/wp-dyn/content/article/2009/12/13/AR2009121300007.html?nav=hcmodule
   1 http://www.washingtonpost.com/wp-dyn/content/article/2009/12/18/AR2009121804042.html?hpid=topnews
   1 http://www.washingtonpost.com/wp-dyn/content/article/2009/12/13/AR2009121300007.html?waporef=ak

is this trunk only or are patches needed for 1.9.2?  the only reports I've seen in the crash database show 3.7a1pre and were logged on 12-18,19,20

20091217-crashdata 0 GetUpvarArgOnTrace
20091218-crashdata 14 GetUpvarArgOnTrace
20091219-crashdata 16 GetUpvarArgOnTrace
20091220-crashdata 9 GetUpvarArgOnTrace

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

This only affects trunk after 2009-12-11.

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (From update of attachment 418750 [details] [diff] [review])
> bool TraceRecorder::can_get(jsval *vp)

Once upon a time we had a has() method, shorter name than known or can_get, and it avoids the underscore war against camelCaps (_ is righteous here, admittedly).

/be

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3 actual patch

attached the wrong patch.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/167467b7b439

Comment 24

[Benjamin Smedberg]

If it helps, I can reproduce this pretty reliably on many pages of washingtonpost.com

Comment 25

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/167467b7b439