482263 - TM: "Assertion failure: x->oprnd2() == lirbuf->sp || x->oprnd2() == gp_ins, at ../jstracer.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

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

__proto__.x getter= function () { return <y/>.([]) }
for each (let x in []) { for each (let x in ['', '']) { } }

asserts at Assertion failure: x->oprnd2() == lirbuf->sp || x->oprnd2() == gp_ins, at ../jstracer.cpp in debug js shell (with -j only), seems to work as expected in opt (with and without -j).

This should be a regression of bug 471821.

The first bad revision is:
changeset:   23225:b369b9f805ba
user:        Andreas Gal
date:        Fri Jan 02 10:55:02 2009 -0800
summary:     Use a single lirbuf for the tracer and rewind lirbuf during GC (471821, r=danderson).

Comment 1

[Graydon Hoare :graydon]

Attachment: Fix the bug

We're ignoring deep-abort conditions in the TRACE_ARGS_ macro. This has the unwelcome effect of re-entering the recorder when it's officially "shut down" (deep aborting), and in particular when we've already rewound the LIR buffer and invalidated the tracker. 

This bug hits it as a symptom, but the root cause is much wider-reaching and will produce all manner of mysterious behavior.

As far as I know this patch fixes it. Passes the testcase, sunspider, trace-test, and alexa global 100 load in browser.

Comment 2

[Graydon Hoare :graydon]

Note that I *think* this line is essentially just an adaptation of the mrbkap action in rev http://hg.mozilla.org/tracemonkey/rev/66ce4cd00877 back in August, but to a macro that didn't even exist back then. So I asked mrbkap to review. Perhaps brendan might prefer to look or comment?

Comment 3

[Brendan Eich [:brendan]]

This is the right fix. We don't js_DeleteRecorder to avoid unwinding on a deleted |this|. The TRACE_[012] macros, which all use the otherwise-unused TRACE_ARGS_ helper macro, are for calls to the recorder "mid-opcode" in the interpreter. At that point we could have deep-aborted from the "pre-opcode" monitorRecording path.

/be

Comment 4

[Graydon Hoare :graydon]

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

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/dd4c754ae365

Comment 6

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/db3446c6de86
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-482263.js,v  <--  regress-482263.js
initial revision: 1.1

Comment 7

[Robert Sayre]

fixed on branch a long time ago:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2de6af22acf3

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with testcase in comment 0 with the following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090522 Minefield/3.6a1pre ID:20090522133810

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090522 Shiretoko/3.5pre ID:20090522153422