Closed Bug 479740 Opened 12 years ago Closed 12 years ago

TM: Crash [@ TraceRecorder::test_property_cache]

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: gkw, Unassigned)

References

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(1 file)

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?
Attached patch patchSplinter Review
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)
OS: Mac OS X → All
Hardware: x86 → All
Attachment #363847 - Attachment is patch: true
Attachment #363847 - Attachment mime type: application/octet-stream → text/plain
Attachment #363847 - Flags: review?(gal) → review?(brendan)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attachment #363847 - Flags: review?(brendan) → review-
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
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.
(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.
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.
-> WFM then.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME
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 → ---
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
Isn't this a dup of 457065?
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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+
(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
Keywords: fixed1.9.1
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
Target Milestone: --- → mozilla1.9.2a1
Crash Signature: [@ TraceRecorder::test_property_cache]
You need to log in before you can comment on or make changes to this bug.