162779 - Ancient JS GC bug where multiple threads racing to GC may fail to GC at all

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Leaving spurious out-of-memory errors.  It's due to rt->gcPoke being cleared too
soon if set, in the code near the top of js_GC.  Patch coming up.

/be

Comment 1

[Brendan Eich [:brendan]]

This bug's patch should go into the 1.0 branch as soon as the 1.5 final landing
happens, if not sooner.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Poke before last ditch call to js_GC from js_AllocGCThing, and clear rt->gcPoke
only after finishing garbage collection, so racing js_GCs see gcPoke set, do
not bail with the early return if (!rt->gcPoke), and flow into the code that
awaits the completion of the garbage collection that's already in progress.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 95337 [details] [diff] [review]
proposed fix

jband sez sr=jband (thanks again to him for helping test the testcase from a
SpiderMonkey embedder).

/be

Comment 4

[timeless]

that's nice of him, but for one minor problem:
1.39 brendan%mozilla.org Jul 11 15:43 Out with the old, in with the new.
http://www.mozilla.org/webtools/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=reviewers.html&root=/cvsroot&subdir=mozilla-org/html/hacking&command=DIFF_FRAMESET&rev1=1.38&rev2=1.39
you should probably ask shaver to sr, or you could get some other sr to sr based
on jband's word that it's good ;-) -- personally i'd probably ask jband to r=
and get ye old random sr.

otoh, it's possible i missed a nonpublished memo, that happens to me constantly.
anyway, i checked, the document still contains this fragment (it had me worried
for a moment):
Super-review does not require domain expertise (module owners and peers supply
that, usually), so the areas below are not pigeon-holes -- you can solicit a
super-review from any reviewer on the list, but using area as a guide will get
quicker results in the typical case.

Comment 5

[Brendan Eich [:brendan]]

Timeless, settle down.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix revised to cope with problem seen by MH (Frank Schroeder)

Comment 7

[Brendan Eich [:brendan]]

Attachment: take 2

Forgot all the other early return paths, which are now fused with a done: block
at the bottom of js_GC that (if !(gcflags & GC_ALREADY_LOCKED)) unlocks
rt->gcLock, then calls the JSGC_END callback (jband, this restores symmetry
with JSGC_BEGIN, which was broken before -- can you review?).

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: oops, don't break JSGC_END's semantic

The JSGC_END GC-callback case, by design, is not called for every racing GC
attempt that synchronizes with the thread actually collecting garbage -- it
must be called only on the thread actually collecting garbage, after it has
finished and released rt->gcLock.  My last patch gratuitously and overzealously
(I was motivated by code-sharing interests, to fuse the 'if (!(gcflags &
GC_ALREADY_LOCKED)) JS_UNLOCK_GC(rt);' statement) made all racing GC-threads
call the GC-callback with JSGC_END.  This would at best do unnecessary work and
at worst break someone.

JSGC_BEGIN is different because we want any thread trying to start GC, before
it has found another thread already collecting garbage, to be able to cause an
early return from js_GC, via a false return from the GC-callback.

Looking for r= and (jband again) sr=.

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 96220 [details] [diff] [review]
oops, don't break JSGC_END's semantic

r=shaver

Comment 10

[Brendan Eich [:brendan]]

Attachment: diff -wu version for reviewing only

jband's around, I have hopes he'll stamp sr= after reading this diff.

/be

Comment 11

[John Bandhauer]

Comment on attachment 96220 [details] [diff] [review]
oops, don't break JSGC_END's semantic

sr=jband looks reasonable to me

Comment 12

[Brendan Eich [:brendan]]

Fixed in the trunk.

/be

Comment 13

[Phil Schwartau]

Marking Verified -