431698 - Crash [@JS_CallTracer|array_trace] browser js1_5/GC/regress-203278-2.js

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

on centos5 x86_64 the stack looks like 

#1  0x00002aaaaad66589 in array_trace (trc=0x7fffa4f8f6e0, obj=0x11950540) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsarray.c:1029
#2  0x00002aaaaada14e8 in JS_TraceChildren (trc=0x7fffa4f8f6e0, thing=0x11950540, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2233
#3  0x00002aaaaada22f9 in JS_CallTracer (trc=0x7fffa4f8f6e0, thing=0x11950540, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2517

with many repeats deep.

On Fedora6 it looks like

#0  0x002bb011 in JS_CallTracer (trc=0xbf8ca5d8, thing=0x12570620, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2437
#1  0x00282e8b in array_trace (trc=0xbf8ca5d8, obj=0x12570660) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsarray.c:1029
#2  0x002ba6a5 in JS_TraceChildren (trc=0xbf8ca5d8, thing=0x12570660, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2233
#3  0x002bb4d0 in JS_CallTracer (trc=0xbf8ca5d8, thing=0x12570660, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2517
#4  0x00282e8b in array_trace (trc=0xbf8ca5d8, obj=0x125706a0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsarray.c:1029

This crash does not always occur and may require the use of the Spider extension. I have reproduced on fedora6 and centos5 x86_64. You may have to run the test twice to see the crash after a fresh build.

1. Install Spider
   <http://bclary.com/projects/spider/spider/spider.xpi>

2. Start Spider and run test

firefox -spider -url "http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/GC/regress-203278-2.js;language=type;text/javascript" -hook http://test.bclary.com/tests/mozilla.org/js/userhookeach.js -start -quit

making sensitive for the moment until someone can look at this.

I'm pretty sure this is a regression. I'll try to narrow it down more in a bit.

Comment 1

[Bob Clary [:bc] (inactive)]

This crash is pretty old, so far at back to 3/27. It typically appears the second time the test is run in Spider which is why it hasn't shown up more reliably in the past. The top frame in many of the crashes is

#0  0x00002aaaaada025b in GetGCThingFlags (thing=Cannot access memory at address 0x7fff0a51dff8
)

Comment 2

[Bob Clary [:bc] (inactive)]

I run into different crashes if I go back as far as Feb. Removing regression keyword since this may not be one.

Comment 3

[Igor Bukanov]

I will look into it.

Comment 4

[Igor Bukanov]

To Bob Clary:

I cannot reproduce this. Do you see this reliably with spider extension on Linux?

Comment 5

[Bob Clary [:bc] (inactive)]

Yes. It does not appear unless you invoke it using Spider. It does not appear the first time you run it. But it does appear the second and subsequent times.

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Yes. It does not appear unless you invoke it using Spider. It does not appear
> the first time you run it. But it does appear the second and subsequent times.
> 
Since I cannot reproduce this, do you have time to debug this over IRC? 

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> (In reply to comment #5)
> > Yes. It does not appear unless you invoke it using Spider. It does not appear
> > the first time you run it. But it does appear the second and subsequent times.
> > 
> Since I cannot reproduce this, do you have time to debug this over IRC? 
> 

In particular, I would like to know what is the value of JSContext.stackLimit when the crash happens. If the limit is not set correctly, then segfaults happens for sure with the test case.

Comment 8

[Igor Bukanov]

Yet one more thing to check is to compile js with -DJS_GC_ASSUME_LOW_C_STACK or, alternatively, changing the line 2514 in js/src/jsgc.c from

    if (RECURSION_TOO_DEEP()) 

to

    if (1 || RECURSION_TOO_DEEP()) 

If this cures the crash, the crash happens due to incorrectly set stack limit.

Comment 9

[Bob Clary [:bc] (inactive)]

After looking at the stack, I believe Igor's opinion is a faulty stack limit. The first call to JS_CallTracer is

#130838 0x00002aaaaada2351 in JS_CallTracer (trc=0x7fffd99d6110, thing=0x788bc80, kind=0) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:2517
#130839 0x00002aaab015312d in XPCWrappedNativeProto::TraceJS (this=0x78b9ec0, trc=0x7fffd99d6110)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/xpconnect/src/xpcprivate.h:1941
#130840 0x00002aaab019ecda in XPCWrappedNative::TraceJS (this=0x7878190, trc=0x7fffd99d6110)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/xpconnect/src/xpcprivate.h:2287

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: stack bottom

Comment 11

[Igor Bukanov]

The reason for the bug is that the debugger depending on the setup may call nsJSRuntimeServiceImpl::GetRuntime and use the result to create JSContext before XPCJSRuntime::XPCJSRuntime calls the function and sets the context callback. Thus this debugger-created context would not have the stack limit initialized leading to over-recursion during when the context runs GC.
 

Comment 12

[Igor Bukanov]

Attachment: v1

The patch moves the callback initialization to nsJSRuntimeServiceImpl::GetRuntime to make sure that they are set before any user of the JSRuntime instance.

To Bob Clary: can you test the patch to check that this fixes the issue?

Comment 13

[Bob Clary [:bc] (inactive)]

will do. eta a little bit.

Comment 14

[Bob Clary [:bc] (inactive)]

it works on 64 bit and 32 but linux for me. Thanks Igor!

Comment 15

[Igor Bukanov]

Comment on attachment 319870 [details] [diff] [review]
v1

The patch makes sure that all JSRuntime callbacks are initialized before the the first context is created.

Comment 16

[Igor Bukanov]

To Bob Clary: can you try also the test case from bug 432231? If, of cause, you was able to reproduce the crash there.

Comment 17

[Bob Clary [:bc] (inactive)]

Sorry, can't reproduce that one.

Comment 18

[Bob Clary [:bc] (inactive)]

Igor, should this remain security sensitive?

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> Igor, should this remain security sensitive?

The bug happens only when uses a some sort of debugger extension. The consequence of the bug is native stack overflow. This is not exploitable with normal setup. So I guess we can drop the security bit.

Comment 20

[Andre-John Mas]

I am not sure whether this bug 434236 is related?

Comment 21

[Nickolay_Ponomarev]

Any reason this shouldn't be checked in?

Comment 22

[Nickolay_Ponomarev]

Er, I looked at the wrong branch. Apparently similar changes were made in bug 408539. Should this bug be closed?

Comment 23

[Bob Clary [:bc] (inactive)]

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&version=Firefox%3A3.1b3pre&signature=JS_CallTracer is the #2 crash for firefox 3.1b3.

Comment 24

[Robert Sayre]

igor, what's the status here?

Comment 25

[Igor Bukanov]

(In reply to comment #24)
> igor, what's the status here?

Sorry, this went out of focus. I am on it.

Comment 26

[Igor Bukanov]

(In reply to comment #23)
> http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&version=Firefox%3A3.1b3pre&signature=JS_CallTracer
> is the #2 crash for firefox 3.1b3.

These stats have nothing to do with this bug. The popularity of JS_CallTracer just reflects that, when the code frees memory for a live object, the crash most likely happens when the object is reached and dereferenced. This is often happens during the marking phase of the GC when the live object graph is traversed using JS_CallTracer.

Comment 27

[Igor Bukanov]

The necessary changes (In reply to comment #22)
> Apparently similar changes were made in bug 408539. Should this bug be closed?

Yes - the changes from the bug 408539 also makes sure that the context callback is set before any context is created.

Comment 28

[Igor Bukanov]

Recording that this is fixed on 1.9.1 - the patch for the bug 408539 that fixes this bug as well is there.

Comment 29

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 only have abnormal 5 exits  in shell due to out of memory.