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

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Unassigned)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla48
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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()>

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
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
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...
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard) → needinfo?(jimb)

Comment 4

2 years ago
Passing the buck: replaceFrameGuts is Shu's brainchild.
Flags: needinfo?(jimb) → needinfo?(shu)

Comment 5

2 years ago
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

2 years ago
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
Flags: needinfo?(jcoppeard)

Comment 7

2 years ago
Created attachment 8743105 [details] [diff] [review]
Fix OOM case in Debugger::replaceFrameGuts.

Unfortuantely, I don't actually know how to convert the fuzz test to a
non-infinite looping test.
Attachment #8743105 - Flags: review?(jimb)

Updated

2 years ago
Flags: needinfo?(shu)

Comment 8

2 years ago
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.
Attachment #8743105 - Flags: review?(jimb) → review+
(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.
Flags: needinfo?(jcoppeard)
(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

2 years ago
(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

2 years ago
(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 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4826513cafc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.