455757 - TM: Loading www.netvibes.com crashes firefox with tracemonkey (jit) enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dominic Williams]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080917020400 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080917020400 Minefield/3.1b1pre

Easy to replicate (on my machinie). Loading www.netvibes.com crashes everytime when Tracemonkey is enabled, and works when disabled. Confirmed not to bebecause of any specific applet on my personalised netvibes.com page and crashes on the default www.netvibes.com page.

Reproducible: Always

Steps to Reproduce:
1. Enable tracemonkey (jit)
2. Go to www.netvibes.com
3. Watch it crash..
Actual Results:  
Firefox crashes (it loads the titlebar information..) and the "send an error" window pops up

Expected Results:  
Load the website

A few nightly's back (maybe a week or slightly less) and it was working fine.

Comment 1

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

Attachment: two related test cases for trace-tests.js

What's breaking here is that we're not importing args past the formal argument count.  Originally we imported everything but it was changed here:

http://hg.mozilla.org/tracemonkey/rev/074d733dbdb8

With the current TM tip, the argc > nargs test fails.  With reverting that changeset, the first test fails instead.  We compile a trace expecting args=2, and the second call fills the second argument with 0.  The third call segfaults as js_NativeStackSlots(cx, 0) == 7 and the entry type map length is 6.

Should we be guarding in js_ExecuteTree that the incoming argc <= args for the trace we compiled?

Comment 3

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

Attachment: aborts trace if accessing arguments beyond nargs, fixes a related crash

So, importing argc beyond nargs seems difficult, if not impossible because we could try to execute a tree with more argc beyond nargs but less argc than originally recorded.

Attached is a preliminary patch that fixes two of the crashes encountered so far.

Comment 4

[Brendan Eich [:brendan]]

Idea: guard early in record_JSOP_CALL on argc matching, with BRANCH_EXIT exit type. This could help 3d-raytrace.js too!

/be

Comment 5

[Andreas Gal :gal]

But what about the initial frame (calldepth == 0)? That we would have to specialize using several trees. Or we have to make the stack unfolding explicit in the tree so that we can BRANCH_EXIT on that somehow.

Comment 6

[Robert Sayre]

This is the crash we're currently hitting in mochitest as well.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 339162 [details] [diff] [review]
aborts trace if accessing arguments beyond nargs, fixes a related crash

>+            unsigned int nargs = JS_MIN((JS_MIN(argc, fp->fun->nargs)), 2);
>+            for (uintN i = 0; i < nargs; i++) {

Drive-by nit: avoid 'unsigned int', use unsigned or uintN but not both in adjacent or new code.

This is the apply(_, arguments) special case, and it handles argc > nargs.

>@@ -5880,7 +5881,7 @@ TraceRecorder::record_JSOP_ARGSUB()
>     JSStackFrame* fp = cx->fp;
>     if (!(fp->fun->flags & JSFUN_HEAVYWEIGHT)) {
>         uintN slot = GET_ARGNO(fp->regs->pc);
>-        if (slot < fp->argc && !fp->argsobj) {
>+        if (slot < fp->fun->nargs && !fp->argsobj) {

This case doesn't handle argc > nargs (slot >= nargs), but the interpreter and the language standard it's based on do:

js> function f(x,y){return arguments[2]}
js> f(1,2,3)
3

Ok, it's better to abort than crash! I'm with you on that. I just wonder why we can't import all actuals if there are more than formals. Sorry if you explained it on IRC and I missed it. Thanks,

/be

Comment 8

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

I'll switch to uintN, having 'unsigned' without 'int' bothers me for some reason :]

The problem I ran into was, say nargs=2, argc=6.  If a trace gets compiled for that expecting argc=6, and we later try to execute with argc=3, we can't seem to import the missing arguments since there's no space on the stack for them.

Comment 9

[Andreas Gal :gal]

Comment on attachment 339162 [details] [diff] [review]
aborts trace if accessing arguments beyond nargs, fixes a related crash

Lets get this in. We can be more creative with argc > nargs if the need arises.

Comment 10

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

Pushed as changeset http://hg.mozilla.org/tracemonkey/rev/b38dc7abba98

Comment 11

[Brendan Eich [:brendan]]

First hunk in patch caused bug 455763.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

test also included in js1_8_1/trace/trace-test.js