As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 890127 - Call u_init(UErrorCode*) and u_cleanup() to fix up the memory-reporting situation wrt ICU
: Call u_init(UErrorCode*) and u_cleanup() to fix up the memory-reporting situa...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 896123 896124
Blocks: 853301
  Show dependency treegraph
 
Reported: 2013-07-03 18:32 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-07-29 09:22 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initialize and cleanup ICU in JS_Init and JS_ShutDown (1.59 KB, patch)
2013-07-19 17:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
n.nethercote: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-03 18:32:08 PDT
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).
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-19 17:51:01 PDT
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)
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-27 21:15:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77439f57dfc
Comment 3 User image Ed Morley [:emorley] 2013-07-29 08:22:51 PDT
https://hg.mozilla.org/mozilla-central/rev/c77439f57dfc

Note You need to log in before you can comment on or make changes to this bug.