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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

=== 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?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 3

[Jeff Walden [:Waldo]]

"".split("") looks arai-ish, notwithstanding the bisection result.

Comment 4

[Jeff Walden [:Waldo]]

(Although I guess NewArray in stack/bisection result is also pretty compelling.  Anyway, eyes can't hurt.)

Comment 5

[Tooru Fujisawa [:arai]]

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);

Comment 6

[Tooru Fujisawa [:arai]]

bug 1266870 might be related to this.

Comment 7

[Tooru Fujisawa [:arai]]

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;
> -}

Comment 8

[Tooru Fujisawa [:arai]]

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

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Use jsop_newarray in inlineConstantStringSplitString.

Replaced MConstant::NewConstraintlessObject+MNewArray::New with jsop_newarray,
and added isResumePoint() check before emitting a resume point.

Comment 10

[Hannes Verschore [:h4writer]]

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.

Comment 11

[Carsten Book [:Tomcat]]

also bughunter found this on http://www.audiocircle.com/index.php?topic=110274.0

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/054453fc28a53bc1aac8a76e5707aa604b37ceb6
Bug 1266579 - Use jsop_newarray in inlineConstantStringSplitString. r=h4writer

Comment 13

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

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

Comment 14

[Tooru Fujisawa [:arai]]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8744544 [details] [diff] [review]
Use jsop_newarray in inlineConstantStringSplitString.

Improve aurora stability, taking it.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88c0b4446910

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx48

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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.