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

VERIFIED FIXED in mozilla1.2alpha

Status

()

P1
critical
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.2alpha
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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
(Assignee)

Comment 1

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

/be
Severity: normal → critical
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla1.2
Priority: -- → P1
Target Milestone: --- → mozilla1.2alpha
(Assignee)

Comment 2

17 years ago
Created attachment 95337 [details] [diff] [review]
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
(Assignee)

Comment 3

17 years ago
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
Attachment #95337 - Flags: superreview+

Comment 4

17 years ago
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.
(Assignee)

Comment 5

17 years ago
Timeless, settle down.

/be
(Assignee)

Comment 6

17 years ago
Created attachment 95713 [details] [diff] [review]
fix revised to cope with problem seen by MH (Frank Schroeder)
Attachment #95337 - Attachment is obsolete: true
(Assignee)

Comment 7

17 years ago
Created attachment 95714 [details] [diff] [review]
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
Attachment #95713 - Attachment is obsolete: true
(Assignee)

Comment 8

17 years ago
Created attachment 96220 [details] [diff] [review]
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
Attachment #95714 - Attachment is obsolete: true
Comment on attachment 96220 [details] [diff] [review]
oops, don't break JSGC_END's semantic

r=shaver
(Assignee)

Comment 10

17 years ago
Created attachment 96355 [details] [diff] [review]
diff -wu version for reviewing only

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

/be

Comment 11

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

sr=jband looks reasonable to me
Attachment #96220 - Flags: superreview+
(Assignee)

Comment 12

17 years ago
Fixed in the trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 13

17 years ago
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.