1856739 - Assertion failure: !linearStr->isPermanentAtom() when creating 25+ chars substring from permanent atom

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Tooru Fujisawa [:arai]]

(this would be the same issue as bug 1856295, but filing as separate bug given this affects already-in-tree code)

The following code hits assertion failure.

var s = "SetIsInlinableLargeFunction".substring(1); gc();

Assertion failure: !linearStr->isPermanentAtom(), at /Users/arai/projects/mozilla-unified/js/src/gc/TraceMethods-inl.h:111
#01: void js::GCMarker::eagerlyMarkChildren<0u>(JSLinearString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf0926c]
#02: void js::GCMarker::eagerlyMarkChildren<0u>(JSString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf08ff0]
#03: void js::GCMarker::scanChildren<0u, JSString>(JSString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf08fa4]
#04: void js::GCMarker::traverse<0u>(JSString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf08e94]
#05: void js::GCMarker::markAndTraverse<0u, JSString>(JSString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xedb810]
#06: void js::GCMarker::markAndTraverseEdge<0u, JSObject*, JSString>(JSObject*, JSString*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf0fc9c]
#07: bool js::GCMarker::processMarkStackTop<0u>(js::SliceBudget&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf0ed64]
#08: bool js::GCMarker::markOneColor<0u, (js::gc::MarkColor)2>(js::SliceBudget&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xf0e5dc]
#09: bool js::GCMarker::doMarking<0u>(js::SliceBudget&, js::gc::ShouldReportMarkTime)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xee2494]
#10: js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::gc::ShouldReportMarkTime)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xee20c4]
#11: js::gc::GCRuntime::markUntilBudgetExhausted(js::SliceBudget&, js::gc::GCRuntime::ParallelMarking, js::gc::ShouldReportMarkTime)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xe708c4]
#12: js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, JS::GCReason, bool)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xe741ec]
#13: js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xe77074]
#14: js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xe781ac]
#15: js::gc::GCRuntime::gc(JS::GCOptions, JS::GCReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xe48380]
#16: JS::NonIncrementalGC(JSContext*, JS::GCOptions, JS::GCReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xec0b60]
#17: GC(JSContext*, unsigned int, JS::Value*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x870440]
#18: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13eb2c]
#19: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13e4b8]
#20: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13f47c]
#21: js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13f2bc]
#22: js::Interpret(JSContext*, js::RunState&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x14e2ec]
#23: MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13ded4]
#24: js::RunScript(JSContext*, js::RunState&)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x13d72c]
#25: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x140a74]
#26: js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x14110c]
#27: ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x326230]
#28: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x325fe4]
#29: EvalUtf8AndPrint(JSContext*, char const*, unsigned long, int, bool)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xcd2bc]
#30: ReadEvalPrintLoop(JSContext*, __sFILE*, bool)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xcbed4]
#31: Process(JSContext*, char const*, bool, FileKind)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0xcafa4]
#32: ProcessArgs(JSContext*, js::cli::OptionParser*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x5a6b4]
#33: Shell(JSContext*, js::cli::OptionParser*)[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x15610]
#34: main[/Users/arai/projects/mozilla-unified/obj/sm-pds/dist/bin/js +0x1012c]

this doesn't happen for short permananent atom like "allowContentIter", but maybe ~24 chars or longer substring hits the assertion, possibly because not inlinable

Comment 1

[Tooru Fujisawa [:arai]]

So, there's no guard against using permanent atom as dependent string base.

https://searchfox.org/mozilla-central/rev/d895cf273d8cdceb4256f561c1ad1bf91135202a/js/src/vm/StringType-inl.h#208-210

MOZ_ALWAYS_INLINE JSLinearString* JSDependentString::new_(
    JSContext* cx, JSLinearString* baseArg, size_t start, size_t length,
    js::gc::Heap heap) {

is the assertion really valid?
what could go wrong if a permanent atom is used as base?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Tooru Fujisawa [:arai] from comment #1)
Looking into this some more, there is no reason we can't have permanent atoms as the base of a dependent string. The assertion was added for a different reason - to replace an if statement which was no longer necessary. However the assertion doesn't hold and can be removed.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1755534

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1856739 - Remove assertion that dependent strings do not have a permanent atom as a base r?sfink

This assertion is unnecessary and doesn't hold. It was added to replace a check
for permanent atoms which is now handled by the mark check, since permanent
atoms are now always marked.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c113362a137
Remove assertion that dependent strings do not have a permanent atom as a base r=sfink

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9c113362a137

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #8)
This does not affect release builds so we can let this ride the trains.