1465350 - Memory leak when copying JSErrorReport

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

js::ErrorToException(...) [1] copies the JSErrorReport including its JSErrorNotes, but stores the result in a js::ScopedJSFreePtr, so when ScopedJSFreePtr's destructor runs, the result is only freed, which means JSErrorReport's destructor isn't executed and the copied JSErrorNotes will leak.

Similar issues are probably present in js::CopyErrorObject(...) [2] and JS::CreateError(...) [3].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#689
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#993-998
[3] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/js/src/jsexn.cpp#1026-1028


Test case:
---
oomAfterAllocations(140);

Function("let a; let a;");
---

Configure flags: --enable-debug --disable-optimize --disable-tests --enable-valgrind --disable-jemalloc

Run with: valgrind --tool=memcheck --leak-check=yes ~/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js /tmp/q.js


Output:
---
==9105== Memcheck, a memory error detector
==9105== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==9105== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==9105== Command: /home/andre/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js /tmp/q.js
==9105== 
==9105== Warning: set address range perms: large range [0x26979c595000, 0x2697dc595000) (noaccess)
out of memory initializing ErrorReport
==9105== Warning: set address range perms: large range [0x26979c595000, 0x2697dc595000) (noaccess)
==9105== 
==9105== HEAP SUMMARY:
==9105==     in use at exit: 72,909 bytes in 5 blocks
==9105==   total heap usage: 8,898 allocs, 8,893 frees, 6,001,222 bytes allocated
==9105== 
==9105== 130 (48 direct, 82 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 5
==9105==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9105==    by 0x492044: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==9105==    by 0x491F78: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==9105==    by 0x491E6C: moz_arena_malloc (malloc_decls.h:115)
==9105==    by 0x5EA512: js_malloc(unsigned long) (Utility.h:388)
==9105==    by 0x5EBAA3: JSErrorNotes* js_new<JSErrorNotes>() (Utility.h:504)
==9105==    by 0x60DE57: js::detail::UniqueSelector<JSErrorNotes>::SingleObject js::MakeUnique<JSErrorNotes>() (in /home/andre/hg/mozilla-inbound/js/src/build-debug-valgrind-obj/dist/bin/js)
==9105==    by 0xD84D61: JSErrorNotes::copy(JSContext*) (jsapi.cpp:7159)
==9105==    by 0xD8A5A6: CopyExtraData(JSContext*, unsigned char**, JSErrorReport*, JSErrorReport*) (jsexn.cpp:252)
==9105==    by 0xD8ACBA: JSErrorReport* CopyErrorHelper<JSErrorReport>(JSContext*, JSErrorReport*) (jsexn.cpp:316)
==9105==    by 0xD8AABC: js::CopyErrorReport(JSContext*, JSErrorReport*) (jsexn.cpp:341)
==9105==    by 0xD8BCED: js::ErrorToException(JSContext*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) (jsexn.cpp:689)
==9105== 
==9105== LEAK SUMMARY:
==9105==    definitely lost: 48 bytes in 1 blocks
==9105==    indirectly lost: 82 bytes in 1 blocks
==9105==      possibly lost: 0 bytes in 0 blocks
==9105==    still reachable: 72,779 bytes in 3 blocks
==9105==         suppressed: 0 bytes in 0 blocks
==9105== Reachable blocks (those to which a pointer was found) are not shown.
==9105== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==9105== 
==9105== For counts of detected and suppressed errors, rerun with: -v
==9105== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
---

Comment 1

[André Bargull [:anba]]

Attachment: bug1465350.patch

I took the easy way out and simply switched the ScopedJSFreePtr to a UniquePtr to ensure the destructor runs. 

And then I've also changed the parameter to pass the UniquePtr by value instead of using a pointer to more clearly express the ownership and fixed a missing error detection in JS::CreateError(...).

Comment 2

[Jan de Mooij [:jandem]]

anba++

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8984173 [details] [diff] [review]
bug1465350.patch

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

If I'm not mistaken, you've touched every call of CopyErrorReport, but you've kept it returning a dumb pointer.  I'm moderately worried about returning a raw pointer and implicitly assuming a deletion-policy by assigning directly into UniquePtr with no policy specified, particularly when in some contexts UniquePtr has one policy, and in other context it has another.  It doesn't look like there's any reason it couldn't return UniquePtr, right?  Please make that change while you're doing this.

Comment 4

[André Bargull [:anba]]

Attachment: bug1465350.patch

Updated patch to return a UniquePtr from CopyErrorHelper per comment #3.

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8753d4a33ab837d106a7bb66b22477a27b847b4

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6350b1a6097e
Use UniquePtr instead of ScopedJSFreePtr for JSErrorReporter. r=Waldo

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6350b1a6097e