Closed Bug 1134074 Opened 5 years ago Closed 5 years ago

Assertion failure: isAtom(), at js/src/vm/String.h:450

Categories

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

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- affected
firefox39 --- fixed

People

(Reporter: decoder, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 09f4968d5f42 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-debug, run with --fuzzing-safe --no-threads):

function bar() {
  var n = foo().split(',');
  n[0] && {};
  bar();
}
bar();
function foo() { return "aaa,bbb,ccc"; };



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000961b0a in JSString::asAtom (this=<optimized out>) at js/src/vm/String.h:450
450	        MOZ_ASSERT(isAtom());
#0  0x0000000000961b0a in JSString::asAtom (this=<optimized out>) at js/src/vm/String.h:450
#1  0x000000000091bbe8 in js::jit::MStringLength::foldsTo (this=0x1b0a478, alloc=...) at js/src/jit/MIR.cpp:1284
#2  0x00000000009be962 in simplified (def=0x1b0a478, this=0x7fffffffaf70) at js/src/jit/ValueNumbering.cpp:620
#3  js::jit::ValueNumberer::visitDefinition (this=this@entry=0x7fffffffaf70, def=def@entry=0x1b0a478) at js/src/jit/ValueNumbering.cpp:748
#4  0x00000000009bffd3 in js::jit::ValueNumberer::visitBlock (this=this@entry=0x7fffffffaf70, block=block@entry=0x1afc9a0, dominatorRoot=dominatorRoot@entry=0x1afb830) at js/src/jit/ValueNumbering.cpp:949
#5  0x00000000009c03de in js::jit::ValueNumberer::visitDominatorTree (this=this@entry=0x7fffffffaf70, dominatorRoot=dominatorRoot@entry=0x1afb830) at js/src/jit/ValueNumbering.cpp:994
#6  0x00000000009c0753 in js::jit::ValueNumberer::visitGraph (this=this@entry=0x7fffffffaf70) at js/src/jit/ValueNumbering.cpp:1032
#7  0x00000000009c08fe in js::jit::ValueNumberer::run (this=0x7fffffffaf70, updateAliasAnalysis=<optimized out>) at js/src/jit/ValueNumbering.cpp:1103
#8  0x00000000008a61e3 in js::jit::OptimizeMIR (mir=mir@entry=0x1afadf8) at js/src/jit/Ion.cpp:1271
#9  0x00000000008be050 in js::jit::CompileBackEnd (mir=mir@entry=0x1afadf8) at js/src/jit/Ion.cpp:1567
#10 0x00000000008bea1b in js::jit::IonCompile (cx=cx@entry=0x1a0c3e0, script=0x7ffff7e5f1f0, baselineFrame=baselineFrame@entry=0x0, osrPc=0x0, constructing=false, recompile=<optimized out>, optimizationLevel=js::jit::Optimization_Normal) at js/src/jit/Ion.cpp:1939
#11 0x00000000008bedd9 in js::jit::Compile (cx=cx@entry=0x1a0c3e0, script=..., script@entry=0x7ffff7e5f1f0, osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2093
#12 0x00000000008bf202 in js::jit::CanEnter (cx=0x1a0c3e0, state=...) at js/src/jit/Ion.cpp:2232
#13 0x00000000005ffca8 in js::RunScript (cx=cx@entry=0x1a0c3e0, state=...) at js/src/vm/Interpreter.cpp:420
#14 0x000000000060057c in js::Invoke (cx=cx@entry=0x1a0c3e0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:513
#15 0x0000000000601465 in js::Invoke (cx=0x1a0c3e0, thisv=..., fval=..., argc=0, argv=<optimized out>, rval=JSVAL_VOID) at js/src/vm/Interpreter.cpp:550
#16 0x00000000008017ff in js::jit::DoCallFallback (cx=0x1a0c3e0, frame=0x7fffffffbfe8, stub_=<optimized out>, argc=0, vp=0x7fffffffbfa0, res=JSVAL_VOID) at js/src/jit/BaselineIC.cpp:9593
#17 0x00007ffff7f77435 in ?? ()
#18 0x0000000000000008 in ?? ()
#19 0x00007fffffffbf58 in ?? ()
#20 0xfff8800000000000 in ?? ()
#21 0xfff9000000000000 in ?? ()
#22 0x00000000019e1680 in js::jit::DoSpreadCallFallbackInfo ()
#23 0x00007ffff7e4f9d0 in ?? ()
#24 0x00007ffff7f7ffd4 in ?? ()
#25 0x0000000000000402 in ?? ()
#26 0x00007fffffffbfe8 in ?? ()
#27 0x0000000001b03c30 in ?? ()
#28 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x1b0a478	28353656
rcx	0x7ffff6cb2f30	140737333899056
rdx	0x0	0
rsi	0x7ffff6f86a80	140737336863360
rdi	0x7ffff6f85180	140737336856960
rbp	0x7fffffffa960	140737488333152
rsp	0x7fffffffa960	140737488333152
r8	0x7ffff7fe8740	140737354041152
r9	0x72746e65632d616c	8247338199356891500
r10	0x7fffffffa6f0	140737488332528
r11	0x7ffff6c3a940	140737333406016
r12	0x1afac70	28290160
r13	0x7fffffffaf70	140737488334704
r14	0x1afca10	28297744
r15	0x1b0a478	28353656
rip	0x961b0a <JSString::asAtom() const+28>
=> 0x961b0a <JSString::asAtom() const+28>:	movl   $0x1c2,0x0
   0x961b15 <JSString::asAtom() const+39>:	callq  0x404ac0 <abort@plt>


Marking s-s because this assertion doesn't sound unproblematic to me.
My blind guess,

The split function is inlined with IonBuilder::inlineConstantStringSplit, which creates a MNewArray which is initialized.  This MNewArray is filled with MConstant dependent-strings created by the first call of String.split in baseline.

This MNewArray is then optimized away by Scalar Replacement, which replace all the LoadElement by the value of the StoreElement.

The Apply type phase then adds the MStringLength to convert the MIRType_String for the Test instruction.

MStringLength fails, as the MConstant contains a dependent-string an not an Atom as we are used to have as a result of the parser.

I guess the following test case should reproduce this bug too:

setJitCompilerOption("ion.warmup.threshold", 30);
function bar(i) {
  if (i >= 40)
    return;
  if ("aaa,bbb,ccc".split(",")[0].length != 3)
    throw "???";
  bar(i + 1);
}
bar(0);


I guess we can safely support dependent strings (of atoms) in MStringLength::foldsTo.
Component: JavaScript Engine → JavaScript Engine: JIT
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150213092656" and the hash "af230f02ff2a".
The "bad" changeset has the timestamp "20150213093856" and the hash "0994e6eef68c".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=af230f02ff2a&tochange=0994e6eef68c
Blocks: 688219
I'm still seeing this, putting a needinfo on djvj because this might be a regression from bug 688219 which he reviewed. Also putting a needinfo on nbp based on comment 1. Can either of you two find someone to fix this? Thanks!
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(kvijayan)
I will work on it tomorrow.
Assignee: nobody → nicolas.b.pierron
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(kvijayan)
Status: NEW → ASSIGNED
Attached patch Add test case.Splinter Review
Attachment #8583251 - Flags: review?(kvijayan)
Comment on attachment 8583252 [details] [diff] [review]
IonBuilder: Atomize strings when inlining String.split.

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

Good find.
Attachment #8583252 - Flags: review?(kvijayan) → review+
Attachment #8583251 - Flags: review?(kvijayan) → review+
I briefly audited the code [1], and it seems that even if we assert that we should have an Atom, all the cases where we do these assertions can safely accept a linear string (including dependent strings which are produced by inlineConstantStringSplit).

The current patch make sure that we have an atom instead of a dependent string of an atom in the MConstant value, which fix this assertion.

I think we can safely open this bug, but as this is a small change we should still backport the patch to other branches.

[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3Ajs%2Fsrc%2Fjit+asAtom&case=true
Flags: needinfo?(jdemooij)
Opening per comment 8.
Group: core-security
Keywords: sec-moderate
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/d4cac14b98af
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1148883
No longer depends on: 1148883
You need to log in before you can comment on or make changes to this bug.