Assertion failure: !ntemplate->hasDynamicElements(), at js/src/jit/MacroAssembler.cpp:1083

VERIFIED FIXED in Firefox 48

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: arai)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla49
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 verified, firefox49 verified)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment)

Reporter

Description

3 years ago
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

3 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

3 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
"".split("") looks arai-ish, notwithstanding the bisection result.
(Although I guess NewArray in stack/bisection result is also pretty compelling.  Anyway, eyes can't hurt.)
Assignee

Comment 5

3 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

3 years ago
bug 1266870 might be related to this.
Assignee

Comment 7

3 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

3 years ago
I think IonBuilder::inlineConstantStringSplitString should also be changed to use jsop_newarray instead of directly calling MNewArray::New.
Assignee

Comment 9

3 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 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+
Flags: needinfo?(hv1989)
Assignee

Comment 12

3 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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee: nobody → arai.unmht
Assignee

Comment 14

3 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?
Duplicate of this bug: 1266870
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::De…
Keywords: crash, topcrash
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

3 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

3 years ago
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx48
Group: javascript-core-security → core-security-release
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>] [@
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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.