590088 - JM: Crash [@ js::Mark] or "Assertion failure: thing,"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Function("var a=e=(function(){*::*}).prototype")()
gc()

crashes js opt shell on JM changeset e42b505b43f3 with -m but without -j at js::Mark and asserts js debug shell at Assertion failure: thing, at ../jsgc.cpp:2115

(gdb) bt
#0  0x00056e03 in js::Mark ()
#1  0x00080130 in js_TraceObject ()
#2  0x000570b6 in js::Mark ()
#3  0x00055da1 in JS_TraceChildren ()
#4  0x000570b6 in js::Mark ()
#5  0x00080130 in js_TraceObject ()
#6  0x000570b6 in js::Mark ()
#7  0x00055da1 in JS_TraceChildren ()
#8  0x000570b6 in js::Mark ()
#9  0x00055d86 in JS_TraceChildren ()
#10 0x000570b6 in js::Mark ()
#11 0x00055d86 in JS_TraceChildren ()
#12 0x000570b6 in js::Mark ()
#13 0x00057c23 in js::MarkConservativeStackRoots ()
#14 0x00057f0b in js_TraceRuntime ()
#15 0x000588d7 in js_GC ()
#16 0x00010767 in JS_GC ()
#17 0x000066b3 in GC ()
#18 0x001d5a07 in js::mjit::stubs::SlowCall ()
#19 0x002d7557 in ?? ()
#20 0x00189152 in js::mjit::JaegerShot ()
#21 0x0006fdb1 in js::Execute ()
#22 0x00014bb8 in JS_ExecuteScript ()
#23 0x0000564c in Process ()
#24 0x000093c7 in shell ()
#25 0x000098f8 in main ()
(gdb) x/i $eip
0x56e03 <_ZN2js4MarkEP8JSTracerPvj+147>:        mov    %edx,(%esi)
(gdb) x/b $edx
0x85000001:     Cannot access memory at address 0x85000001
(gdb) x/b $esi
0xfa000 <_ZL10str_searchP9JSContextjPN2js5ValueE+896>:  0x00

Comment 1

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

Attachment: fix

Great bug. JM is putting garbage in the call object. Two options: 1) conservatively scan the slots, and 2) only copy slots that can escape (leaving the rest undefined).

This patch takes the second approach as I think long-term it will lead to more optimal code. Block objects probably have the same bug, I will file a follow-up.

Comment 2

[David Mandelin [:dmandelin]]

Comment on attachment 468881 [details] [diff] [review]
fix

Just to make sure I understand the cause, the copying part doesn't work because the slots are not necessarily up to date, in particular if they are constants? And you fixed it this way instead of making sure the slots are flushed because flushing the slots would slow us down?

Comment 3

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

> the slots are not necessarily up to date

Yes, though in this case, slots we don't care about - ones that can't escape. The call object copies them all blindly. JM doesn't sync unescaping slots, so they are garbage memory.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> > the slots are not necessarily up to date
> 
> Yes, though in this case, slots we don't care about - ones that can't escape.
> The call object copies them all blindly. JM doesn't sync unescaping slots, so
> they are garbage memory.

Got it. Thanks.

Comment 5

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

Landed, but then backed out due to reftest failure.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch, revised to fix the bug that bounced it off tinderbox

The problem was that the new CALLS_EVAL function/script bit introduced for strict mode arguments is only propagated upward (to parents of closures) if both are strict mode. But the way we use that function in JM requires it to propagate always. In particular, for this bug, we need to copy all values into call objects on function exit if eval is called transitively, because any eval at any level down could access any local variable.

Comment 7

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

Comment on attachment 470066 [details] [diff] [review]
Patch, revised to fix the bug that bounced it off tinderbox

Nice find, thanks!

Comment 8

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/projects/jaegermonkey/rev/145621513207

Comment 9

[Christian Holler (:decoder)]

E4X has been removed, so we won't add the test.