Closed Bug 1465350 Opened 2 years ago Closed 2 years ago

Memory leak when copying JSErrorReport


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: anba, Assigned: anba)



(1 file, 1 obsolete file)

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].


Test case:

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

==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== 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== 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== 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/
==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== 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== For counts of detected and suppressed errors, rerun with: -v
==9105== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Attached patch bug1465350.patch (obsolete) — Splinter Review
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(...).
Assignee: nobody → andrebargull
Attachment #8984173 - Flags: review?(jwalden+bmo)
Comment on attachment 8984173 [details] [diff] [review]

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.
Attachment #8984173 - Flags: review?(jwalden+bmo) → review-
Attached patch bug1465350.patchSplinter Review
Updated patch to return a UniquePtr from CopyErrorHelper per comment #3.
Attachment #8984173 - Attachment is obsolete: true
Attachment #8985194 - Flags: review?(jwalden+bmo)
Attachment #8985194 - Flags: review?(jwalden+bmo) → review+
Pushed by
Use UniquePtr instead of ScopedJSFreePtr for JSErrorReporter. r=Waldo
Keywords: checkin-needed
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.