1264961 - Assertion failure: !inFrameMaps(frame), at js/src/vm/Debugger-inl.h:25 with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 10f66b316457 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe):

loadFile(`
  var o = {}
  var global = this;
  var p = new Proxy(o, {
    "deleteProperty": function (await , key) {
      var g = newGlobal();
      g.parent = global;
      g.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
    }
  })
  for (var i=0; i<100; i++);
  assertEq(delete p.foo, true);
`);
function loadFile(lfVarx) {
    oomTest(function() {
        eval(lfVarx);
    })
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000716e42 in js::Debugger::onLeaveFrame (cx=cx@entry=0x7ffff6908800, frame=..., pc=pc@entry=0x7ffff31c7b25 "%", ok=ok@entry=false) at js/src/vm/Debugger-inl.h:25
#0  0x0000000000716e42 in js::Debugger::onLeaveFrame (cx=cx@entry=0x7ffff6908800, frame=..., pc=pc@entry=0x7ffff31c7b25 "%", ok=ok@entry=false) at js/src/vm/Debugger-inl.h:25
#1  0x00000000006ff590 in HandleExceptionBaseline (pc=0x7ffff31c7b25 "%", rfe=<optimized out>, frame=..., cx=0x7ffff6908800) at js/src/jit/JitFrames.cpp:691
#2  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:837
#3  0x00007ffff7fe8608 in ?? ()
[...]
#23 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x0	0
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffa1e0	140737488331232
rsp	0x7fffffffa1b0	140737488331184
r8	0x7ffff7fdf7c0	140737354004416
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7ffff6f76be0	140737336798176
r11	0x0	0
r12	0x0	0
r13	0x7ffff6908800	140737330055168
r14	0x7ffff31c7b25	140737272118053
r15	0x7fffffffa8e0	140737488333024
rip	0x716e42 <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+1010>
=> 0x716e42 <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+1010>:	movl   $0x19,0x0
   0x716e4d <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+1021>:	callq  0x4ab5c0 <abort()>

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151013053056" and the hash "8d9c20c241be7d7b3cfa90a3368a77db42172781".
The "bad" changeset has the timestamp "20151013054956" and the hash "d80f9d6921f8209ef01aa730be9a97ab727704d1".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d9c20c241be7d7b3cfa90a3368a77db42172781&tochange=d80f9d6921f8209ef01aa730be9a97ab727704d1

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, I tried to use OOM_VERBOSE=1 to get that stack, but every time I run this, the allocation numbers are different so I'm not sure how many allocations to skip via "continue -i <number>".

Once you have the correct OOM_VERBOSE stack I guess you could needinfo? the Debugger folks...

Comment 3

[Jon Coppeard (:jonco) (PTO until 3rd April)]

The allocation this fails on seems to change and I don't know why that is.

Given the assertion though, I think the problem might be in Debugger::replaceFrameGuts() which can exit early on OOM without processing every frame.

Comment 4

[Jim Blandy :jimb]

Passing the buck: replaceFrameGuts is Shu's brainchild.

Comment 5

[Shu-yu Guo [:shu]]

I get a different assertion when running this test locally:

(Unable to print stack trace)
Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?), at /home/shu/moz/central/js/src/builtin/TestingFunctions.cpp:1324
fish: “dist/bin/js crash.js” terminated by signal SIGSEGV (Address boundary error)

Comment 6

[Shu-yu Guo [:shu]]

First, OOMTest doesn't play nice with the way Debugger works. A Debugger handler could throw OOM, and it'll get handled by the Debugger as an uncaught exception. If there's no user-set handler for uncaught exceptions, that exception will be reported, cleared, and the code returns false [1]. This trips up the OOMTest assertion.

Jon, how should we handle this?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp?from=handleUncaughtExceptionHelper#1161-1220

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Fix OOM case in Debugger::replaceFrameGuts.

Unfortuantely, I don't actually know how to convert the fuzz test to a
non-infinite looping test.

Comment 8

[Jim Blandy :jimb]

Comment on attachment 8743105 [details] [diff] [review]
Fix OOM case in Debugger::replaceFrameGuts.

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

I'm torn; this really does need a test. For these fuzzer OOMs I'm inclined to just let the patch land, even untested, because I don't think they're going to be encountered much in practice; the existing tests should exercise the paths adequately.

Comment 9

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Shu-yu Guo [:shu] from comment #6)
If you pass --fuzzing-safe, it should disable the assert in oomTest() that checks whether an exception is raised when the code returns false.

Comment 10

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Jon Coppeard (:jonco) from comment #9)
Also you can pass |false| as the second argument to oomTest() to get the same effect.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Jon Coppeard (:jonco) from comment #9)
> (In reply to Shu-yu Guo [:shu] from comment #6)
> If you pass --fuzzing-safe, it should disable the assert in oomTest() that
> checks whether an exception is raised when the code returns false.

Ah, wonderful, thank you.

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #8)
> Comment on attachment 8743105 [details] [diff] [review]
> Fix OOM case in Debugger::replaceFrameGuts.
> 
> Review of attachment 8743105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm torn; this really does need a test. For these fuzzer OOMs I'm inclined
> to just let the patch land, even untested, because I don't think they're
> going to be encountered much in practice; the existing tests should exercise
> the paths adequately.

And I really want to check in a test too. I don't know how to check in something that terminates on success yet.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4826513cafc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4826513cafc