1402876 - Crash [@ js::jit::JSJitFrameIter::checkInvalidation] with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 8db0c4ecd94c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --ion-aa=flow-sensitive):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf5affb40 (LWP 20965)]
js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec, ionScriptOut=0xf5aec768) at js/src/jit/JSJitFrameIter.cpp:64
#0  js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec, ionScriptOut=0xf5aec768) at js/src/jit/JSJitFrameIter.cpp:64
#1  0x083757ad in js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec) at js/src/jit/JSJitFrameIter.cpp:42
#2  0x082f5e77 in InvalidateActivation (fop=fop@entry=0xf5fc3d60, activations=..., invalidateAll=invalidateAll@entry=false) at js/src/jit/Ion.cpp:3029
#3  0x082f6efb in js::jit::Invalidate (types=..., fop=0xf5fc3d60, invalid=..., resetUses=true, cancelOffThread=true) at js/src/jit/Ion.cpp:3184
#4  0x0833150a in js::jit::Invalidate (cancelOffThread=true, resetUses=true, invalid=..., cx=0xf5d82800) at js/src/jit/Ion.cpp:3223
#5  js::jit::Invalidate (cx=0xf5d82800, script=0xf5709438, resetUses=true, cancelOffThread=true) at js/src/jit/Ion.cpp:3269
#6  0x08afb64b in InvalidateAfterBailout (cx=0xf5d82800, outerScript=..., reason=<optimized out>) at js/src/jit/BaselineBailouts.cpp:1727
#7  0x08b00280 in HandleBoundsCheckFailure (innerScript=..., outerScript=..., cx=<optimized out>) at js/src/jit/BaselineBailouts.cpp:1740
#8  js::jit::FinishBailoutToBaseline (bailoutInfo=0x0) at js/src/jit/BaselineBailouts.cpp:2046
#9  0x2a047504 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0xd3ff0000	-738263040
ebx	0x2a08b2ae	705213102
ecx	0xf5722360	-177069216
edx	0xf573ab20	-176968928
esi	0x8dceff4	148697076
edi	0xf5aec7ec	-173094932
ebp	0xf5aec748	4121872200
esp	0xf5aec740	4121872192
eip	0x83756c3 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+99>
=> 0x83756c3 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+99>:	mov    -0x4(%ebx,%eax,1),%edx
   0x83756c7 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+103>:	mov    (%edx),%ecx


The testcase is not 100% deterministic but it reproduces fairly often for me. It seems to be sensitive to a lot of things though, like output redirection. I can't reduce it further without turning it into a 0.1% testcase :/

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Excellent find.

In CodeGenerator::link, we call InvalidateCompilerOutputsForScript if linking fails, to mark all CompilerOutputs for the script as invalid. When we're recompiling an Ion script, however, the script may still have a valid IonScript and we incorrectly invalidate its CompilerOutput.

The bug this code was trying to fix is actually already handled correctly (nbp added this code later):

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/js/src/jit/CodeGenerator.cpp#9875-9878

So we can just remove InvalidateCompilerOutputsForScript I think. That fixes the crashes for me.

Comment 3

[Jan de Mooij [:jandem]]

I'll mark this sec-moderate; we should backport this but it seems hard to exploit.

Comment 4

[Jan de Mooij [:jandem]]

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

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8912247 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Just removes some code from an OOM-handling path.
[String changes made/needed]: None.

Comment 6

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

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912247 [details] [diff] [review]
Patch

Fix a sec moderate, taking it.
For esr, I don't think it matches the expectation (sec-high or critical only)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f06912ba5bb7