Closed
Bug 479740
Opened 16 years ago
Closed 16 years ago
TM: Crash [@ TraceRecorder::test_property_cache]
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: gkw, Unassigned)
References
Details
(Keywords: crash, testcase, verified1.9.1)
Crash Data
Attachments
(1 file)
452 bytes,
patch
|
brendan
:
review-
|
Details | Diff | Splinter Review |
function f() {
for ( foobar in (function() {
for (var x = 0; x < 2; ++x) {
if (x % 2 == 1) { yield (this.z.z) = function(){} }
eval("this", false);
}
})())
function(){}
}
f();
crashes both js opt and dbg shells with -j at TraceRecorder::test_property_cache near null. Seems to work as expected without -j.
Not sure if this warrants blocking1.9.1 but nominating just-in-case.
===
Full verbose output:
$ ./js-dbg-tm-intelmac -j
js> function f() {
for ( foobar in (function() {
for (var x = 0; x < 2; ++x) {
if (x % 2 == 1) { yield (this.z.z) = function(){} }
eval("this", false);
}
})())
function(){}
}
Flushing fragments for JSScript 0x30ef30.
js> f();
capture stack type callee0: 0=O
capture stack type this0: 0=O
capture stack type vars0: 1=I
recording starting from typein:4@9
globalObj=0x29f000, shape=184
import vp=0x30ec80 name=$callee0 type=object flags=0
import vp=0x30ec84 name=$this0 type=object flags=0
import vp=0x30ec88 name=$<anonymous>.x type=int flags=0
start
state = param 0 ecx
0
sp = ld state[0]
4
rp = ld state[4]
12
cx = ld state[12]
8
gp = ld state[8]
16
eos = ld state[16]
20
eor = ld state[20]
40
globalObj = ld state[40]
-24
$callee0 = ld sp[-24]
-16
$this0 = ld sp[-16]
-8
ld1 = ld sp[-8]
$<anonymous>.x = i2f ld1
ld2 = ld cx[0]
eq1 = eq ld2, 0
xf1: xf eq1 -> pc=0x30ee65 imacpc=0x0 sp+0 rp+0
00009: 4 getlocal 0
00012: 4 int8 2
00014: 4 mod
sti sp[0] = ld1
#40000000:0 /* 2 */
2
sti sp[8] = 2
js_imod1 = js_imod ( ld1 2 )
-1
eq2 = eq js_imod1, -1
xt1: xt eq2 -> pc=0x30ee6a imacpc=0x0 sp+16 rp+0
00015: 4 one
00016: 4 eq
i2f1 = i2f js_imod1
sti sp[0] = js_imod1
#3FF00000:0 /* 1 */
1
sti sp[8] = 1
eq3 = eq js_imod1, 1
xf2: xf eq3 -> pc=0x30ee6c imacpc=0x0 sp+16 rp+0
00020: 4 getthisprop "z"
sti sp[0] = eq3
eq4 = eq $this0, 0
xt2: xt eq4 -> pc=0x30ee70 imacpc=0x0 sp+0 rp+0
eq5 = eq $this0, globalObj
xt3: xt eq5 -> pc=0x30ee70 imacpc=0x0 sp+0 rp+0
Bus error
Flags: blocking1.9.1?
The comments above the return is not correct. getThis() return false when cx->fp->argv[-1] is null. Normally cx->fp->argv[-1] is the same as cx->fp->thisp. But there are still possibility that cx->fp->thisp is not exactly cx->fp->argv[-1]. So we still need to check cx->fp->thisp here.
Attachment #363847 -
Flags: review?(gal)
Updated•16 years ago
|
Attachment #363847 -
Attachment is patch: true
Attachment #363847 -
Attachment mime type: application/octet-stream → text/plain
Updated•16 years ago
|
Attachment #363847 -
Flags: review?(gal) → review?(brendan)
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Updated•16 years ago
|
Attachment #363847 -
Flags: review?(brendan) → review-
Comment 2•16 years ago
|
||
Comment on attachment 363847 [details] [diff] [review]
patch
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -9112,9 +9112,7 @@
> {
> LIns* this_ins;
>
>- /* its safe to just use cx->fp->thisp here because getThis() returns false if thisp
>- is not available */
>- return getThis(this_ins) && getProp(cx->fp->thisp, this_ins);
>+ return getThis(this_ins) && cx->fp->thisp && getProp(cx->fp->thisp, this_ins);
This isn't right, we need to call js_ComputeThis under TraceRecorder::getThis if !(cx->fp->flags & JSFRAME_COMPUTED_THIS). See the COMPUTE_THIS macro in jsinterp.cpp.
/be
Comment 3•16 years ago
|
||
Leon, thanks for the patch (I didn't mean to sound ungrateful). If getThis calls js_ComputeThis, then I think the bug should be fixed, although more testing would be needed. Do you want to take this bug and patch it? It's a blocker and it needs to be assigned. If not, sayrer can find an owner.
/be
I can not see this bug in the latest tracemonkey tree and mozilla-central. The changeset http://hg.mozilla.org/tracemonkey/rev/5c7fe2d7c24d avoids the crash. Maybe the problem is just hidden by this check-in. Please assign this to other people or just close this bug.
Reporter | ||
Comment 5•16 years ago
|
||
(In reply to comment #4)
> I can not see this bug in the latest tracemonkey tree and mozilla-central. The
> changeset http://hg.mozilla.org/tracemonkey/rev/5c7fe2d7c24d avoids the crash.
> Maybe the problem is just hidden by this check-in. Please assign this to other
> people or just close this bug.
Confirming WFM - I don't know whether that patch indeed fixes this bug, thus leaving resolution to someone else who might know.
Comment 6•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/5c7fe2d7c24d is not a fix. It might have change the behavior sufficiently to not trigger the problem in this particular testcase.
Reporter | ||
Comment 7•16 years ago
|
||
-> WFM then.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME
Comment 8•16 years ago
|
||
Please read bug 452498 comment 123, where I cited one of your tests and wrote
> Requires -j:
> =====
> (function(){ eval("for (x in ['', {}, '', {}]) { this; }" )})()
>
> Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at
> ../jstracer.cpp:4172
I think this is bug 479740.
Andreas, could you field this one? Should be easy to fix via js_ComputeThis, so long as that being called at recording time makes all the trace-execution-time assumptions true.
/be
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 9•16 years ago
|
||
Oops, sorry about that -- should have read ahead in bugmail before commenting. I still do not like marking a blocker WFM, especially if the patch that seemed to "fix" the bug didn't do anything related to the crash site, by any theory of the crash we can think of.
But I can't think of a better way to resolve this bug. Can we leave it open for a bit in case the symptom reappears?
/be
Comment 10•16 years ago
|
||
Isn't this a dup of 457065?
Reporter | ||
Comment 11•16 years ago
|
||
This seems to have been fixed by:
changeset: 25410:094c2e9de304
user: Andreas Gal
date: Wed Feb 25 18:47:22 2009 -0800
summary: Improve blacklisting (479109, r=graydon).
I haven't yet verified this, I was using the script(s) created in bug 482536, and haven't tested the script fully yet.
Updated•16 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 12•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/10f3c054d958
/cvsroot/mozilla/js/tests/js1_8/regress/regress-479740.js,v <-- regress-479740.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
Reporter | ||
Comment 13•16 years ago
|
||
(In reply to comment #11)
> This seems to have been fixed by:
>
> changeset: 25410:094c2e9de304
> user: Andreas Gal
> date: Wed Feb 25 18:47:22 2009 -0800
> summary: Improve blacklisting (479109, r=graydon).
>
> I haven't yet verified this, I was using the script(s) created in bug 482536,
> and haven't tested the script fully yet.
I have manually verified that the fix in bug 479109 does indeed fix this issue as well.
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 14•16 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: --- → mozilla1.9.2a1
Updated•14 years ago
|
Crash Signature: [@ TraceRecorder::test_property_cache]
You need to log in
before you can comment on or make changes to this bug.
Description
•