624110 - TM: Crash [@ js::TraceRecorder::traverseScopeChain] or "Assertion failure: isObject()," or TraceRecorder::traverseScopeChain doesn't handle call objects for strict mode eval frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

./js -j -m
var COUNT = 10; eval("'use strict'; for (let j = 0; j < COUNT; j++); x;");
Assertion failure: isObject(), at ../jsvalue.h:602
Aborted (core dumped)

The method performs a scope chain walk looking for Call objects, and it blindly calls getCallObjCalleeFunction() at one point -- but that's not valid on strict eval Call objects, and we quickly assert in debug builds, probably crash in opt builds.

This'll bite any strict mode eval that tries to use a variable not defined in the eval code in code the tracer tries to record, which seems to warrant fixing before we release strict eval to the masses in (probably) b9.

Comment 1

[Jeff Walden [:Waldo]]

Recording a couple more failures here to make sure I fix everything here:

var COUNT = 10;
function t() { eval("'use strict'; for (let j = 0; j < COUNT; j++); x;"); }
t()
Assertion failure: fp->hasCallObj(), at ../jsinterpinlines.h:735


var COUNT = 10;
function t() { eval("'use strict'; for (let j = 0; j < COUNT; j++); x;"); }
try { t(); throw new Error('fail'); } catch (e) { assertEq(e instanceof ReferenceError, true); }
Assertion failure: fp->hasCallObj(), at ../jsinterpinlines.h:735

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

This was marked as a beta9 blocker after code freeze and before we spun builds, and didn't make it into the build. What gives?

Comment 3

[Andreas Gal :gal]

Eh. This is rare if ever used on the web but a test case that crashes us was filed openly, so if we ship this, that blows. I will check what kind of crash. This might be not sg:critical, just sg:dos.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving back to a nomination based on comment 3 - we can always get it in a dot release, or approve a safe patch.

Comment 5

[Jeff Walden [:Waldo]]

I'm pretty sure this bug is not a security concern.  It's a call object where we call getCallObjCalleeFunction() on it.  That retrieves a slot which, for ordinary callobj-having frames is non-null, and pulls a function private out of it.  For strict eval frames it's null, so pulling a function private from it should be a safe near-null crash.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Patch and test

This fixes the testcase.

More interesting, perhaps, is what's not changed -- all the other places that call getCallObjCallee(Function)?.  The key is that eval variables are defined with PropertyStub, so the getterOp and setterOp are NULL.  This causes all the callers in methodjit, and in the tracer (save these two), to never be hit, because they depend on a shape having a particular non-stub op -- either to be hit (the methodjit bits), or to cause their callers to never be called in that situation (the tracer bits).  But I could probably use a double-check here.  :-)

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 503033 [details] [diff] [review]
Patch and test

I'm really the wrong one for this; probably jorendorff or brendan.

My only comment is that it would be nice to factor out a "call and (for eval or heavy)" predicate, called from both places, and lay down a thick layer of comment icing as to why we care about some call objects and not others (I had trouble understanding this from the current comment near the top of traverseScopeChain).

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 503033 [details] [diff] [review]
Patch and test

To be honest, I'm not even entirely sure I understand the reason for the distinction.  Heavyweight means "requires Call object" (ignoring strict eval), so it would have seemed to me unnecessary.  But the change did seem in the spirit of the current code, and I was loathe to make less careful changes now without a better understanding of the code.

Comment 9

[Brendan Eich [:brendan]]

See bug 622781. We need to fix that bug first and rebase here.

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 503033 [details] [diff] [review]
Patch and test

Luke's suggestion about an inline helper to common that chunky condition is good, but if we can get rid of this code (or simplify it both in size and in how often it runs) by fixing bug 622781 first, we should.

/be

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 503033 [details] [diff] [review]
Patch and test

It would seem to me better to keep this "chunky", actually.  That will make it clearer that further changes are needed, in a particular way, when we refactor scope chain handling to make it no longer object-based.  (If bug 622781 beats it, all the better, but I'd rather not hold my breath when we can fix this mistake now.)  Can you explain what that condition actually does?

But if I'm to fix this by adding an inline helper, despite my thinking it potentially less illuminating in the future, could you at least explain its point to me?

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 503033 [details] [diff] [review]
Patch and test

Waldo: no deep explanation is needed. An inline helper is just commoning repeated code -- DRY.

Were you asking about the isHeavyweight condition? I'm not sure why that is there, probably in case a lightweight function has a Call object created for it due to debugger mischief. That "should not happen", though. Maybe dmandelin remembers more.

There's no guarantee this open-coding will help or hinder any revision of scopes to use non-objects for activation scopes. The code is full of Call object uses. If an inline helper is added, it will test &js_CallClass and then you'll have to find its callers. But clearly we can go too far with this argument, and open-code too much.

For now this seems fine.

/be

Comment 13

[Jeff Walden [:Waldo]]

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

Comment 14

[Jeff Walden [:Waldo]]

Backed out in a blaze of orange.  Not for problems with this patch, but for problems with a patch that landed at the same time as it, then its followups, then complete orangetastrophe which might either have been those patches' fault or a different push not by me at all.  I'll figure it out tomorrow afternoon hopefully.

Comment 15

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/46b9f2a8343b

Comment 16

[David Mandelin [:dmandelin]]

(In reply to comment #12)
> Were you asking about the isHeavyweight condition? I'm not sure why that is
> there, probably in case a lightweight function has a Call object created for it
> due to debugger mischief. That "should not happen", though. Maybe dmandelin
> remembers more.

I don't remember exactly. I remember there was a fear of debugger mischief at the time. But I don't remember whether that condition was required to pass test failures or if it was theoretical.

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/bc2ae2ec69c4
http://hg.mozilla.org/mozilla-central/rev/46b9f2a8343b

Comment 18

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.