Closed Bug 514819 Opened 10 years ago Closed 10 years ago

Crash [@ js_IsAboutToBeFinalized] or [@ js_GetGCThingTraceKind] with defineSetter, eval, for...in

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- unaffected

People

(Reporter: gkw, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [ccbr][sg:critical?] fixed-in-tracemonkey)

Crash Data

Attachments

(1 file, 3 obsolete files)

__defineSetter__("x", eval)
for (let c in [,0,,,,,,0,0,0,0]) x = c

crashes opt js shell on TM branch without -j at js_IsAboutToBeFinalized at a scary location 0x2000000a. (dbg shell at js_IsAboutToBeFinalized at 0x20000009)

autoBisect shows this is probably related to bug 513530:

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=6a7fc22fd61d&tochange=f074bd6d68d6

[ autoBisect sort-of screws up due to build failure on some intermediate changesets, but regression window is small enough to only pinpoint that bug. :) ]

===

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000002000000a
Crashed Thread:  0

Thread 0 Crashed:
0   js-opt-tm-darwin              	0x0004a6b2 js_IsAboutToBeFinalized + 34
1   js-opt-tm-darwin              	0x0001bd72 js_atom_sweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*) + 50
2   js-opt-tm-darwin              	0x00027fe7 JS_DHashTableEnumerate + 135
3   js-opt-tm-darwin              	0x0001bdcf js_SweepAtomState + 79
4   js-opt-tm-darwin              	0x0004c53c js_GC + 540
5   js-opt-tm-darwin              	0x0001df37 js_DestroyContext(JSContext*, JSDestroyContextMode) + 247
6   js-opt-tm-darwin              	0x0000d259 JS_DestroyContext + 25
7   js-opt-tm-darwin              	0x0000818a main + 922
8   js-opt-tm-darwin              	0x000026db _start + 209
9   js-opt-tm-darwin              	0x00002609 start + 41
Opening up since bug 513530 wasn't on any other branch except tracemonkey.
Group: core-security
Whiteboard: [ccbr]
Congrats Gregor. Your first GC hazard :) Want to debug?
I'm seeing debug shell crashes at js_GetGCThingTraceKind around the same regressionwindow timeframe too and am guessing they're the same bug, till proved otherwise.
Summary: Crash [@ js_IsAboutToBeFinalized] with defineSetter, eval, for...in → Crash [@ js_IsAboutToBeFinalized] or [@ js_GetGCThingTraceKind] with defineSetter, eval, for...in
Oh, the testcase in comment #0 also has to passed in to the shell as an argument (i.e. `./js testcase.js` ) in order to crash.
Whiteboard: [ccbr] → [ccbr][sg:critical?]
Assignee: general → gwagner
I've tried similar patches to the one for bug 513530, I should have remembered. You have to chase the callers of GetGCThingFlags{,OrNull} up the call graph and ensure that each call site either (a) rules out string type via a jsval tag or similar check; or (b) is protected by !JSString::isStatic((JSString *) thing).

We could just add (b) to GetGCThingFlags* but that would slow down a hot path. I recall going up the call graph one or two levels and finding a reduction in type so that not all calls needed the !isStatic test.

/be
Attached patch quick fix (obsolete) — Splinter Review
Seems that somebody wants to finalize my static strings :(
The "GC hazard" for IsAboutToBeFinalized should be fixed with this patch but I don't know yet if it really fixes the root of the problem.
Gary, do you also have a testcase for the GetGCThingTraceKind crash?
Comment on attachment 398870 [details] [diff] [review]
quick fix

Not a proper fix. IsAboutToBeFinalized is a hot path shared for objects and doubles too. We can put the string check in there.
Attachment #398870 - Flags: review-
this is crashing opt builds on the tinderbox as well
#7 meant can't. We should back out until this is fixed.
This is easy to fix. We should not be atomizing atomized strings. Patch in a few.

/be
I'd hacked similar patches so take blame for not remembering during review.

The only path hit here that could be called warm is js_AtomizeString, and it was already failing to test its str parameter's ATOMIZED flag, which meant we would intern int strings in the atom table (unit strings had been fixed to not go in the table). That was why we were calling js_FinalizeStringRT from the uninterner. A red flag.

js_AtomizeString should not reintern an already-atomized string, whether unit, int, or fully interned in the runtime's atom table.

/be
Assignee: gwagner → brendan
Status: NEW → ASSIGNED
Attachment #398880 - Flags: review?(gal)
Attachment #398870 - Attachment is obsolete: true
A bit more narration:

GetGCThingFlags{,OrNull} are static and called only in a relatively few places in jsgc.cpp. I inspected all callers. Some are for objects only. Others could be for strings but enumerating them and their callers shows that by not interning unit and int strings in the atom table, and checking JSString::isStatic in cool-path API entry points such as JS_GetExternalStringGCType, we can assert !isStatic in the bottom-most common code, namely THING_TO_ARENA. This macro is always called before THING_TO_INDEX. So it asserts !JSString::isStatic((JSString *) thing).

/be
Test in comment 0.

/be
Flags: in-testsuite?
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2
Attachment #398880 - Attachment is obsolete: true
Attachment #398881 - Flags: review?(gal)
Attachment #398880 - Flags: review?(gal)
Attachment #398881 - Flags: review?(gal) → review+
Comment on attachment 398881 [details] [diff] [review]
pick spacing and paren nits in revised THING_TO_ARENA

I don't much like the isAtomized early out in AtomizeString. I think that should be an assert and the caller should check. Looks good otherwise.
(In reply to comment #15)
> (From update of attachment 398881 [details] [diff] [review])
> I don't much like the isAtomized early out in AtomizeString. I think that
> should be an assert and the caller should check. Looks good otherwise.

As noted on iChat, asserting there and making all ten callers check is dopey, just code duplication for no demonstrated perf reason. The callers are all cool paths except for js_ValueToStringId, which checks str->isAtomized() precisely to avoid calling js_AtomizeString already.

/be
(In reply to comment #6)
> Created an attachment (id=398870) [details]
> quick fix
> 
> Seems that somebody wants to finalize my static strings :(
> The "GC hazard" for IsAboutToBeFinalized should be fixed with this patch but I
> don't know yet if it really fixes the root of the problem.
> Gary, do you also have a testcase for the GetGCThingTraceKind crash?

Yup.


''.*
for each(x in [0]) {
    gczeal(1)
}
[]


Pass this in as a CLI argument to a debug js shell and it will crash at js_GetGCThingTraceKind at 0x20000009.
I'd asked for the previous patch to be landed to fix the talos crashes but my plea was misunderstood -- oh well, better late than never. Good to have Gregor's patch to build on.

I'm totally relying on the compiler to optimize (2 <= length && length <= 3) and what follows.

/be
Attachment #398881 - Attachment is obsolete: true
Attachment #398905 - Flags: review?(gal)
Attachment #398905 - Flags: review?(gal) → review+
(In reply to comment #18)
> Created an attachment (id=398905) [details]
> I'm totally relying on the compiler to optimize (2 <= length && length <= 3)
> and what follows.

And even what comes before -- all the special casing before the slow patch in js_AtomizeString that depends on length could be in an "if (length <= 3) {...}" but I didn't want to over-do the hand-coded optimization.

Fixed on tm:

http://hg.mozilla.org/tracemonkey/rev/3be24656202c

/be
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?] fixed-in-tracemonkey
Depends on: 514884
(In reply to comment #18)
> Created an attachment (id=398905) [details]
> my final answer (builds on gwagner's fix)

Assertion with final answer, i.e. TM tip without -j:

gczeal(1)
for each(let x in [new Number(1)]) {
    print(x)
} []

Assertion failure: !JSString::isStatic((JSString *) (thing)), at ../jsgc.cpp:1184

Should I file a new bug? (It'll have to wait for me though - bedtime at EDT)
(In reply to comment #20)
> (In reply to comment #18)
> > Created an attachment (id=398905) [details] [details]
> > my final answer (builds on gwagner's fix)
> 
> Assertion with final answer, i.e. TM tip without -j:
> 
> gczeal(1)
> for each(let x in [new Number(1)]) {
>     print(x)
> } []
> 
> Assertion failure: !JSString::isStatic((JSString *) (thing)), at
> ../jsgc.cpp:1184

DEBUG-only, due to this assertion:

2725	        JS_ASSERT(kind == js_GetGCThingTraceKind(JSVAL_TO_GCTHING(v)));

> Should I file a new bug? (It'll have to wait for me though - bedtime at EDT)

No, I'll fix it forthwith.

/be
Followup tm cset for this bug:

http://hg.mozilla.org/tracemonkey/rev/ae85c4258b25

/be
Static gc-things without hot-path perf hits testing for "not in gc heap", I like it. How much did the sum of these patches win on SS?

/be
current tip compared to changeset:   32143:991d79f172c3
tag:         tip
user:        Jeff Walden <jwalden@mit.edu>
date:        Fri Sep 04 02:16:05 2009 -0700


TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           1.012x as fast    1280.4ms +/- 0.4%   1265.6ms +/- 0.6%     significant

=============================================================================

  3d:                  ??                 189.9ms +/- 0.6%    190.4ms +/- 0.8%     not conclusive: might be *1.003x as slow*
    cube:              ??                  53.3ms +/- 1.3%     53.8ms +/- 1.6%     not conclusive: might be *1.009x as slow*
    morph:             -                   39.9ms +/- 1.0%     39.4ms +/- 0.9% 
    raytrace:          ??                  96.7ms +/- 0.6%     97.2ms +/- 1.3%     not conclusive: might be *1.005x as slow*

  access:              1.009x as fast     208.9ms +/- 0.7%    207.1ms +/- 0.5%     significant
    binary-trees:      1.018x as fast      55.5ms +/- 0.9%     54.5ms +/- 0.9%     significant
    fannkuch:          -                   92.1ms +/- 0.7%     91.8ms +/- 1.0% 
    nbody:             -                   41.3ms +/- 1.4%     40.9ms +/- 1.0% 
    nsieve:            -                   20.0ms +/- 1.7%     19.9ms +/- 2.0% 

  bitops:              -                   56.8ms +/- 1.4%     56.4ms +/- 1.1% 
    3bit-bits-in-byte: -                    2.5ms +/- 15.1%      2.3ms +/- 15.0% 
    bits-in-byte:      -                   14.8ms +/- 2.0%     14.6ms +/- 2.5% 
    bitwise-and:       *1.033x as slow*     3.0ms +/- 0.0%      3.1ms +/- 7.3%     significant
    nsieve-bits:       -                   36.5ms +/- 1.4%     36.4ms +/- 1.4% 

  controlflow:         1.012x as fast      51.4ms +/- 0.7%     50.8ms +/- 1.3%     significant
    recursive:         1.012x as fast      51.4ms +/- 0.7%     50.8ms +/- 1.3%     significant

  crypto:              -                   77.7ms +/- 2.3%     76.5ms +/- 1.7% 
    aes:               -                   46.1ms +/- 3.5%     45.0ms +/- 3.3% 
    md5:               -                   19.8ms +/- 1.5%     19.8ms +/- 1.5% 
    sha1:              -                   11.8ms +/- 2.6%     11.7ms +/- 4.1% 

  date:                1.040x as fast     182.9ms +/- 1.2%    175.8ms +/- 0.7%     significant
    format-tofte:      -                   92.5ms +/- 0.5%     92.3ms +/- 0.9% 
    format-xparb:      1.083x as fast      90.4ms +/- 2.4%     83.5ms +/- 0.6%     significant

  math:                ??                  45.5ms +/- 1.9%     46.0ms +/- 1.0%     not conclusive: might be *1.011x as slow*
    cordic:            -                   15.3ms +/- 2.3%     15.3ms +/- 2.3% 
    partial-sums:      ??                  20.6ms +/- 1.8%     20.9ms +/- 1.1%     not conclusive: might be *1.015x as slow*
    spectral-norm:     ??                   9.6ms +/- 3.8%      9.8ms +/- 3.1%     not conclusive: might be *1.021x as slow*

  regexp:              ??                  56.0ms +/- 0.9%     56.5ms +/- 2.6%     not conclusive: might be *1.009x as slow*
    dna:               ??                  56.0ms +/- 0.9%     56.5ms +/- 2.6%     not conclusive: might be *1.009x as slow*

  string:              1.013x as fast     411.3ms +/- 0.3%    406.1ms +/- 1.1%     significant
    base64:            1.152x as fast      20.5ms +/- 1.8%     17.8ms +/- 1.7%     significant
    fasta:             -                   87.3ms +/- 0.6%     87.2ms +/- 0.6% 
    tagcloud:          1.011x as fast     133.3ms +/- 0.6%    131.9ms +/- 0.6%     significant
    unpack-code:       *1.008x as slow*   130.6ms +/- 0.3%    131.7ms +/- 0.5%     significant
    validate-input:    1.056x as fast      39.6ms +/- 0.9%     37.5ms +/- 8.4%     significant
There is also the patch from bug 514267 included that explains the win for base64. But overall a great win!
(In reply to comment #22)
> Followup tm cset for this bug:
> 
> http://hg.mozilla.org/tracemonkey/rev/ae85c4258b25
> 
> /be

With tm changeset http://hg.mozilla.org/tracemonkey/rev/05f46557d718 (that includes the above fix):

(function () {
    gczeal(1);
    eval("<x/>")
})()

Assertion failure: !JSString::isStatic((JSString *) (thing)), at ../jsgc.cpp:1184

I'm guessing this is (still) part of this bug, and this also seems E4X-related since when I replace "<x/>" with "[]", the assert doesn't occur.
(In reply to comment #26)
> (In reply to comment #22)
> > Followup tm cset for this bug:
> > 
> > http://hg.mozilla.org/tracemonkey/rev/ae85c4258b25
> > 
> > /be
> 
> With tm changeset http://hg.mozilla.org/tracemonkey/rev/05f46557d718 (that
> includes the above fix):
> 
> (function () {
>     gczeal(1);
>     eval("<x/>")
> })()
> 
> Assertion failure: !JSString::isStatic((JSString *) (thing)), at
> ../jsgc.cpp:1184
> 
> I'm guessing this is (still) part of this bug, and this also seems E4X-related
> since when I replace "<x/>" with "[]", the assert doesn't occur.

Ditto (with -j turned on):

(function () {
    (eval("\
        (function () {\
            for (var y = 0; y < 16; ++y) {\
                if (y % 3 == 2) {\
                    gczeal(1);\
                } else {\
                    print(0 / 0);\
                }\
            }\
        });\
    "))()
})();

Assertion failure: thing, at ../jsgc.cpp:2610

Regression window is http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c216d3357c5c&tochange=49b7b7e34b5e which involves this bug so (again) I'm guessing this is related.
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #22)
> > > Followup tm cset for this bug:
> > > 
> > > http://hg.mozilla.org/tracemonkey/rev/ae85c4258b25
> > > 
> > > /be
> > 
> > With tm changeset http://hg.mozilla.org/tracemonkey/rev/05f46557d718 (that
> > includes the above fix):
> > 
> > (function () {
> >     gczeal(1);
> >     eval("<x/>")
> > })()
> > 
> > Assertion failure: !JSString::isStatic((JSString *) (thing)), at
> > ../jsgc.cpp:1184
> > 
> > I'm guessing this is (still) part of this bug, and this also seems E4X-related
> > since when I replace "<x/>" with "[]", the assert doesn't occur.

It's all about E4X but in conjunction with JSTempValueRooter not tagging variant pointers it scans as jsvals, instead relying on them pointing into the gc-heap and therefore having flag bytes per thing telling gc-thing-type. I will fix shortly.

> Ditto (with -j turned on):
> 
> (function () {
>     (eval("\
>         (function () {\
>             for (var y = 0; y < 16; ++y) {\
>                 if (y % 3 == 2) {\
>                     gczeal(1);\
>                 } else {\
>                     print(0 / 0);\
>                 }\
>             }\
>         });\
>     "))()
> })();
> 
> Assertion failure: thing, at ../jsgc.cpp:2610
> 
> Regression window is
> http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c216d3357c5c&tochange=49b7b7e34b5e
> which involves this bug so (again) I'm guessing this is related.

No, this is probably caused by the patch for 513981, so please file separately.

/be
(In reply to comment #28)
> > Regression window is
> > http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c216d3357c5c&tochange=49b7b7e34b5e
> > which involves this bug

And what's more, this bug is not listed among the csets there (I forgot to note).

/be
(In reply to comment #28)
> > Regression window is
> > http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c216d3357c5c&tochange=49b7b7e34b5e
> > which involves this bug so (again) I'm guessing this is related.
> 
> No, this is probably caused by the patch for 513981, so please file separately.

Sure.

> And what's more, this bug is not listed among the csets there (I forgot to
> note).
> 
> /be

I perhaps was referring to bug 513530 - which was the changeset that this bug is probably related to - so apologies for not being clear on that.
(In reply to comment #28)
> > > (function () {
> > >     gczeal(1);
> > >     eval("<x/>")
> > > })()
> > > 
> > > Assertion failure: !JSString::isStatic((JSString *) (thing)), at
> > > ../jsgc.cpp:1184
[...]
> It's all about E4X but in conjunction with JSTempValueRooter not tagging
> variant pointers it scans as jsvals, instead relying on them pointing into the
> gc-heap and therefore having flag bytes per thing telling gc-thing-type. I will
> fix shortly.

I should have seen this coming, Igor wrote very clear comments. But I don't see why we should assume object-tagged (0-tagged) jsvals can contain any gc-thing pointer, just for JSTempValueRooter to use untagged pointer types.

The patch to use a tagged jsval instead of the JSTVU_SINGLE untagged unioned pointer types will be more than I can justify as a followup, however. I'll file it separately.

/be
(In reply to comment #30)
> (In reply to comment #28)
> > > Regression window is
> > > http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=c216d3357c5c&tochange=49b7b7e34b5e
> > > which involves this bug so (again) I'm guessing this is related.
> > 
> > No, this is probably caused by the patch for 513981, so please file separately.
> 
> Sure.

Filed bug 514999. Thanks, Brendan!
Depends on: 515000
Filed bug 515000 on the JSTempValueRooter thing.

/be
Blocks: 515199
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
http://hg.mozilla.org/mozilla-central/rev/3be24656202c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Crash Signature: [@ js_IsAboutToBeFinalized] [@ js_GetGCThingTraceKind]
Crash Signature: [@ js_IsAboutToBeFinalized] [@ js_GetGCThingTraceKind] → [@ js_IsAboutToBeFinalized] [@ js_GetGCThingTraceKind]
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.