Closed Bug 1811171 Opened 2 years ago Closed 2 years ago

LeakSanitizer: [@ js::jit::IonScript::New] with finally

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

All
Linux
defect

Tracking

()

RESOLVED FIXED
111 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox109 --- wontfix
firefox110 --- wontfix
firefox111 --- fixed

People

(Reporter: gkw, Assigned: iain)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, testcase)

Attachments

(2 files)

(function () {
  (function () {
    function* f(z) {
      try {
        yield 1;
      } finally {
        for (let i = 0; i < 99; i++) {}
        var x = Math.pow(1, z);
      }
    }
    for (let i = 0; i < 99; i++) {
      let y = f(1);
      y.next();
      y.return();
    }
  })();
})();
==806==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 49039 byte(s) in 89 object(s) allocated from:
    #0 0x5608b5ec5b9e in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x5608b5fb33c3 in js_arena_malloc(unsigned long, unsigned long) /home/gen16gx500/shell-cache/js-64-asan-linux-x86_64-60b4965aa0ca/objdir-js/dist/include/js/Utility.h:366:10
    #2 0x5608b5fb33c3 in unsigned char* js_pod_arena_malloc<unsigned char>(unsigned long, unsigned long) /home/gen16gx500/shell-cache/js-64-asan-linux-x86_64-60b4965aa0ca/objdir-js/dist/include/js/Utility.h:576:26
    #3 0x5608b5fb33c3 in unsigned char* js::MallocProvider<JSContext>::maybe_pod_arena_malloc<unsigned char>(unsigned long, unsigned long) /home/gen16gx500/trees/mozilla-central/js/src/vm/MallocProvider.h:57:12
    #4 0x5608b5fb33c3 in unsigned char* js::MallocProvider<JSContext>::pod_arena_malloc<unsigned char>(unsigned long, unsigned long) /home/gen16gx500/trees/mozilla-central/js/src/vm/MallocProvider.h:109:12
    #5 0x5608b5fb33c3 in unsigned char* js::MallocProvider<JSContext>::pod_malloc<unsigned char>(unsigned long) /home/gen16gx500/trees/mozilla-central/js/src/vm/MallocProvider.h:127:12
    #6 0x5608b8239a0f in js::jit::IonScript::New(JSContext*, js::IonCompilationId, unsigned int, unsigned int, unsigned int, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:663:19
    #7 0x5608b81cb4e8 in js::jit::CodeGenerator::link(JSContext*, js::jit::WarpSnapshot const*) /home/gen16gx500/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:13318:26
    #8 0x5608b825d1fb in LinkCodeGen(JSContext*, js::jit::CodeGenerator*, JS::Handle<JSScript*>, js::jit::WarpSnapshot const*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:295:17
    #9 0x5608b825d1fb in js::jit::IonCompile(JSContext*, JS::Handle<JSScript*>, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1642:17
    #10 0x5608b825d1fb in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:1800:24
    #11 0x5608b825eb07 in BaselineCanEnterAtBranch(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2001:25
    #12 0x5608b825eb07 in IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2052:12
    #13 0x5608b825f073 in js::jit::IonCompileScriptForBaselineOSR(JSContext*, js::jit::BaselineFrame*, unsigned int, unsigned char*, js::jit::IonOsrTempData**) /home/gen16gx500/trees/mozilla-central/js/src/jit/Ion.cpp:2164:8
    #14 0x1b35ef8dbdec  (<unknown module>)
    #15 0x1b35ef8ea553  (<unknown module>)
    #16 0x1b35ef8eb4b5  (<unknown module>)
    #17 0x1b35ef8d84ed  (<unknown module>)
    #18 0x5608b82cd36b in EnterJit(JSContext*, js::RunState&, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:107:5
    #19 0x5608b82cd36b in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:205:10
    #20 0x5608b6226001 in js::RunScript(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:421:32
    #21 0x5608b62508a9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:579:13
    #22 0x5608b7809d7c in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/gen16gx500/trees/mozilla-central/js/src/jit/BaselineIC.cpp:1591:10
    #23 0x1b35ef8dada8  (<unknown module>)
    #24 0x1b35ef8e87b3  (<unknown module>)
    #25 0x1b35ef8d84ed  (<unknown module>)
    #26 0x5608b82cd36b in EnterJit(JSContext*, js::RunState&, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:107:5
    #27 0x5608b82cd36b in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:205:10
    #28 0x5608b6226001 in js::RunScript(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:421:32
    #29 0x5608b62508a9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:579:13
    #30 0x5608b7809d7c in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/gen16gx500/trees/mozilla-central/js/src/jit/BaselineIC.cpp:1591:10
    #31 0x1b35ef8dada8  (<unknown module>)
    #32 0x1b35ef8e814a  (<unknown module>)
    #33 0x1b35ef8d84ed  (<unknown module>)
    #34 0x5608b82cd36b in EnterJit(JSContext*, js::RunState&, unsigned char*) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:107:5
    #35 0x5608b82cd36b in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/jit/Jit.cpp:205:10
    #36 0x5608b6226001 in js::RunScript(JSContext*, js::RunState&) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:421:32
    #37 0x5608b6255521 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:812:13
    #38 0x5608b6255521 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) /home/gen16gx500/trees/mozilla-central/js/src/vm/Interpreter.cpp:844:10
    #39 0x5608b6489833 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/gen16gx500/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:473:10
    #40 0x5608b6489c2b in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /home/gen16gx500/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:497:10

SUMMARY: AddressSanitizer: 49039 byte(s) leaked in 89 allocation(s).
The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f5e7bea40350
user:        Iain Ireland
date:        Wed Mar 30 23:19:48 2022 +0000
summary:     Bug 885514: Support finally in Warp r=jandem

Run with --fuzzing-safe --no-threads --ion-eager (with ASAN_OPTIONS=detect_leaks=1), compile with AR=ar sh ../configure --enable-address-sanitizer --enable-fuzzing --disable-jemalloc --disable-stdcxx-compat --without-sysroot --with-ccache --enable-nspr-build --enable-ctypes --enable-debug-symbols --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 60b4965aa0ca.

Guessing likely not s-s. Iain, is bug 885514 a likely regressor?

Flags: needinfo?(iireland)

Set release status flags based on info from the regressing bug 885514

It looks like bug 1762770, which was a follow-up to bug 885514, is the real culprit. I believe this testcase will also trigger the bug we fixed in 1762770, which may have confused mozregression.

This bug is a bit of a mess. We call .return() on a generator. This is implemented by throwing the magic GeneratorClosing value. We resume (baseline) execution in the finally block. We OSR back into Ion because of the loop in the finally block.

On the next iteration of the outer loop, when we call return, we once again throw GeneratorClosing. In OnLeaveIonFrame, we rematerialize the frame to recover the return value and the generator object. To do so, we call SnapshotIterator::initInstructionResults, which doesn't find existing frame recovery data in the activation and invalidates the IonScript.

The frame is still technically on the stack, so we don't delete the IonScript immediately. There is code in HandleException that is intended to decrement the invalidation count when we return from an invalidated stack frame. However, we did not expect to invalidate a script in the middle of HandleExceptionIon, so we check whether the IonScript has been invalidated outside the loop. Therefore, we never decrement the invalidation count for that IonScript, and it is never freed.

We could do a quick fix to handle the case where we invalidate while handling the exception, but if possible I think it's better to rewrite OnLeaveIonFrame to avoid invalidating the IonScript in the first place. I will look into it.

Assignee: nobody → iireland
Flags: needinfo?(iireland)
Regressed by: 1762770
No longer regressed by: 885514

Awkwardly, it looks like we actually depend on looking up the return value in the rematerialized frame (and in particular the fact that the rematerialized frame is persistent) for forced returns using the debugger. My first attempt at a fix was to read the return value and environment chain out of the snapshot directly to avoid rematerializing the frame, but that breaks the connection between OnLeaveIonFrame and PropagateForcedReturn in bug1756592.js. So I'll have to try something else.

Blocks: sm-security
Severity: -- → S3
Priority: -- → P3
Attached file Bug 1811171: Update comment r=nbp (deleted) —

After some discussion, we came to the conclusion that this code still needs to exist, but the comment is misleading. Hopefully this describes the justification better.

Depends on D168001

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: