620335 - TM: "Assertion failure: !argsobj.getPrivate(),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

This test case:
---
function f() {
    "use strict";
    eval("");
    var a = 0;
    for (var i = 0; i < 20; i++) {
        a--;
    }
}
f();
---
Asserts with -j: 
Assertion failure: !argsobj.getPrivate(), at ../jsfun.cpp:272

Comment 1

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

Assertions should be critical.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: tentative patch

The cause here seems obvious -- js_PutArgsObject() isn't allowing for the fact that the arguments object may be strict.

The attached patch is my guess at a fix.  It does the obvious thing of allowing the arguments object to be normal or strict.  I fully admit I don't know what I'm doing, though.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 498939 [details] [diff] [review]
tentative patch

Ok, ignore that patch, it's completely bogus.

Comment 4

[Nicholas Nethercote [inactive]]

Waldo and I determined that this revision is the culprit:

  changeset:   53418:8721b595e7ab
  user:        Luke Wagner <[e-mail]>
  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)

Strict mode arguments objects shouldn't have their private slot set.  StrictArgumentsClass does has JSCLASS_HAS_PRIVATE, but it's meant to be always NULL and that's all that it's checked for.

However, the private slot gets set here:

#0 JSObject::setPrivate (this=0x7ffff0e10058, data=0x7ffff11010b0) at ../jsobj.h:722
#1 0x00000000005ee616 in js::FlushNativeStackFrameVisitor::visitFrameObjPtr (this=0x7fffffffc0e0, p=0x7ffff11010c0, fp=0x7ffff11010b0) at ../jstracer.cpp:3059
#2 0x00000000005e351f in js::VisitFrameSlots<js::FlushNativeStackFrameVisitor> (visitor=..., cx=0xa901a0, depth=0, fp=0x7ffff11010b0, next=0x0) at ../jstracer.cpp:1806
#3 0x00000000005e2863 in js::VisitStackSlots<js::FlushNativeStackFrameVisitor> (visitor=..., cx=0xa901a0, callDepth=0) at ../jstracer.cpp:1826
#4 0x00000000005af708 in js::FlushNativeStackFrame (cx=0xa901a0, callDepth=0, mp=0xada540, np=0xa754b0) at ../jstracer.cpp:3415

This shouldn't happen.  I'm not sure what the fix is, perhaps Luke or Waldo
can work it out from here?

I do know that it would be nice if we could remove JSCLASS_HAS_PRIVATE from
StrictArgumentsClass so we can't accidentally set it.  To do that would
require removing the |JS_ASSERT(!argsobj.getPrivate());| assertion from
js_PutArgsObject().  stubs::GetElem() also can call getPrivate() on strict
mode arguments objects, but this looks like a NULL test that could be
replaced with something similar.

Also, the comments around jsobj.h:729 should be updated to explain that
strict arguments objects don't use the private slot (and any other
differences between strict and non-strict arguments should be explained
there).

Comment 5

[Jeff Walden [:Waldo]]

Seems maybe that setPrivate call should be guarded by |if (frameobj->isNormalArguments())|, probably also add in a |else assert(isStrict)| too.  But I don't fully understand what the stack-frame-visiting pattern implemented here does, so conceivably that's not actually right.

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #4)
> Waldo and I determined that this revision is the culprit:
> 
>   changeset:   53418:8721b595e7ab
>   user:        Luke Wagner <[e-mail]>
>   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)

Heh, only insofar as it didn't predict the future (this patch precedes strict args) ;-)

Comment 7

[Jeff Walden [:Waldo]]

Well, not really.  hg revision dates being useless strike again!

http://hg.mozilla.org/mozilla-central/rev/76f962d07ce0
http://hg.mozilla.org/mozilla-central/rev/8721b595e7ab

Comment 8

[Jeff Walden [:Waldo]]

Er, strikes.

Comment 9

[David Mandelin [:dmandelin]]

Jeff, Nick, how serious is this? Can it cause crashes or security issues?

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> Jeff, Nick, how serious is this? Can it cause crashes or security issues?

I'd guess no, but I don't really know.

Comment 11

[Jeff Walden [:Waldo]]

I'm not sure.  It depends on whether we have places that grab out a frame pointer from strict mode arguments and then that affects control flow.  I don't know if such places exist.  They shouldn't.  But then again, this situation shouldn't have existed either.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> I'm not sure.  It depends on whether we have places that grab out a frame
> pointer from strict mode arguments and then that affects control flow.  I don't
> know if such places exist.  They shouldn't.  But then again, this situation
> shouldn't have existed either.

This code in stubs::GetElem() is a concern:

        } else if (obj->isArguments()) {
            uint32 arg = uint32(i);

            if (arg < obj->getArgsInitialLength()) {
                copyFrom = obj->addressOfArgsElement(arg);
                if (!copyFrom->isMagic()) {
                    if (JSStackFrame *afp = (JSStackFrame *) obj->getPrivate())
                        copyFrom = &afp->canonicalActualArg(arg);
                    goto end_getelem;
                }
            }
        }

Comment 13

[David Mandelin [:dmandelin]]

Marking blocking for now, but don't hesitate to suggest unblocking if you decide it's not important.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Patch and test per comment 5's understanding

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> 
> I do know that it would be nice if we could remove JSCLASS_HAS_PRIVATE from
> StrictArgumentsClass so we can't accidentally set it.  To do that would
> require removing the |JS_ASSERT(!argsobj.getPrivate());| assertion from
> js_PutArgsObject().  stubs::GetElem() also can call getPrivate() on strict
> mode arguments objects, but this looks like a NULL test that could be
> replaced with something similar.

I can live without this...
 
> Also, the comments around jsobj.h:729 should be updated to explain that
> strict arguments objects don't use the private slot (and any other
> differences between strict and non-strict arguments should be explained
> there).

...but this is dead easy.  Waldo, can you add this to your patch before landing?  You admitted yourself on IRC that you should have done this earlier :)

Comment 16

[Jeff Walden [:Waldo]]

Er.  Right.  I was going to do that, wasn't I?  Will do before push.  If I forget, someone castigate me harshly even more!

Comment 17

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/37ebdb3e7617

...and the comment fixes:

http://hg.mozilla.org/tracemonkey/rev/25908114259b

Comment 18

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

http://hg.mozilla.org/mozilla-central/rev/37ebdb3e7617