488693 - TM: "Assertion failure: !JS_TRACE_MONITOR(cx).needFlush, at ../jstracer.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(new Function("for (var x = 0; x < 2; ++x) { gczeal(2)} "))()

asserts dbg js shell with -j at Assertion failure: !JS_TRACE_MONITOR(cx).needFlush, at ../jstracer.cpp:4482

This is occurring very often with jsfunfuzz.

autoBisect shows this is probably related to bug 487845 :

The first bad revision is:
changeset:   27192:4c157cfe2289
user:        Jason Orendorff
date:        Tue Apr 14 08:45:37 2009 -0500
summary:     Bug 487845 - TM: After deep-bailing, we can lirbuf->rewind() and then return to a dead code page. r=gal.

Comment 1

[Bob Clary [:bc] (inactive)]

also in the browser only:
js1_5/extensions/regress-350531.js
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5%2Fextensions%2Fregress-350531.js;language=type;text/javascript>

js1_7/extensions/regress-458288.js

js1_8/regress/regress-464418.js
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_8%2Fregress%2Fregress-464418.js;language=type;text/javascript>

Comment 2

[Jason Orendorff [:jorendorff]]

It is definitely due to the patch fingered in comment 0.  There are two possibly easy fixes.

1. delete the assertion, since it's new and apparently bogus (afaik we didn't crash due to recording when the JIT cache is needFlush before)

2. after GC (that is, in js_GC and also when entering a request, since GC may have happened) if needFlush is set, purge the cache

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 4

[Andreas Gal :gal]

Comment on attachment 373223 [details] [diff] [review]
v1

Looks good. We have some test coverage for cache flushing. As long tinderboxes are happy we should be good.

Comment 5

[Jason Orendorff [:jorendorff]]

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

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v1 to v2

v1 wasn't flushing the JIT cache in all cases where js_CheckGlobalObjectShape returns false.

v2 fixes that but also takes the opportunity to make a few more changes. Note in particular the last hunk. v1 inserted that "Even if there is a mismatch we can start recording" comment. I now claim that it's just a bug: we must give up recording there, because globalShape and globalSlots may not have been populated.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 8

[Andreas Gal :gal]

Comment on attachment 373377 [details] [diff] [review]
v2

Did you check for perf impact? (SS shell)

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #5)
> http://hg.mozilla.org/tracemonkey/rev/a6071b1aa626

caused js1_7/regress/regress-464403.js to Assertion failure: !tm->recorder, at ../jstracer.cpp:4395

Comment 10

[Andreas Gal :gal]

js1_7/regress/regress-464403.js works with TM tip for me.

Comment 11

[Bob Clary [:bc] (inactive)]

the failure went away when it was backed out by:

http://hg.mozilla.org/tracemonkey/rev/a9e5683faba0
http://hg.mozilla.org/tracemonkey/rev/02918f4d3bcd

the new patch http://hg.mozilla.org/tracemonkey/rev/34479ba0e4fb is different. I brought it up so the new patch could be tested to make sure it didn't have the same assertion. I don't see the assertion with tracemonkey tip now either.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/34479ba0e4fb

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929