419091 - Assertion failure: JS_PROPERTY_CACHE(cx).disabled >= 0, at jsinterp.c:463 using js.c Scatter() test and gczeal(2)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brian Crowder]

gczeal(2);

function f() {
    for (let i = 0; i < 10; i++) {
        let y = { x: i }
    }
}

for (let i = 0; i < 10; i++)
    scatter([f, f]);

Comment 1

[Jason Orendorff [:jorendorff]]

At the top of js_GC, we call js_DisablePropertyCache for a certain set of contexts.  At the bottom, we call js_EnablePropertyCache for a certain set of contexts.  If the two sets are not equal, the effect on .disabled can be unbalanced.

JS_SetContextThread racing with GC can make this happen.  Sequence of events:

* Thread 1 creates context cx2 and calls JS_ClearContextThread(cx2), so cx2->thread is NULL.
* Thread 1 starts thread 2.
* Thread 1 enters GC.
* The "disable pcache" code in js_GC runs; at this point cx2->thread is NULL.
* Thread 2 calls JS_SetContextThread(cx2), setting cx2->thread.  This happens outside all locks/requests.
* The "enable pcache" code in js_GC runs; at this point cx2->thread is non-null, so js_GC calls js_EnablePropertyCache(cx2) without having disabled the property cache on thread 2.  The assertion fails here.

That story is consistent with my gdb-ing.

One sort of heavy-handed fix would be to JS_LOCK_GC and JS_AWAIT_GC_DONE in JS_{Set,Clear}ContextThread.

I think another possibility would be to make js_GC disable/enable the cache only for contexts that are in active requests (i.e. stopped inside libjs at a safe point other than JS_BeginRequest).  Haven't thought this through, though.

Comment 2

[Mike Moening]

I also just hit this bug in a big way.
This pretty much breaks any multi-threaded embedder.
It happens exactly as described above in comment #1.

We have an idle pool of many contexts.
js_SetContextThread() is called one of the contexts while another is running GC.

A thread could also call JS_ClearContextThread() and unbalance the enable/disable pairing while GC is running too.

This bug should block 1.9.

I also don't like the JS_LOCK_GC above.  Thread contention is evil.
I suppose you could keep a temporary list of those contexts which you called js_DisablePropertyCache() on to ensure you only call js_EnablePropertyCache() on the right ones.  A flag on the context indicating its property cache is "disabled" would do the same I suppose...

Brendan, I need a work around (or fix).
Can you suggest one?
I can write the patch.  Tell me how you want this done.

Thanks.

Comment 3

[Brendan Eich [:brendan]]

Mike: with the patch for bug 425828 applied to your tree, how are things?

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Running the code in comment 0 a few times still hits this assertion in trunk (debug JS_THREADSAFE js shell on Mac).

blocking1.9?

Comment 5

[Mike Shaver (:shaver emeritus)]

We do not live in a world in which we would not ship 1.9 (or FF3) because this bug was left unfixed, given that the propcache disabling itself is just paranoia (per the check-in comment) and this only affects debug builds that race context creation against GC. Minusing!

Comment 6

[Mike Moening]

I thought there were several threads in FF3?  main, anti-phishing, others?
This is just plain busted and needs fixing Mike.

Comment 7

[Benjamin Smedberg]

As noted in bug 429978: we walk the context list here without holding the GC lock: http://mxr.mozilla.org/seamonkey/source/js/src/jsgc.c#3179

Comment 8

[Mike Shaver (:shaver emeritus)]

Nobody's saying it doesn't need fixing (that would be WONTFIX or INVALID), just that it would not keep us from shipping if this bug were the last one left.  JS is only run on one thread in FF3 outside of extensions, and even with (unknown) extensions the specific race is a) rare, and b) harmless unless scripts run during GC, which they very much should not.

Comment 9

[Brendan Eich [:brendan]]

MikeM: we'll fix before the 1.8 source release (which should happen pretty soon, first half of this year or bust). Shaver, please set blocking js1.8src for such bugs you may mark 1.9-.

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Roger, wilco.  Thanks.

Comment 11

[Mike Moening]

Brendan,
Any news?  A forthcoming patch would be nice...
I'm still running into this on a regular basis..

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I've hit this assert frequently with the DOM worker threads and I don't think we could try shipping the feature with this bug.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v1 - the "don't do that" fix

This fixes the bug by not disabling the property cache during GC.  (This is just what bsmedberg did in bug 429978 comment 4.)

bent and Mike, give this a shot.  brendan may minus this one, as it removes some paranoia.  Thinking it over, I'm not sure that particular bit of paranoia is worth keeping, though.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 325825 [details] [diff] [review]
v1 - the "don't do that" fix

This seems to prevent the asserts in my threading tests. Thanks!

Comment 15

[Brendan Eich [:brendan]]

Take my bug... please!

(Who remembers Henny Youngman?)

/be

Comment 16

[Brendan Eich [:brendan]]

Should go into hg.m.o/m-c for 1.9.1, for sure.

bent, want to make this block your workerpool bug for compleatness?

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 325825 [details] [diff] [review]
v1 - the "don't do that" fix

>-    /*
>-     * Clear property cache weak references and disable the cache so nothing
>-     * can fill it during GC (this is paranoia, since scripts should not run
>-     * during GC).
>-     */
>-    js_DisablePropertyCache(cx);

Agree on taking this out, at this place in jsgc.cpp -- but we should JS_ASSERT(!cx->runtime->gcRunning) in js_FillPropertyCache to cover the bet.

>-    if (!(rt->shapeGen & SHAPE_OVERFLOW_BIT)) {
>-        js_EnablePropertyCache(cx);
>-#ifdef JS_THREADSAFE
>-        iter = NULL;
>-        while ((acx = js_ContextIterator(rt, JS_FALSE, &iter)) != NULL) {
>-            if (!acx->thread || acx->thread == cx->thread)
>-                continue;
>-            js_EnablePropertyCache(acx);
>-        }
>-#endif
>-    }
>-

This isn't right, if we manage to overflow the shape identifier space then we should disable here (rather than decide not to re-enable), and run without the property cache until such time as shapes are GC'd to reduce the total number of shapes to less 2**24.

/be

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Indeed, blocks bug 437152.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to comment #17)
Sorry for missing that, my spider-sense tingled but I ignored it.  :-P

I'm scratching my head over something though.  I don't see what implements the "until such time as..." part currently; instead it looks like the property cache would remain disabled forever, and GC would start happening very frequently (from js_GenerateShape).

If that's right, it would be a separate bug; for now I'll just retain the current behavior by flipping that if-block (rather than deleting it).

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: v2

This update addresses comment 17, but see comment 19.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 326015 [details] [diff] [review]
v2

You're right, I dreamt about healing (and avoiding fruitless mega-GC-run attempts to reduce shape population) but never implemented. (Too many late nights were required to get the shape-based stuff done and right for Fx3.)

Could you please file a bug on that and cite it in a FIXME: comment for the overflow paragraph in js_GC? Thanks, r=me with that.

/be

Comment 22

[Jason Orendorff [:jorendorff]]

Filed bug 440834.

Comment 23

[Brendan Eich [:brendan]]

We'll do a mini-branch in cvs.mozilla.org for js1.8, and back-port what lands here. Should be easy. If you happen to have a cvs tree around and want to do the patch, we'll be grateful.

/be

Comment 24

[Jason Orendorff [:jorendorff]]

Pushed (with FIXME comment):
http://hg.mozilla.org/mozilla-central/index.cgi/rev/d971499d7eda

Should this bug remain open for tracking js1.8?  I'm marking it FIXED for now.

Comment 25

[Brendan Eich [:brendan]]

It blocks js1.8src even thought it is resolved, so it will be tracked.

/be

Comment 26

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/extensions/regress-419091.js,v  <--  regress-419091.js
initial revision: 1.1

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

Comment 27

[ash_mozilla]

Attachment: v2 CVS

Patch v2 updated to apply to CVS for RC2.

Comment 28

[ash_mozilla]

Attachment: v2 CVS-2

Fix broken logic inversion around rt->shapeGen