Closed Bug 714066 Opened 8 years ago Closed 8 years ago

Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter 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.
Attachment #584748 - Flags: review?(wmccloskey)
Summary: Missed FreeChunkList call in JSRuntime::onOutOfMemory → Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
Attached patch v2 (obsolete) — Splinter 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.
Attachment #584748 - Attachment is obsolete: true
Attachment #584748 - Flags: review?(wmccloskey)
Attachment #584767 - Flags: review?(wmccloskey)
Attached patch v3Splinter Review
In v2 I have not eliminated everything related to waiveGCQuota. This version should purge all dead wood.
Attachment #584767 - Attachment is obsolete: true
Attachment #584767 - Flags: review?(wmccloskey)
Attachment #584769 - Flags: review?(wmccloskey)
Attachment #584769 - Flags: review?(wmccloskey) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/91c63f6c1b13
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Blocks: 714280
You need to log in before you can comment on or make changes to this bug.