Closed Bug 635594 Opened 9 years ago Closed 9 years ago

Assertion failure: !tm->recorder, at jstracer.cpp:7131

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: nikosverschore)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

The following code asserts on TM tip (run with -j only):

test();
function f() {
  for ( ; ; test++ ) {
  }
}
function test()
{
  var src = "return f(" +Array(10*1000).join("0,")+"Math.atan2());";
  var result = new Function(src)();
}


Not blocking for now because this does not crash for me when passing through assertions and it does not work with -m.
Assignee: general → wmccloskey
OS: Linux → All
Hardware: x86_64 → All
Attached patch patch (obsolete) — Splinter Review
This has been around for a while. I checked an October 14th build, and it still happens.

The code in RecordLoopEdge looks like this:

    if (tm->recorder) {
        if (pc == tm->recorder->tree->ip) {
            tm->recorder->closeLoop();
        } else {
            ...
        }
    }
    JS_ASSERT(!tm->recorder);

So the code assumes that closeLoop will finish recording. However, there are a few paths where it doesn't: compile() can fail or we can leave early on a |callDepth != 0| check.

If this happens (and we're in an opt build where we don't assert), the remainder of RecordLoopEdge looks up the loop and decides whether to record or execute a trace. In this example, nothing bad happens. But I think we should probably fix this. If we exit closeLoop in a different way, it seems like we might be able to continue recording and eventually compile that into a trace at some later time. If we try to execute that trace, bad stuff will happen.

The attached patch just makes sure we never have a recorder by aborting if we do.
Attachment #514272 - Flags: review?(lw)
I've also been busy with this bug report (apparently a little bit to late ;) ) and I've come to the same conclusion as #comment 1. Also the patches are alike, but checks on the return value of the closeLoop function and return on error (instead of doing further). I don't know which patch to prefer.
Attachment #514272 - Flags: review?(lw) → review+
Comment on attachment 514272 [details] [diff] [review]
patch

kosver's patch looks better. Should we take either one?
Attachment #514272 - Flags: approval2.0?
Attachment #514272 - Flags: approval2.0? → approval2.0+
Comment on attachment 514312 [details] [diff] [review]
alternative patch

This was the patch that was checked-in, bringing over r+ and hoping someone will symbolically set back approval2.0+.
Attachment #514312 - Flags: review+
Attachment #514312 - Flags: approval2.0?
Comment on attachment 514272 [details] [diff] [review]
patch

This patch was obsoleted.
Attachment #514272 - Attachment is obsolete: true
date && time ./autoBisect.py -p -j -a 32 -e 0c398bad4c99 -i /home/fuzz1/Desktop/635594.js /home/fuzz1/fuzzing/jsfunfuzz/jsunhappy.py 8 20 /home/fuzz1/fuzzing/js-known/mozilla-central/ && date

As a retroactive note,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   36380:8e2153c50ecd
user:        David Anderson
date:        Tue Dec 08 11:15:34 2009 -0800
summary:     Fixed loops ending in JSOP_GOTO not compiling properly (bug 533042, r=gal).
Blocks: 533042
Note for future ref: hg qref -u"user name <email>" -m"..." to credit fine folks such as kosver.

/be
Also assigning to kosver..
Assignee: wmccloskey → nikosverschore
Attachment #514312 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/4586c38a6a6e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 676763
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.