Closed
Bug 1266579
Opened 8 years ago
Closed 8 years ago
Assertion failure: !ntemplate->hasDynamicElements(), at js/src/jit/MacroAssembler.cpp:1083
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | verified |
firefox49 | --- | verified |
People
(Reporter: gkw, Assigned: arai)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(1 file)
3.19 KB,
patch
|
h4writer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 1152d99d8c53 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager): for (var i = 0; i < 1; i++) { "8pan08pa8pan08pa".split('') } Backtrace: 0 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x00000001003e2fdd js::jit::MacroAssembler::initGCThing(js::jit::Register, js::jit::Register, JSObject*, bool, bool) + 1741 (MacroAssembler.cpp:1083) 1 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x00000001002415e0 js::jit::CodeGenerator::visitNewArray(js::jit::LNewArray*) + 432 (CodeGenerator.cpp:5199) 2 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x00000001002404bf js::jit::CodeGenerator::generateBody() + 1135 (LIR.h:733) 3 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x000000010025adca js::jit::CodeGenerator::generate() + 458 (CodeGenerator.cpp:8848) 4 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x0000000100298510 js::jit::GenerateCode(js::jit::MIRGenerator*, js::jit::LIRGraph*) + 272 (Ion.cpp:1947) 5 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x00000001002985b5 js::jit::CompileBackEnd(js::jit::MIRGenerator*) + 101 (Ion.cpp:1969) 6 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x000000010029a2b2 js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, bool) + 4210 (Ion.cpp:2229) 7 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x000000010029adae js::jit::IonCompileScriptForBaseline(JSContext*, js::jit::BaselineFrame*, unsigned char*) + 1102 (Ion.cpp:2584) 8 js-dbg-64-dm-clang-darwin-1152d99d8c53 0x00000001001d0a4c js::jit::DoWarmUpCounterFallbackOSR(JSContext*, js::jit::BaselineFrame*, js::jit::ICWarmUpCounter_Fallback*, js::jit::IonOsrTempData**) + 204 (BaselineIC.cpp:142) 9 ??? 0x0000000101ef87c5 0 + 4327442373 10 ??? 0x0000000105051890 0 + 4379187344 LIR/MIR is on the stack - setting s-s to be safe as a start.
Reporter | ||
Comment 1•8 years ago
|
||
=== Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20160420031344" and the hash "beca0a8904718c01dfe57a758ce2ceccc028dc2b". The "bad" changeset has the timestamp "20160420034139" and the hash "7c1f8d3d4f69add3995d27d4c70c92d286aa54b9". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=beca0a8904718c01dfe57a758ce2ceccc028dc2b&tochange=7c1f8d3d4f69add3995d27d4c70c92d286aa54b9 Hannes, which of the bugs in the list is the likely regressor?
Flags: needinfo?(hv1989)
Keywords: crash
Reporter | ||
Comment 2•8 years ago
|
||
Nevermind, I got a better bisection result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/be2f6cb7251c user: Hannes Verschore date: Wed Apr 20 06:40:16 2016 -0400 summary: Bug 1241088: SharedStubs - part 2: port NewArray and NewObject shared stubs to work in ion, r=efaust Hannes, is bug 1241088 a likely regressor?
Blocks: 1241088
Comment 3•8 years ago
|
||
"".split("") looks arai-ish, notwithstanding the bisection result.
Comment 4•8 years ago
|
||
(Although I guess NewArray in stack/bisection result is also pretty compelling. Anyway, eyes can't hurt.)
Assignee | ||
Comment 5•8 years ago
|
||
Looks like, templteObject created in TryAttachStringSplit doesn't suffice the following assertion: https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/js/src/jit/MacroAssembler.cpp#1083 > MOZ_ASSERT_IF(!ntemplate->denseElementsAreCopyOnWrite(), !ntemplate->hasDynamicElements()); The templateObject is created here. https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/js/src/jit/BaselineIC.cpp#5875 > RootedValue arr(cx); > > // Copy the array before storing in stub. > if (!CopyArray(cx, obj, &arr)) > return false; > > // Atomize all elements of the array. > RootedObject arrObj(cx, &arr.toObject()); > uint32_t initLength = GetAnyBoxedOrUnboxedArrayLength(arrObj); > for (uint32_t i = 0; i < initLength; i++) { > JSAtom* str = js::AtomizeString(cx, GetAnyBoxedOrUnboxedDenseElement(arrObj, i).toString()); > if (!str) > return false; > > if (!SetAnyBoxedOrUnboxedDenseElement(cx, arrObj, i, StringValue(str))) { > // The value could not be stored to an unboxed dense element. > return true; > } > } and passed to MNewArray here. https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/js/src/jit/MCallOptimize.cpp#1453 > JSObject* templateObject = nullptr; > if (!inspector->isOptimizableCallStringSplit(pc, &stringStr, &stringSep, &templateObject)) > return InliningStatus_NotInlined; > ... > MConstant* templateConst = MConstant::NewConstraintlessObject(alloc(), templateObject); > current->add(templateConst); > > MNewArray* ins = MNewArray::New(alloc(), constraints(), initLength, templateConst, > templateObject->group()->initialHeap(constraints()), pc);
Assignee | ||
Comment 6•8 years ago
|
||
bug 1266870 might be related to this.
Assignee | ||
Comment 7•8 years ago
|
||
Previously that MNewArray was using visitNewArrayCallVM because of lir->mir()->shouldUseVM(). > - if (lir->mir()->shouldUseVM()) { > + if (!templateObject) { > visitNewArrayCallVM(lir); > return; > } > bool > -MNewArray::shouldUseVM() const > -{ > - if (!templateObject()) > - return true; > - > - if (templateObject()->is<UnboxedArrayObject>()) { > - MOZ_ASSERT(templateObject()->as<UnboxedArrayObject>().capacity() >= length()); > - return !templateObject()->as<UnboxedArrayObject>().hasInlineElements(); > - } > - > - MOZ_ASSERT(length() <= NativeObject::MAX_DENSE_ELEMENTS_COUNT); > - > - size_t arraySlots = > - gc::GetGCKindSlots(templateObject()->asTenured().getAllocKind()) - ObjectElements::VALUES_PER_HEADER; > - > - return length() > arraySlots; > -}
Assignee | ||
Comment 8•8 years ago
|
||
I think IonBuilder::inlineConstantStringSplitString should also be changed to use jsop_newarray instead of directly calling MNewArray::New.
Assignee | ||
Comment 9•8 years ago
|
||
Replaced MConstant::NewConstraintlessObject+MNewArray::New with jsop_newarray, and added isResumePoint() check before emitting a resume point.
Attachment #8744544 -
Flags: review?(hv1989)
Comment 10•8 years ago
|
||
Comment on attachment 8744544 [details] [diff] [review] Use jsop_newarray in inlineConstantStringSplitString. Review of attachment 8744544 [details] [diff] [review]: ----------------------------------------------------------------- Good find! I indeed forgot to convert it to jsop_newarray there.
Attachment #8744544 -
Flags: review?(hv1989) → review+
Updated•8 years ago
|
Flags: needinfo?(hv1989)
Comment 11•8 years ago
|
||
also bughunter found this on http://www.audiocircle.com/index.php?topic=110274.0
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/054453fc28a53bc1aac8a76e5707aa604b37ceb6 Bug 1266579 - Use jsop_newarray in inlineConstantStringSplitString. r=h4writer
This missed the merge to m-c before 48 moved to aurora, so you'll need to request an uplift. https://hg.mozilla.org/mozilla-central/rev/054453fc28a5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Assignee: nobody → arai.unmht
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8744544 [details] [diff] [review] Use jsop_newarray in inlineConstantStringSplitString. Approval Request Comment [Feature/regressing bug #]: bug 1241088 [User impact if declined]: browser crashes by opening certain website (comment #11) [Describe test coverage new/current, TreeHerder]: tested on mozilla-central treeherder [Risks and why]: low, this applies a same change that is in bug 1241088 patch to one more places that is missed by the patch there. [String/UUID change made/needed]: None
Attachment #8744544 -
Flags: approval-mozilla-aurora?
Comment 16•8 years ago
|
||
Comment on attachment 8744544 [details] [diff] [review] Use jsop_newarray in inlineConstantStringSplitString. Improve aurora stability, taking it.
Attachment #8744544 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::AtomizeString]
[@ js::StringBuffer::append]
[@ JSRope::flattenInternal<T>]
[@ JSFlatString* JSRope::flattenInternal<T>]
[@ mozilla::detail::VectorImpl<T>::copyConstruct<T>]
[@ JS::DispatchTraceKindTyped<T>]
[@ js::DispatchTyped<T>]
[@ → [@ js::AtomizeString]
[@ js::StringBuffer::append]
[@ JSRope::flattenInternal<T>]
[@ JSFlatString* JSRope::flattenInternal<T>]
[@ mozilla::detail::VectorImpl<T>::copyConstruct<T>]
[@ JS::DispatchTraceKindTyped<T>]
[@ js::DispatchTyped<T>]
[@
Comment 18•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx48
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Crash Signature: [@ js::AtomizeString]
[@ js::StringBuffer::append]
[@ JSRope::flattenInternal<T>]
[@ JSFlatString* JSRope::flattenInternal<T>]
[@ mozilla::detail::VectorImpl<T>::copyConstruct<T>]
[@ JS::DispatchTraceKindTyped<T>]
[@ js::DispatchTyped<T>]
[@ → [@ js::AtomizeString]
[@ js::StringBuffer::append]
[@ JSRope::flattenInternal<T>]
[@ JSFlatString* JSRope::flattenInternal<T>]
[@ mozilla::detail::VectorImpl<T>::copyConstruct<T>]
[@ JS::DispatchTraceKindTyped<T>]
[@ js::DispatchTyped<T>]
[@
Comment 19•8 years ago
|
||
This affected more than one version (48 and 49). This should have had sec-approval before it went in and a rating. Luckily, 47 is unaffected so I'll just walk away now.
status-firefox47:
--- → unaffected
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•