Last Comment Bug 714066 - Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
: Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Igor Bukanov
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (4.62 KB, patch)
2011-12-29 07:47 PST, Igor Bukanov
no flags Details | Diff | Review
v2 (9.15 KB, patch)
2011-12-29 08:48 PST, Igor Bukanov
no flags Details | Diff | Review
v3 (10.76 KB, patch)
2011-12-29 08:59 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | 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]
v1

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]
v2

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]
v3

In v2 I have not eliminated everything related to waiveGCQuota. This version should purge all dead wood.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-29 14:11:58 PST
Backed out due to Linux make check orange:
https://tbpl.mozilla.org/php/getParsedLog.php?id=8218822&tree=Mozilla-Inbound&full=1

https://hg.mozilla.org/integration/mozilla-inbound/rev/21ff3ef9e329
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:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=8218822&tree=Mozilla-
> Inbound&full=1
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/21ff3ef9e329

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] (khuey@mozilla.com) 2011-12-30 04:54:29 PST
https://hg.mozilla.org/mozilla-central/rev/91c63f6c1b13

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