557841 - TM: "Assertion failure: BINDNAME in global code resolved to non-global object, at ../jstracer.cpp"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

({e:(eval("for(let a = 0; a < 4; a++){x = print}",[])),get 6()("")})


asserts js debug shell on TM tip with -j at Assertion failure: BINDNAME in global code resolved to non-global object, at ../jstracer.cpp:13679

autoBisect shows this is probably related to bug 556937:

The first bad revision is:
changeset:   40429:9acb5bcc134f
user:        Jason Orendorff
date:        Tue Apr 06 16:49:33 2010 -0500
summary:     Bug 556937 - TM: Trace JSOP_SETNAME when preceding BINDNAME produces the global object. r=dmandelin.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

The fact that we trace in eval like that, when there's a With object on the scope chain, is pretty dumb. We record the loop, emit code for it, and then never execute it, because ExecuteTree calls ScopeChainCheck.

Comment 2

[Brendan Eich [:brendan]]

Why is two-arg eval still alive? Someone please take bug 531675.

Jason: could this bug be fixed by moving ScopeChainCheck to the recorder?

/be

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> Jason: could this bug be fixed by moving ScopeChainCheck to the recorder?

Even if we check the scope chain in startRecorder (or wherever), I think we still need the check in ExecuteTree (or somewhere) to guard against weird API use like:

  script = JS_CompileScript(...)
  JS_ExecuteScript(cx, script, global, ...)  // record a trace
  JS_ExecuteScript(cx, script, badobj, ...)  // execute it

and probably variations on that theme using JS_CloneFunctionObject, etc.

I favor checking in both places, because the invariant "we don't have non-cacheable objects on the scope chain at record time" seems only sane. I'll file and patch a followup bug if you agree.

Comment 4

[Brendan Eich [:brendan]]

Yes, have to check both places -- I should have written "copying" not "moving".

/be

Comment 5

[David Mandelin [:dmandelin]]

Do you still want this patch reviewed or are you going to do a new one?

Comment 6

[Jason Orendorff [:jorendorff]]

Please review it. I'll file a followup bug.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2 - check before recording starts

hg got this diff wrong. It shows me moving a big chunk of code up; actually I moved the asserts down.

Comment 8

[David Mandelin [:dmandelin]]

Comment on attachment 439627 [details] [diff] [review]
v2 - check before recording starts

Looks good. The new abort reason should be added to js/src/tracevis/acts.py. r+ with that.

Comment 9

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/6c84ffb9c16e

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6c84ffb9c16e

Comment 11

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug557841.js.