483940 - TM: trace through val[obj] (both setting and getting) rather than aborting

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P2)

Description

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

for (var y = 0; y < 2; ++y) { (/x/)[this] }

asserts at Assertion failure: JSVAL_IS_NULL(v), at ../jstracer.cpp:4952 with -j, seems to work as expected without -j, and in opt.

autoBisect shows bug 478525, http://hg.mozilla.org/tracemonkey/rev/f449fe8bd097 may be related:

The first bad revision is:
changeset:   26145:f449fe8bd097
parent:      26142:33c5c42a29c7
user:        Andreas Gal
date:        Tue Mar 17 15:39:42 2009 -0700
summary:     Try harder to trace array access with non-int / non-string index (478525, r=brendan).

Comment 1

[Jeff Walden [:Waldo]]

Attachment: A start

This will trace this:

var o = {toString: function() { return 0; } };
for (var y =0; y < 3; y++)
  print("" + [1][o]);

but not this:

var o = {toString: function() { return "0"; } };
for (var y =0; y < 3; y++)
  print("" + [1][o]);

Recursion prevention in imacros bites here, not quite sure what to do about that yet -- tail-recursive imacros?

On a last note, using |this| as the index will abort with this patch because of the |getprop toString| within the getobjprop imacro invocation -- gotta use a different object with a custom toString (for now, until JSFastNative can be traced) to meaningfully trace into the imacro.

Comment 2

[Andreas Gal :gal]

484120 stops the crash. This bug needs to improve performance for this case.

Comment 3

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

for (var z = 0; z < 2; ++z) { ({ 0.1: null })}

is another similar-looking testcase that asserts at Assertion failure: !JSVAL_IS_PRIMITIVE(v), at ../source/jstracer.cpp:8054 instead and has an identical regression window. (debug with -j)

Comment 4

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

(In reply to comment #3)
> for (var z = 0; z < 2; ++z) { ({ 0.1: null })}
> 
> is another similar-looking testcase that asserts at Assertion failure:
> !JSVAL_IS_PRIMITIVE(v), at ../source/jstracer.cpp:8054 instead and has an
> identical regression window. (debug with -j)

The testcase in comment #0 has been fixed by the patch in bug 484120 per comment #2, but this testcase was not. Spun off as bug 484151.

Comment 5

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

Should this be marked fixed-in-tracemonkey as the patch in bug 484120 fixed this?

Comment 6

[Robert Sayre]

(In reply to comment #5)
> Should this be marked fixed-in-tracemonkey as the patch in bug 484120 fixed
> this?

Waldo?

Comment 7

[Jeff Walden [:Waldo]]

No; bug 484120 changed the code so that we abort rather than hit the assertion, but we still should not abort and actually handle this case correctly -- which the patch here semi-but-not-completely fixes.  Need to update and see about wrapping it up still...

Comment 8

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

x = this
for(let y in ['', 0]) (/x/g [[]]) = x;

asserts identically as comment #0 with TM tip with -j in debug; I don't know if they're the same cause.

Will autoBisect later...

Comment 9

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

(In reply to comment #8)
> Will autoBisect later...

As in comment #0 for that testcase, this is also related to the same bug 478525.

Comment 10

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

Bug morphing-madness - now the testcase in comment #0 asserts differently:

$ ./js-dbg-tm-intelmac -j
js> for (var y = 0; y < 2; ++y) { (/x/)[this] }
Assertion failure: initializing || known(p), at ../jstracer.cpp:1954

while the testcases in comment #3 and comment #8 seem to work as expected(i.e. they no longer assert).

Can this bug please have an update esp. since we're so near code freeze - I wouldn't like another game of testcase-morphing-fun again..

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #10)
> Bug morphing-madness - now the testcase in comment #0 asserts differently:
> 
> $ ./js-dbg-tm-intelmac -j
> js> for (var y = 0; y < 2; ++y) { (/x/)[this] }
> Assertion failure: initializing || known(p), at ../jstracer.cpp:1954

see bug 488203 comment 14

Comment 12

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

(In reply to comment #11)
> (In reply to comment #10)
> > Bug morphing-madness - now the testcase in comment #0 asserts differently:
> > 
> > $ ./js-dbg-tm-intelmac -j
> > js> for (var y = 0; y < 2; ++y) { (/x/)[this] }
> > Assertion failure: initializing || known(p), at ../jstracer.cpp:1954
> 
> see bug 488203 comment 14

? I'm on TM tip, which has the patch that supposedly fixes that - http://hg.mozilla.org/tracemonkey/rev/e8c23c42db7f - see bug 488203 comment 18, or am I wrong?

Comment 13

[Bob Clary [:bc] (inactive)]

I don't think it fixed it.

Comment 14

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

(In reply to comment #10)
> Bug morphing-madness - now the testcase in comment #0 asserts differently:

Now comment #0 has morphed _again_, it's now crashing dbg builds at TraceRecorder::getThis at a possibly exploitable 0xfffffffc.

I'm not sure to security-fy this or not because it's in debug only, doesn't seem to crash opt.

Comment 15

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

(In reply to comment #14)
> (In reply to comment #10)
> > Bug morphing-madness - now the testcase in comment #0 asserts differently:
> 
> Now comment #0 has morphed _again_, it's now crashing dbg builds at
> TraceRecorder::getThis at a possibly exploitable 0xfffffffc.
> 
> I'm not sure to security-fy this or not because it's in debug only, doesn't
> seem to crash opt.

Waldo suggests I file a new bug, so spun off as bug 489040.

Comment 17

[Jeff Walden [:Waldo]]

I think this work has been superseded by other work elsewhere.