Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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.
Attachment #584748 - Flags: review?(wmccloskey)
(Assignee)

Updated

6 years ago
Summary: Missed FreeChunkList call in JSRuntime::onOutOfMemory → Missed FreeChunkList call in JSRuntime::onOutOfMemory and other fixes
(Assignee)

Comment 2

6 years ago
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.
Attachment #584748 - Attachment is obsolete: true
Attachment #584748 - Flags: review?(wmccloskey)
Attachment #584767 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

6 years ago
Created attachment 584769 [details] [diff] [review]
v3

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+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c245807aad3a
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
(Assignee)

Comment 6

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

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/91c63f6c1b13
https://hg.mozilla.org/mozilla-central/rev/91c63f6c1b13
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
Blocks: 714280
You need to log in before you can comment on or make changes to this bug.