1,104 or 1,932 bytes in 92 blocks are definitely lost in loss record 1 of 1 as detected by Valgrind

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: billm)

Tracking

(Blocks: 1 bug, {testcase, valgrind})

Trunk
mozilla13
x86_64
Linux
testcase, valgrind
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2] )

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 588518 [details]
sort-of reduced testcase

Attached testcase shows an error with js opt builds on m-c changeset 8ffdb4c7404a when run with -m -a.

valgrind --smc-check=all-non-file --leak-check=full ./js -m -a w6-orig.js

===

==26243== 1,104 bytes in 92 blocks are definitely lost in loss record 1 of 1
==26243==    at 0x4C29313: malloc (vg_replace_malloc.c:263)
==26243==    by 0x505682: SaveScriptFilename(JSContext*, char const*) (Utility.h:135)
==26243==    by 0x506CAC: JSScript::NewScriptFromEmitter(JSContext*, js::BytecodeEmitter*) (jsscript.cpp:1155)
==26243==    by 0x57E5C2: js::frontend::CompileScript(JSContext*, JSObject*, js::StackFrame*, JSPrincipals*, JSPrincipals*, unsigned int, unsigned short const*, unsigned long, char const*, unsigned int, JSVersion, JSString*, unsigned int) (BytecodeCompiler.cpp:361)
==26243==    by 0x41B4E9: JS_EvaluateUCScriptForPrincipals (jsapi.cpp:5339)
==26243==    by 0x41B744: JS_EvaluateUCScript (jsapi.cpp:5390)
==26243==    by 0x4058D5: EvalInContext(JSContext*, unsigned int, JS::Value*) (js.cpp:3017)
==26243==    by 0x405B3A5: ???
==26243==    by 0x5C9AC6: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1051)
==26243==    by 0x5C9E2F: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1109)
==26243==    by 0x498DD8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:3050)
==26243==    by 0x5C9B02: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1078)
(Reporter)

Comment 1

5 years ago
Created attachment 588520 [details]
another sort-of reduced testcase

==26674== 1,932 bytes in 92 blocks are definitely lost in loss record 1 of 1
==26674==    at 0x4C29313: malloc (vg_replace_malloc.c:263)
==26674==    by 0x505682: SaveScriptFilename(JSContext*, char const*) (Utility.h:135)
==26674==    by 0x506CAC: JSScript::NewScriptFromEmitter(JSContext*, js::BytecodeEmitter*) (jsscript.cpp:1155)
==26674==    by 0x57E5C2: js::frontend::CompileScript(JSContext*, JSObject*, js::StackFrame*, JSPrincipals*, JSPrincipals*, unsigned int, unsigned short const*, unsigned long, char const*, unsigned int, JSVersion, JSString*, unsigned int) (BytecodeCompiler.cpp:361)
==26674==    by 0x41B4E9: JS_EvaluateUCScriptForPrincipals (jsapi.cpp:5339)
==26674==    by 0x41B744: JS_EvaluateUCScript (jsapi.cpp:5390)
==26674==    by 0x4058D5: EvalInContext(JSContext*, unsigned int, JS::Value*) (js.cpp:3017)
==26674==    by 0x405B3A5: ???
==26674==    by 0x5C9AC6: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1051)
==26674==    by 0x5C9E2F: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1109)
==26674==    by 0x498DD8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:3050)
==26674==    by 0x5C9B02: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1078)
==26674== 
==26674== LEAK SUMMARY:
==26674==    definitely lost: 1,932 bytes in 92 blocks
==26674==    indirectly lost: 0 bytes in 0 blocks
==26674==      possibly lost: 0 bytes in 0 blocks
==26674==    still reachable: 0 bytes in 0 blocks
==26674==         suppressed: 0 bytes in 0 blocks

Tested on Ubuntu Linux 64-bit.
(Reporter)

Updated

5 years ago
Summary: 1,104 bytes in 92 blocks are definitely lost in loss record 1 of 1 → 1,104 or 1,932 bytes in 92 blocks are definitely lost in loss record 1 of 1 as detected by Valgrind
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
I can reproduce.  billm, this looks like yours.
Assignee: general → wmccloskey
Depends on: 661903
Created attachment 589336 [details] [diff] [review]
remove unused JSRuntime fields

Just a clean-up while I was in the neighbourhood:  JSRuntime has a couple of unused members left over from bug 661903.
Attachment #589336 - Flags: review?(wmccloskey)
(Assignee)

Comment 4

5 years ago
Comment on attachment 589336 [details] [diff] [review]
remove unused JSRuntime fields

I'll try to get a chance to look at this soon.
Attachment #589336 - Flags: review?(wmccloskey) → review+
Comment on attachment 589336 [details] [diff] [review]
remove unused JSRuntime fields

These fields were removed elsewhere.  I'm guessing Igor got them.
Attachment #589336 - Attachment is obsolete: true

Comment 6

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 589336 [details] [diff] [review]
> remove unused JSRuntime fields
> 
> These fields were removed elsewhere.  I'm guessing Igor got them.

That was Luke :)
(Assignee)

Comment 7

5 years ago
Created attachment 598453 [details] [diff] [review]
patch for unrelated leaks

I'm not sure why, but I can't reproduce this problem. When I run it on the given changeset, it says that no leaks are possible. Nick, could you maybe look into this a little more, since you're able to reproduce it? Normally, script filenames are marked via their JSScript and then swept out of the table and freed. If they're not getting freed, that suggests that there are scripts still alive when the last GC happens. That's not supposed to happen.

I did find a leak with this test case on tip. I also fixed an uninitialized memory read.
Attachment #598453 - Flags: review?(christopher.leary)
Attachment #598453 - Flags: review?(christopher.leary) → review+
Blocks: 728779
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/323b7bb0129e

I think we want to leave this open to address the original problem with this test case.
Whiteboard: [MemShrink:P2] → [MemShrink:P2] [leave-open]
https://hg.mozilla.org/mozilla-central/rev/323b7bb0129e
Target Milestone: --- → mozilla13
(In reply to Ed Morley [:edmorley] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/323b7bb0129e

Ftr, this fixed bug 728779.
(Reporter)

Comment 11

5 years ago
(In reply to Bill McCloskey (:billm) from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/323b7bb0129e
> 
> I think we want to leave this open to address the original problem with this
> test case.

The original problem with this testcase is now gone.

I'll assume that the seemingly-unrelated patch in comment 7 somehow fixed the problem.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Whiteboard: [MemShrink:P2] [leave-open] → [MemShrink:P2]
You need to log in before you can comment on or make changes to this bug.