Call u_init(UErrorCode*) and u_cleanup() to fix up the memory-reporting situation wrt ICU

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

You can call ICU functions without initializing the library, and you can exit without calling the cleanup function.  But doing so means you leak stuff, which tinderbox stats don't much like.  We should init and cleanup at some sort of reasonable points so as not to have ICU perturb stats (and to fix the leak-ish sort of behavior that's happening now, although realistically this stuff's going to exist for about the lifetime of the process).
Depends on: 896123
Depends on: 896124
Created attachment 778756 [details] [diff] [review]
Initialize and cleanup ICU in JS_Init and JS_ShutDown

This depends on the patch in bug 896124, obviously.  I *think* the patch I reviewed from you that accounted for ICU allocations will play nicely with this -- u_cleanup() also clears the stored memory-management function pointers, so they have to be reset, but XPCJSRuntime should do that, so I think we're okay -- but you'll want to be aware of that issue when updating that patch.

Before:

[jwalden@find-waldo-now src]$ valgrind --leak-check=full dbg/js -e 'Intl'
==25032== Memcheck, a memory error detector
==25032== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==25032== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==25032== Command: dbg/js -e Intl
==25032== 
==25032== 
==25032== HEAP SUMMARY:
==25032==     in use at exit: 187,743 bytes in 181 blocks
==25032==   total heap usage: 6,785 allocs, 6,604 frees, 3,588,734 bytes allocated
==25032== 
==25032== 150,784 bytes in 1 blocks are possibly lost in loss record 170 of 170
==25032==    at 0x4A06409: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==25032==    by 0xBFC117: uprv_malloc_50 (cmemory.c:85)
==25032==    by 0xC4C4B0: icu_50::UMemory::operator new[](unsigned long) (uobject.cpp:70)
==25032==    by 0xCF6B3D: icu_50::isAvailableLocaleListInitialized(UErrorCode&) (coll.cpp:291)
==25032==    by 0xCF69F0: icu_50::Collator::getAvailableLocales(int&) (coll.cpp:509)
==25032==    by 0xBE78F7: ucol_countAvailable_50 (ucol_res.cpp:748)
==25032==    by 0x4F12FC: intl_availableLocales(JSContext*, int (*)(), char const* (*)(int), JS::MutableHandle<JS::Value>) (Intl.cpp:435)
==25032==    by 0x4F11D3: js::intl_Collator_availableLocales(JSContext*, unsigned int, JS::Value*) (Intl.cpp:738)
==25032==    by 0x4D2BE4: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:226)
==25032==    by 0x4D24B9: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (Interpreter.cpp:477)
==25032==    by 0x4CCC43: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2500)
==25032==    by 0x4C69E3: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:434)
==25032== 
==25032== LEAK SUMMARY:
==25032==    definitely lost: 0 bytes in 0 blocks
==25032==    indirectly lost: 0 bytes in 0 blocks
==25032==      possibly lost: 150,784 bytes in 1 blocks
==25032==    still reachable: 36,959 bytes in 180 blocks
==25032==         suppressed: 0 bytes in 0 blocks
==25032== Reachable blocks (those to which a pointer was found) are not shown.
==25032== To see them, rerun with: --leak-check=full --show-reachable=yes
==25032== 
==25032== For counts of detected and suppressed errors, rerun with: -v
==25032== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)

After:

[jwalden@find-waldo-now src]$ valgrind --leak-check=full dbg/js -e 'Intl'
==25246== Memcheck, a memory error detector
==25246== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==25246== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==25246== Command: dbg/js -e Intl
==25246== 
==25246== 
==25246== HEAP SUMMARY:
==25246==     in use at exit: 0 bytes in 0 blocks
==25246==   total heap usage: 6,787 allocs, 6,787 frees, 3,589,358 bytes allocated
==25246== 
==25246== All heap blocks were freed -- no leaks are possible
==25246== 
==25246== For counts of detected and suppressed errors, rerun with: -v
==25246== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)
Attachment #778756 - Flags: review?(n.nethercote)
Attachment #778756 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77439f57dfc
Target Milestone: --- → mozilla25

Comment 3

4 years ago
https://hg.mozilla.org/mozilla-central/rev/c77439f57dfc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.