Closed Bug 1330363 Opened 3 years ago Closed 3 years ago

Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:459

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + verified

People

(Reporter: decoder, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 2963cf6be7f8 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

enableSPSProfiling();
function gs(x) {}
Array.from("def", gs);
var d = Array.from.call(Date, ["A", "B"]);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
mozilla::Vector<js::jit::OptimizationAttempt, 4ul, js::jit::JitAllocPolicy>::operator[] (aIndex=<optimized out>, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/mozilla/Vector.h:459
#0  mozilla::Vector<js::jit::OptimizationAttempt, 4ul, js::jit::JitAllocPolicy>::operator[] (aIndex=<optimized out>, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/mozilla/Vector.h:459
#1  js::jit::TrackedOptimizations::trackOutcome (outcome=JS::TrackedOutcome::CantInlineNativeNoSpecialization, this=<optimized out>) at js/src/jit/OptimizationTracking.cpp:58
#2  js::jit::IonBuilder::trackOptimizationOutcomeUnchecked (this=this@entry=0x7ffff0507378, outcome=outcome@entry=JS::TrackedOutcome::CantInlineNativeNoSpecialization) at js/src/jit/OptimizationTracking.cpp:1160
#3  0x0000000000734b85 in js::jit::IonBuilder::trackOptimizationOutcome (outcome=JS::TrackedOutcome::CantInlineNativeNoSpecialization, this=0x7ffff0507378) at js/src/jit/IonBuilder.h:1144
#4  js::jit::IonBuilder::inlineNativeCall (this=this@entry=0x7ffff0507378, callInfo=..., target=0x7ffff06abe40) at js/src/jit/MCallOptimize.cpp:59
#5  0x00000000006480da in js::jit::IonBuilder::inlineSingleCall (this=this@entry=0x7ffff0507378, callInfo=..., targetArg=targetArg@entry=0x7ffff06abe40) at js/src/jit/IonBuilder.cpp:4145
#6  0x0000000000648b64 in js::jit::IonBuilder::inlineCalls (this=this@entry=0x7ffff0507378, callInfo=..., targets=..., choiceSet=..., maybeCache=<optimized out>) at js/src/jit/IonBuilder.cpp:4466
#7  0x0000000000649be2 in js::jit::IonBuilder::inlineCallsite (this=this@entry=0x7ffff0507378, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:4218
#8  0x000000000064a028 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff0507378, argc=<optimized out>, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:5161
#9  0x000000000064eb17 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff0507378, op=op@entry=JSOP_NEW) at js/src/jit/IonBuilder.cpp:1995
#10 0x00000000006500c8 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff0507378, cfgblock=cfgblock@entry=0x7ffff03392d8, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1512
#11 0x0000000000645ddb in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff0507378) at js/src/jit/IonBuilder.cpp:1433
#12 0x0000000000646ce9 in js::jit::IonBuilder::build (this=this@entry=0x7ffff0507378) at js/src/jit/IonBuilder.cpp:842
#13 0x000000000065c879 in js::jit::IonCompile (cx=cx@entry=0x7ffff695f000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x7fffffffc068, osrPc=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2280
#14 0x000000000065d2b2 in js::jit::Compile (cx=cx@entry=0x7ffff695f000, script=script@entry=..., osrFrame=osrFrame@entry=0x7fffffffc068, osrPc=osrPc@entry=0x7ffff692dab5 "\343\201C\b\377\377\377\031\346V", forceRecompile=<optimized out>) at js/src/jit/Ion.cpp:2533
#15 0x000000000065dd30 in BaselineCanEnterAtBranch (pc=0x7ffff692dab5 "\343\201C\b\377\377\377\031\346V", osrFrame=0x7fffffffc068, script=..., cx=<optimized out>) at js/src/jit/Ion.cpp:2724
#16 js::jit::IonCompileScriptForBaseline (cx=0x7ffff695f000, frame=frame@entry=0x7fffffffc068, pc=pc@entry=0x7ffff692dab5 "\343\201C\b\377\377\377\031\346V") at js/src/jit/Ion.cpp:2782
#17 0x0000000000ea4b92 in js::jit::DoWarmUpCounterFallbackOSR (cx=0x7ffff695f000, frame=0x7fffffffc068, stub=0x7ffff03361a0, infoPtr=0x7fffffffbfb0) at js/src/jit/BaselineIC.cpp:144
#18 0x00007ffff7e43334 in ?? ()
[...]
#29 0x0000000000000000 in ?? ()
rax	0x2062520	33957152
rbx	0x7fffffffb430	140737488335920
rcx	0x11b3df0	18562544
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb3c0	140737488335808
rsp	0x7fffffffb3c0	140737488335808
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff0507378	140737225192312
r13	0x7ffff06abe40	140737226915392
r14	0x1	1
r15	0x0	0
rip	0x754779 <js::jit::IonBuilder::trackOptimizationOutcomeUnchecked(JS::TrackedOutcome)+233>
=> 0x754779 <js::jit::IonBuilder::trackOptimizationOutcomeUnchecked(JS::TrackedOutcome)+233>:	movl   $0x0,0x0
   0x754784 <js::jit::IonBuilder::trackOptimizationOutcomeUnchecked(JS::TrackedOutcome)+244>:	ud2    


Marking this s-s because the assertion indicates out-of-bounds. Impact is unclear because the test seems to require profiling to be active.
Priority: -- → P1
Would it be possible to get a bisect on this?
Flags: needinfo?(choller)
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20170109044108" and the hash "c0593a4dc2012821d9799e3abfed3469f7fe41d1".
The "bad" changeset has the timestamp "20170109060808" and the hash "953bb49f3aafaea044051d11d501df0980c3d2e2".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0593a4dc2012821d9799e3abfed3469f7fe41d1&tochange=953bb49f3aafaea044051d11d501df0980c3d2e2

Hannes, is bug 1328228 a likely regressor?
Blocks: 1328228
Flags: needinfo?(choller) → needinfo?(hv1989)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Thanks. Yes looks like a likely regressor. Taking.
[Tracking Requested - why for this release]:
Tracking 53+ for this sec high issue.
What is the best way to handle this?

We have inlineNativeCall that adds optimization information w.r.t being able to inline a call.
We have IonBuilder::inlineArray which calls jsop_newarray and adds optimization information.

There is some bad interaction going on between those causing this failure.
Flags: needinfo?(hv1989) → needinfo?(shu)
(In reply to Hannes Verschore [:h4writer] from comment #6)
> What is the best way to handle this?
> 
> We have inlineNativeCall that adds optimization information w.r.t being able
> to inline a call.
> We have IonBuilder::inlineArray which calls jsop_newarray and adds
> optimization information.
> 
> There is some bad interaction going on between those causing this failure.

I see, this is unfortunate. There's no way to distinguish restarting the optimization of a bytecode vs what's going on with the inlining of new Array here: that the inlining of a call becomes equivalent to another bytecode that has its own set of optimization criteria.

The right thing to do here would be to keep both sets of optimization info somehow. The way optimization info is kept for inlining decisions right now is very naive. We assume for N call targets, there are exactly N attempts (see the amendOptimizationAttempt call in IonBuilder::inlineCalls), where the i-th index into the attempts_ vector is for the i-th target. There's no space for the NewArray sub-attempts to go. For tracking inline sites, we could do the following:

1. Keep a map of the call target index to the attempts_ index, and just store the sub-attempts in between targets. For example, suppose I have 2 call targets. Target index 0 is a NewArray and has 2 sub-attempts in for jsop_newarray. The optimization info would look like:

attempts_[0] is (Call_Inline, Inlined)
attempts_[1] is (NewArray_Foo, ...)
attempts_[2] is (NewArray_Bar, ...)
attempts_[3] is (Call_Inline, ...)

targetIndexToAttemptIndex[0] is 0
targetIndexToAttemptIndex[1] is 3

2. startTrackingOptimizations would need to know when it's being called from inlining code and not clear out the optimizations.

That's not very general and is still kind of gross. What do you think, Hannes?
Flags: needinfo?(shu)
Attached patch PatchSplinter Review
(In reply to Shu-yu Guo [:shu] from comment #7)
> (In reply to Hannes Verschore [:h4writer] from comment #6)
> > What is the best way to handle this?
> > 
> > We have inlineNativeCall that adds optimization information w.r.t being able
> > to inline a call.
> > We have IonBuilder::inlineArray which calls jsop_newarray and adds
> > optimization information.
> > 
> > There is some bad interaction going on between those causing this failure.
> 
> I see, this is unfortunate. There's no way to distinguish restarting the
> optimization of a bytecode vs what's going on with the inlining of new Array
> here: that the inlining of a call becomes equivalent to another bytecode
> that has its own set of optimization criteria.
> 
> The right thing to do here would be to keep both sets of optimization info
> somehow. The way optimization info is kept for inlining decisions right now
> is very naive. We assume for N call targets, there are exactly N attempts
> (see the amendOptimizationAttempt call in IonBuilder::inlineCalls), where
> the i-th index into the attempts_ vector is for the i-th target. There's no
> space for the NewArray sub-attempts to go. For tracking inline sites, we
> could do the following:
> 
> 1. Keep a map of the call target index to the attempts_ index, and just
> store the sub-attempts in between targets. For example, suppose I have 2
> call targets. Target index 0 is a NewArray and has 2 sub-attempts in for
> jsop_newarray. The optimization info would look like:
> 
> attempts_[0] is (Call_Inline, Inlined)
> attempts_[1] is (NewArray_Foo, ...)
> attempts_[2] is (NewArray_Bar, ...)
> attempts_[3] is (Call_Inline, ...)
> 
> targetIndexToAttemptIndex[0] is 0
> targetIndexToAttemptIndex[1] is 3
> 
> 2. startTrackingOptimizations would need to know when it's being called from
> inlining code and not clear out the optimizations.
> 
> That's not very general and is still kind of gross. What do you think,
> Hannes?

That is code I totally don't know. Not sure I want to do this the last week before a merge. Added a patch that will temporarily disable tracking the data for jsop_array and jsop_object. Which solves the issue. And do the above in a follow-up bug in FF54. I know it is quite ugly, but didn't want to spend too much effort on it.
Attachment #8827193 - Flags: review?(shu)
Comment on attachment 8827193 [details] [diff] [review]
Patch

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

I agree with the decision to fix the crash for 53. The solution I suggested isn't very quick.

::: js/src/jit/IonBuilder.cpp
@@ +5709,5 @@
> +IsCallOpcode(JSOp op)
> +{
> +    return op == JSOP_CALL || op == JSOP_CALLITER || op == JSOP_NEW || op == JSOP_SUPERCALL ||
> +           op == JSOP_EVAL || op == JSOP_STRICTEVAL;
> +}

Could you add a TODO to remind that we should need to do something better for the NewArray and NewObject optimizations in the future?
Attachment #8827193 - Flags: review?(shu) → review+
Opened bug 1331882
https://hg.mozilla.org/mozilla-central/rev/0a7d9f3e25ab
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Assignee: nobody → hv1989
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.