479740 - TM: Crash [@ TraceRecorder::test_property_cache]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

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

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

Comment 1

[Leon Sha]

Attachment: patch

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.

Comment 2

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

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

Comment 4

[Leon Sha]

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.

Comment 5

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

(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

[Andreas Gal :gal]

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.

Comment 7

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

-> WFM then.

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Brendan Eich [:brendan]]

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

[Andreas Gal :gal]

Isn't this a dup of 457065?

Comment 11

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

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.

Comment 12

[Bob Clary [:bc] (inactive)]

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

Comment 13

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

(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.

Comment 14

[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