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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

__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

Comment 1

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

Opening up since bug 513530 wasn't on any other branch except tracemonkey.

Comment 2

[Andreas Gal :gal]

Congrats Gregor. Your first GC hazard :) Want to debug?

Comment 3

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

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.

Comment 4

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

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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Gregor Wagner]

Attachment: 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?

Comment 7

[Andreas Gal :gal]

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.

Comment 8

[Robert Sayre]

this is crashing opt builds on the tinderbox as well

Comment 9

[Andreas Gal :gal]

#7 meant can't. We should back out until this is fixed.

Comment 10

[Brendan Eich [:brendan]]

This is easy to fix. We should not be atomizing atomized strings. Patch in a few.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: complete fix, no hot paths touched

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

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Brendan Eich [:brendan]]

Test in comment 0.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: pick spacing and paren nits in revised THING_TO_ARENA

Comment 15

[Andreas Gal :gal]

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.

Comment 16

[Brendan Eich [:brendan]]

(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

Comment 17

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

(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.

Comment 18

[Brendan Eich [:brendan]]

Attachment: my final answer (builds on gwagner's fix)

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

Comment 19

[Brendan Eich [:brendan]]

(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

Comment 20

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

(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)

Comment 21

[Brendan Eich [:brendan]]

(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

Comment 22

[Brendan Eich [:brendan]]

Followup tm cset for this bug:

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

/be

Comment 23

[Brendan Eich [:brendan]]

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

Comment 24

[Gregor Wagner]

current tip compared to changeset:   32143:991d79f172c3
tag:         tip
user:        Jeff Walden <[e-mail]>
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

Comment 25

[Gregor Wagner]

There is also the patch from bug 514267 included that explains the win for base64. But overall a great win!

Comment 26

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

(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.

Comment 27

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

(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.

Comment 28

[Brendan Eich [:brendan]]

(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

Comment 29

[Brendan Eich [:brendan]]

(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

Comment 30

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

(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.

Comment 31

[Brendan Eich [:brendan]]

(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

Comment 32

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

(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!

Comment 33

[Brendan Eich [:brendan]]

Filed bug 515000 on the JSTempValueRooter thing.

/be

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3be24656202c

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ae85c4258b25

Comment 36

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a85a5702977f

Comment 37

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/68ad00a63adf