Last Comment Bug 714066 - Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
: Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Igor Bukanov
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 702251 714280
  Show dependency treegraph
Reported: 2011-12-29 07:40 PST by Igor Bukanov
Modified: 2011-12-30 07:31 PST (History)
18 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (4.62 KB, patch)
2011-12-29 07:47 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (9.15 KB, patch)
2011-12-29 08:48 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
v3 (10.76 KB, patch)
2011-12-29 08:59 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-12-29 07:40:28 PST
In the bug 702251 I changed ChunkPool::expire so it no longer frees the chunk list itself delegating that to the caller. However, I have not updated JSRuntime::onOutOfMemory to release the list of the chunks returned that the expire returns after the change leading to a chunk leak on OOM condition.
Comment 1 Igor Bukanov 2011-12-29 07:47:17 PST
Created attachment 584748 [details] [diff] [review]

The patch fixes the leak and another old issue that I have found when checking for bad expire calls.

When calling startBackgroundSweep in GCCycle the code assumes that gckind != GC_LAST_CONTEXT && rt->state != JSRTS_LANDING means we should do the background sweep. However, the background sweep is setup like:

    if (gckind != GC_LAST_CONTEXT && rt->state != JSRTS_LANDING) {
        if (rt->gcHelperThread.prepareForBackgroundSweep(cx))
            cx->gcBackgroundFree = &rt->gcHelperThread;

That is, the background sweep may not be available if prepareForBackgroundSweep fails on OOM. I chnaged the code so it starts the background sweep only if cx->gcBackgroundFree is not null.
Comment 2 Igor Bukanov 2011-12-29 08:48:08 PST
Created attachment 584767 [details] [diff] [review]

The new version fixes yet another issue - the trace JIT removal has not removed no longer needed ThreadData::waiveGCQuota. So the patch eliminates the field and related code.
Comment 3 Igor Bukanov 2011-12-29 08:59:06 PST
Created attachment 584769 [details] [diff] [review]

In v2 I have not eliminated everything related to waiveGCQuota. This version should purge all dead wood.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-29 14:11:58 PST
Backed out due to Linux make check orange:
Comment 6 Igor Bukanov 2011-12-29 15:37:20 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Backed out due to Linux make check orange:
> Inbound&full=1

Apparently the test depend on the operation callback reporting OOM if the GC cannot shrink below gcMaxBytes. So I will land the v2 version of the patch without the latest cleanup delegating that to another bug.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-30 04:54:29 PST

Note You need to log in before you can comment on or make changes to this bug.