591418 - JM: "Assertion failure: fp->argv[-1] == fp->getThisValue(),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function() {
  for each(let z in [0, 0, 0, 0]) {
    eval("\
      for(var a = 0; a < 1; ++a) {\
        this\
      }\
    ")
  }
})()

asserts js debug shell on JM changeset 33b05dd43cd4 with -j at Assertion failure: fp->argv[-1] == fp->getThisValue(), at ../jstracer.cpp:10090

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Chris, is it true that argv[-1] should == fp->thisv? Do they get updated in sync when we compute |this|?

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

I think the patch in bug 587809 fixes it.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Wait... dmandelin said he landed that already.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

thisv is |null| and argv[-1] is the global object -- I have to look into where recording of the JSOP_THIS occurs with respect to the interpreter executing it, because a |null| thisv indicates lazy compute-and-set of argv[-1].

Comment 5

[Jesse Ruderman]

(In reply to comment #2)
> I think the patch in bug 587809 fixes it.

autobisect tells me the opposite: that the patch in bug 587809 caused it.

The first bad revision is:
changeset:   50705:305b00ec07e7
user:        Chris Leary <[e-mail]>
date:        Wed Aug 18 18:17:30 2010 -0700
summary:     [JAEGER] Bug 587809 pre-landing: land tracer changes for eager this so we can see what it does with X64 on tinderbox

Comment 6

[David Mandelin [:dmandelin]]

This comment in jsinterp.h suggest that the assert is bogus here:

     * Usually if argv != NULL then thisv == argv[-1], but natives may
     * assign to argv[-1]. Also, obj_eval can trigger a special case
     * where two stack frames have the same argv. If one of the frames fills
     * in both argv[-1] and thisv, the other frame's thisv is left null.

The assertion is:

    JS_ASSERT_IF(fp->argv, fp->argv[-1] == fp->getThisValue());

The comment suggests that when eval is in play, it is OK for fp->getThisValue() to return NULL. 

Chris, what do you think? Should we just:

    JS_ASSERT_IF(fp->argv && !fp->getThisValue().isNull, 
                 fp->argv[-1] == fp->getThisValue());

Or should we be looking for fp->argv == fp->down->argv or some such?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

dbaron hit this today - Chris, had a chance to see comment #6?

Comment 9

[Luke Wagner [:luke]]

I see someone else has already removed the presumed-bogus assert.  I'd dup the bug, but its hard to find when a line was deleted using hg annotate.

Comment 10

[Jesse Ruderman]

Yes, looks like *someone* already removed the bogus assert ;)  hg grep ftw.

hg cat -r 33b05dd43cd4 jstracer.cpp | edit

line 10090 is
JS_ASSERT_IF(fp->argv, fp->argv[-1] == fp->getThisValue());

hg grep --all "JS_ASSERT_IF\(fp" jstracer.cpp

changeset:   8721b595e7ab
user:        Luke Wagner
date:        Mon Aug 09 22:43:33 2010 -0700
summary:     Bug 539144 - Make formal args a jit-time const offset from fp; rm argv/argc/thisv/script/callobj (r=brendan,dvander)

Comment 11

[Luke Wagner [:luke]]

Hah.  Yeah, that assertion totally had it coming.

Comment 12

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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