Last Comment Bug 514999 - TM: "Assertion failure: thing, at ../jsgc.cpp"
: TM: "Assertion failure: thing, at ../jsgc.cpp"
Status: VERIFIED FIXED
: fixed-in-tracemonkey [sg:critical]
: assertion, regression, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: x86 Mac OS X
: P1 critical (vote)
: ---
Assigned To: Andreas Gal :gal
: general
:
: 513981
: jsfunfuzz
  Show dependency treegraph
 
Reported: 2009-09-06 23:04 PDT by Gary Kwong [:nth10sd]
Modified: 2010-01-25 00:06 PST (History)
13 users (show)
sayrer: blocking1.9.2+
dveditz: wanted1.9.0.x-
bclary: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  1.0+
  ---
  ---
  ---
  beta1-fixed
  .6+
  .6-fixed


Attachments
patch (2.73 KB, patch)
2009-09-07 00:18 PDT, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review
patch (11.70 KB, patch)
2009-09-09 17:23 PDT, Andreas Gal :gal
mrbkap: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
patch for 1.9.1 (9.15 KB, patch)
2009-11-04 14:31 PST, Andreas Gal :gal
mrbkap: review+
Details | Diff | Splinter Review

Summon comment box

Description Gary Kwong [:nth10sd] 2009-09-06 23:04:02 PDT
(function () {
    (eval("\
        (function () {\
            for (var y = 0; y < 16; ++y) {\
                if (y % 3 == 2) {\
                    gczeal(1);\
                } else {\
                    print(0 / 0);\
                }\
            }\
        });\
    "))()
})();

asserts js debug shell with -j at Assertion failure: thing, at ../jsgc.cpp:2610

Brendan says (and I confirmed) that this is most probably related to bug 513981. Security-sensitive since bug 513981 is locked too.
Comment 1 Gary Kwong [:nth10sd] 2009-09-06 23:04:39 PDT
TM branch tip too.
Comment 2 Andreas Gal :gal 2009-09-06 23:09:54 PDT
Awesome fuzzer work Gary. This is really bad news. Its very likely 513981 as you suspected.
Comment 3 Andreas Gal :gal 2009-09-06 23:17:33 PDT
It doesn't crash if I use TMFLAGS=full. This is going to be fun.
Comment 4 Brendan Eich [:brendan] 2009-09-06 23:22:53 PDT
(In reply to comment #2)
> Awesome fuzzer work Gary. This is really bad news. Its very likely 513981 as
> you suspected.

As *I* suspect :-P. See bug 514819 comment 28.

/be
Comment 5 Gary Kwong [:nth10sd] 2009-09-06 23:26:02 PDT
(In reply to comment #0)
> Brendan says (and I confirmed) that this is most probably related to bug
> 513981. Security-sensitive since bug 513981 is locked too.

(In reply to comment #4)
> (In reply to comment #2)
> > Awesome fuzzer work Gary. This is really bad news. Its very likely 513981 as
> > you suspected.
> 
> As *I* suspect :-P. See bug 514819 comment 28.
> 
> /be

Yeah, I (or autoBisect) only get partial credit, full credit goes out to Brendan. ;-)
Comment 6 Andreas Gal :gal 2009-09-07 00:18:20 PDT
Created attachment 399034 [details] [review]
patch
Comment 7 Andreas Gal :gal 2009-09-07 00:20:33 PDT
I concur with blocking and this is also needed for the 3.5 branch.
Comment 8 Andreas Gal :gal 2009-09-07 00:41:35 PDT
sayrer, this and the other patch should go on trunk to make asap (assuming I get review for this and tryserver doesn't hate me)
Comment 9 Andreas Gal :gal 2009-09-07 00:41:42 PDT
bake, not make
Comment 10 Andreas Gal :gal 2009-09-07 12:31:58 PDT
I am getting a trace-test failure with the patch. Investigating.
Comment 11 Lucas Adamski 2009-09-08 21:38:39 PDT
This is not a security issue per se tho right?  Trying to figure out sg: rating.
Comment 12 Blake Kaplan (:mrbkap) 2009-09-09 01:09:26 PDT
I'd rate this as sg:critical. At the very least, if this particular bug is checked in as-is, it leaves a sg:critical bug in its wake.
Comment 13 Andreas Gal :gal 2009-09-09 17:23:07 PDT
Created attachment 399641 [details] [review]
patch

We can re-enter the interpreter while an outer use of nativeVp is still active, so we have to move nativeVp from cx to InterpState (debugged by Blake).
Comment 14 Blake Kaplan (:mrbkap) 2009-09-09 17:34:48 PDT
Comment on attachment 399641 [details] [review]
patch

Looks good.
Comment 15 Andreas Gal :gal 2009-09-09 17:38:35 PDT
http://hg.mozilla.org/tracemonkey/rev/2bcc3e339a63
Comment 16 Graydon Hoare :graydon 2009-09-09 20:36:03 PDT
Busted trace tests, gczeal is not defined in opt build. Fixing.
Comment 17 Graydon Hoare :graydon 2009-09-09 22:50:19 PDT
http://hg.mozilla.org/tracemonkey/rev/dad8ab8cb1dd should have fixed it, back in a bit to confirm.
Comment 18 Andreas Gal :gal 2009-09-09 23:01:54 PDT
Thanks graydon.
Comment 19 Daniel Veditz 2009-09-10 11:27:38 PDT
We'll switch this to blocking the next specific 1.9.1.x after it lands on trunk
and 1.9.2 successfully.
Comment 20 Blake Kaplan (:mrbkap) 2009-09-16 17:33:00 PDT
http://hg.mozilla.org/mozilla-central/rev/2bcc3e339a63
Comment 21 Blake Kaplan (:mrbkap) 2009-09-16 17:34:25 PDT
http://hg.mozilla.org/mozilla-central/rev/dad8ab8cb1dd
Comment 23 Bob Clary [:bc:] 2009-10-08 17:33:03 PDT
js/src/trace-test/tests/basic/testNativeArgsRooting.js
Comment 24 Bob Clary [:bc:] 2009-10-15 09:10:47 PDT
v 1.9.3, 1.9.2
Comment 25 Daniel Veditz 2009-10-16 11:11:47 PDT
Comment on attachment 399641 [details] [review]
patch

Approved for 1.9.1.5, a=dveditz for release-drivers
Comment 26 Robert Sayre 2009-11-04 09:23:02 PST
andreas, this doesn't apply to 1.9.1, can you refresh it?
Comment 27 Andreas Gal :gal 2009-11-04 11:17:44 PST
Looking.
Comment 28 Andreas Gal :gal 2009-11-04 14:01:29 PST
I am going to have to drop parts of the patch that fix the problem for getters/setters (since thats not on 191). If we ever take the getters/setters patch on top of this patch, we will lose those changes, create a gc hazard, and shoot ourselves in the foot. Fair warning.
Comment 29 Andreas Gal :gal 2009-11-04 14:03:29 PST
Actually 1.9.1 looks quite a bit different than what we tested with. I am not sure I am comfortable dropping this into a release based on my manual rebasing only.
Comment 30 Andreas Gal :gal 2009-11-04 14:06:35 PST
Ok no wonder this doesn't work. 513981 is missing in between.
Comment 31 Andreas Gal :gal 2009-11-04 14:31:14 PST
Ok on top of 513981 this looks manageable. Warning about getter/setter patch still applies. Patch in a sec.
Comment 32 Andreas Gal :gal 2009-11-04 14:31:42 PST
Created attachment 410351 [details] [review]
patch for 1.9.1
Comment 33 Andreas Gal :gal 2009-11-08 21:27:38 PST
Blake, could you land this for me?
Comment 35 Al Billings [:abillings] 2009-11-20 11:26:55 PST
Bob, can you verify this on the 1.9.1 nightly?
Comment 36 Bob Clary [:bc:] 2009-11-23 11:38:54 PST
v 1.9.1, testcase in comment 0 does not assert on 1.9.1 mac debug shell.

Note You need to log in before you can comment on or make changes to this bug.