Closed Bug 1377738 Opened 2 years ago Closed 2 years ago

virtual memory leak in JS_DestroyContext

Categories

(Core :: JavaScript Engine, defect)

56 Branch
All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: alexbmstu, Assigned: jonco)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

JS_DestroyContext(  JS_NewContext(1024*1024)  );


Actual results:

> 256 KB of virtual memory is leaked. Because gc.emptyChunks_ isn't free.


Expected results:

no virtual memory leak.
Hardware: Unspecified → All
Summary: memory leak in JS_DestroyContext → virtual memory leak in JS_DestroyContext
(In reply to Alex from comment #0)
It seems to me that this should be freed in GCRuntime::finish().  Can you post your whole testcase?
Flags: needinfo?(alexbmstu)
#include <stdio.h>
#include <stdlib.h>
#include "include/jsapi.h"
#include "include/js/Initialization.h"
int main(int argc, char** argv) {
    if (!JS_Init())
        return 1;
    for (int i = 0; i < 100; ++i) {
        JS_DestroyContext( JS_NewContext(1024 * 1024) );
    }
    JS_ShutDown();
    return 0;
}

// > this should be freed in GCRuntime::finish().
// But  gc.nursery.disable();  adds the new empty chunks again.
Flags: needinfo?(alexbmstu)
(In reply to Alex from comment #2)
You're right, nice catch.
Assignee: nobody → jcoppeard
Patch to disable the nursery before we free the empty chunks, otherwise the nursery will add more empty chunks in its destructor.

I removed the loop over zone groups as there is only one nursery again now since bug 1340822.
Attachment #8883919 - Flags: review?(jdemooij)
Comment on attachment 8883919 [details] [diff] [review]
bug1377738-nursery-memory-leak

Review of attachment 8883919 [details] [diff] [review]:
-----------------------------------------------------------------

Yes nice find.
Attachment #8883919 - Flags: review?(jdemooij) → review+
Depends on: 1379232
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c30acbc14073
Disable assertion that chunk pools are empty on destruction for XPCShell failures on Windows 2012 r=me
https://hg.mozilla.org/mozilla-central/rev/5c7396eed432
https://hg.mozilla.org/mozilla-central/rev/c30acbc14073
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attached patch bug1377738-betaSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Generational GC.
[User impact if declined]: Observed memory leak.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple fix that has been on m-c since 8th July.
[String changes made/needed]: None.

Requesting backport as users have been affected by this memory leak (see bug 1379250).
Attachment #8886248 - Flags: review+
Attachment #8886248 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1379250
Can I push for this being landed on Beta?

I was trying to get this taken seriously and fixed throughout the 55 nightly cycle but only a couple of people actually tried digging into it.
more coverage at
bug 1298905
bug 1300724

and this possibly also fixes bug 1361016
(In reply to Danial Horton from comment #11)
> Can I push for this being landed on Beta?
The patch was nominated for being landed on beta in comment 9. It make take a day or two for approval and landing.
Comment on attachment 8886248 [details] [diff] [review]
bug1377738-beta

gc leak fix, beta55+
Attachment #8886248 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jon Coppeard (:jonco) from comment #9)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jon's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Duplicate of this bug: 1361016
Duplicate of this bug: 1347909
You need to log in before you can comment on or make changes to this bug.