Last Comment Bug 1130672 - Crash [@ js::jit::Assembler::bind] involving oomAfterAllocations
: Crash [@ js::jit::Assembler::bind] involving oomAfterAllocations
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla44
Assigned To: Jakob Stoklund Olesen [:jolesen]
: Hannes Verschore [:h4writer]
Depends on:
Blocks: jsfunfuzz 912928
  Show dependency treegraph
Reported: 2015-02-06 17:47 PST by Gary Kwong [:gkw] [:nth10sd]
Modified: 2015-11-05 02:49 PST (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

stack (11.40 KB, text/plain)
2015-02-06 17:48 PST, Gary Kwong [:gkw] [:nth10sd]
no flags Details
ARM assembler: Check oom() before using editSrc(). (5.34 KB, patch)
2015-10-06 07:38 PDT, Jakob Stoklund Olesen [:jolesen]
jdemooij: review+
Details | Diff | Splinter Review
ARM assembler: Check oom() before using editSrc(). (5.77 KB, patch)
2015-10-07 06:00 PDT, Jakob Stoklund Olesen [:jolesen]
no flags Details | Diff | Splinter Review

Description User image Gary Kwong [:gkw] [:nth10sd] 2015-02-06 17:47:14 PST
function f(code) {
function g(code) {
function h() {

crashes js debug 32-bit ARM-simulator shell on m-c changeset aa5f8d47a0ba with --fuzzing-safe --no-threads --ion-eager at js::jit::Assembler::bind.

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect is running.
Comment 1 User image Gary Kwong [:gkw] [:nth10sd] 2015-02-06 17:48:18 PST
Created attachment 8560800 [details]

(lldb) bt 5
* thread #1: tid = 0xa69a2, 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(js::jit::RepatchLabel*) [inlined] js::jit::Instruction::encode(this=0x0000000c) const at Assembler-arm.h:1840, queue = '', stop reason = EXC_BAD_ACCESS (code=1, address=0xc)
  * frame #0: 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(js::jit::RepatchLabel*) [inlined] js::jit::Instruction::encode(this=0x0000000c) const at Assembler-arm.h:1840
    frame #1: 0x00602b63 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::Assembler::bind(this=0x0066b1ee, label=<unavailable>) + 99 at Assembler-arm.cpp:2522
    frame #2: 0x004b93c7 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`GenerateReadSlot(JSContext*, js::jit::IonScript*, js::jit::MacroAssembler&, js::jit::IonCache::StubAttacher&, JSObject*, js::NativeObject*, js::Shape*, js::jit::Register, js::jit::TypedOrValueRegister, js::jit::Label*) [inlined] js::jit::IonCache::StubAttacher::jumpRejoin(js::jit::MacroAssembler&) + 1447 at IonCaches.cpp:214
    frame #3: 0x004b9397 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`GenerateReadSlot(cx=<unavailable>, ion=<unavailable>, masm=<unavailable>, attacher=<unavailable>, obj=<unavailable>, holder=<unavailable>, shape=<unavailable>, object=<unavailable>, output=<unavailable>, failures=<unavailable>) + 1399 at IonCaches.cpp:874
    frame #4: 0x004b8141 js-dbg-32-dm-nsprBuild-armSim-darwin-aa5f8d47a0ba`js::jit::GetPropertyIC::tryAttachNative(JSContext*, JS::Handle<JSScript*>, js::jit::IonScript*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, void*, bool*) [inlined] js::jit::GetPropertyIC::object(this=0x01a0fad0, this=0x01a275b0, this=0x01a275b0) const + 929 at IonCaches.h:590
Comment 2 User image Gary Kwong [:gkw] [:nth10sd] 2015-02-06 18:34:28 PST
autoBisect didn't turn up anything useful (it was crashing/asserting at various places in the past), so setting needinfo? from Jan and/or Marty to help move this forward.
Comment 3 User image Christian Holler (:decoder) 2015-02-09 14:33:57 PST
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Comment 4 User image Jan de Mooij [:jandem] 2015-02-10 12:30:55 PST
Marty can you look at this? Based on comment 0 I assume it's ARM-only.
Comment 5 User image Gary Kwong [:gkw] [:nth10sd] 2015-04-08 14:20:25 PDT
Bouncing back to Jan - Marty is no longer paid staff with Mozilla.
Comment 6 User image Gary Kwong [:gkw] [:nth10sd] 2015-04-27 15:49:08 PDT
This still occurs as of m-c rev caf25344f73e.
Comment 7 User image Jan de Mooij [:jandem] 2015-04-30 03:26:33 PDT
Hm this is an OOM in the ARM assembler buffer :( Doug, do you know how this is supposed to work?
Comment 8 User image Douglas Crosher [:dougc] 2015-05-01 16:20:15 PDT
(In reply to Jan de Mooij [:jandem] from comment #7)
> Hm this is an OOM in the ARM assembler buffer :( Doug, do you know how this
> is supposed to work?

The ARM backend pool allocation consistency check failures were corrected and it no longer flags an oom due to these. The test case causes an allocation failure in initWithAllocator() and then later seems so be picking up a bad pointer dealing with labels. There is a comment at the allocation site "We need to wait until an AutoJitContextAlloc is created by the MacroAssembler before allocating any space.".

There is an oom flag in the assembler and this is set in many cases upon an allocation failure, and execution continues without the allocation. This looks like a foot-gun. Every use of these objects would need to be protected, and internal consistency checks adapted for the oom cases. Code dealing with labels seems to be a recurring problem for the ARM backend and OOMs. Dealing with these by throwing an exception seems more appropriate, or checking a 'success' flag and exiting the path on a failure.

There have been other OOM bugs addressed by simply crashing more gracefully when the allocation fails, see 'CrashAtUnhandlableOOM()', but this seems contrary to the strategy of flagging the OOM and continuing. If this is acceptable then perhaps this could be used to workaround this current issue too but calling this from initWithAllocator(). Or perhaps initWithAllocator() could return a 'success' flag and the caller could abort compilation if there is a failure. Perhaps the oom flag could be removed entirely and CrashAtUnhandlableOOM could be called on all these unhandled OOM cases?
Comment 9 User image Jan de Mooij [:jandem] 2015-07-29 06:03:57 PDT
Gary, do you have a newer test for this? The one in comment 0 doesn't repro anymore..
Comment 10 User image Gary Kwong [:gkw] [:nth10sd] 2015-07-29 16:07:03 PDT
autoBisect shows this is probably related to the following changeset:

The first good revision is:
user:        Jon Coppeard
date:        Fri May 22 18:52:38 2015 +0100
summary:     Bug 1155618 - Add better support for testing OOM behaviour r=terrence

Bug 1155618 probably caused the issue to go away. I guess we can mark this WFM and file a new bug should the issue occur again.
Comment 11 User image Christian Holler (:decoder) 2015-07-29 16:20:11 PDT
This issue is still active, just replace the oomAfterAllocations argument by 3 or 4 instead of 2 to reproduce the issue again.
Comment 12 User image Gary Kwong [:gkw] [:nth10sd] 2015-07-29 16:21:23 PDT
Thanks :decoder! Re-setting needinfo? for Jan.
Comment 13 User image Jakob Stoklund Olesen [:jolesen] 2015-10-06 07:38:03 PDT
Created attachment 8670257 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

The editSrc() function returns a pointer into the assembly buffer. This
may not be a valid pointer if the assembly buffer ran out of memory.

Harden AssemblerBuffer::getInst() by adding assertions. Since invalid
uses of getInst() may be security bugs, use MOZ_RELEASE_ASSERT for the
range check.
Comment 14 User image Jakob Stoklund Olesen [:jolesen] 2015-10-06 09:24:49 PDT
The included test case is not good, and should be omitted.

- dis is not available in a release build, and js dies with a reference error.
- The magic number in oomAfterAllocations(3) makes for a fragile test.
Comment 15 User image Jan de Mooij [:jandem] 2015-10-07 04:39:37 PDT
Comment on attachment 8670257 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

Review of attachment 8670257 [details] [diff] [review]:

::: js/src/jit-test/tests/ion/bug1130672.js
@@ +9,5 @@
> +        dis.z
> +}
> +f()
> +f()
> +f("oomAfterAllocations(3)")

oomAfterAllocations is only defined in debug builds. You could add something like

if (typeof oomAfterAllocations !== 'function')

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +285,5 @@
>          return getInst(off);
>      }
> +    // Get a pointer to the instruction at offset |off| which must be within the bounds of the
> +    // buffer. Use |getInstOrNull()| if |off| may be unassigned.

Nit: comments should fit within 80 columns (for code it's 99 columns).
Comment 16 User image Jakob Stoklund Olesen [:jolesen] 2015-10-07 06:00:40 PDT
Created attachment 8670764 [details] [diff] [review]
ARM assembler: Check oom() before using editSrc().

Updated patch: Removed test case per comment above. Reflowed comments to 80 col.
Comment 18 User image Carsten Book [:Tomcat] 2015-10-08 07:28:03 PDT
Comment 19 User image Jan de Mooij [:jandem] 2015-11-05 02:49:54 PST
Fixed by Jakob. Thanks!

Note You need to log in before you can comment on or make changes to this bug.