1827072 - Assertion failure: nbytes > 0, at gc/Nursery.cpp:1643

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[lukas.bernhard]

Attachment: crash_1.js

Steps to reproduce:

On git commit 5b0c0c8f927a9b5e88adf6283380678ef2acd604 the attached crash1.js file asserts in the js-shell when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe crash1.js . Bisecting the issue points to commit aabb183c44709472f297f4f9467ac7558a6dfba6 related to bug 1825675.
I attached a second file, crash2.js, which bisects to the same commit but produces a different backtrace. Note that crash2.js also requires a different command line to reproduce: obj-x86_64-pc-linux-gnu/dist/bin/js --baseline-warmup-threshold=10 --fuzzing-safe --nursery-strings=off crash2.js.
The fuzzers are getting swamped with this crash signature.

#0  0x00005555584fd45c in js::Nursery::registerMallocedBuffer (this=0x7ffff3a26158, buffer=0x7ffff238e120, nbytes=0)
    at js/src/gc/Nursery.cpp:1643
#1  0x0000555557cc0a70 in JSLinearString::newValidLength<(js::AllowGC)1, unsigned char> (cx=0x7ffff3a30100, chars=..., length=0, 
    heap=js::gc::DefaultHeap) at js/src/vm/StringType-inl.h:294
#2  0x0000555557e77e35 in NewString (cx=0x7ffff3a30100, argc=2, vp=0x7ffff28e3090)
    at js/src/builtin/TestingFunctions.cpp:3368
#3  0x00005555575769ce in CallJSNative (cx=0x7ffff3a30100, native=0x555557e76fe0 <NewString(JSContext*, unsigned int, JS::Value*)>, 
    reason=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:486
#4  0x00005555575761ad in js::InternalCallOrConstruct (cx=0x7ffff3a30100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:580
#5  0x00005555575775a1 in InternalCall (cx=0x7ffff3a30100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:647
#6  0x0000555557577365 in js::CallFromStack (cx=0x7ffff3a30100, args=..., reason=js::CallReason::Call)
    at js/src/vm/Interpreter.cpp:652
#7  0x0000555557589dd4 in js::Interpret (cx=0x7ffff3a30100, state=...) at js/src/vm/Interpreter.cpp:3395
#8  0x00005555575758ee in MaybeEnterInterpreterTrampoline (cx=0x7ffff3a30100, state=...)
    at js/src/vm/Interpreter.cpp:400
#9  0x0000555557574e40 in js::RunScript (cx=0x7ffff3a30100, state=...) at js/src/vm/Interpreter.cpp:458
#10 0x000055555757915c in js::ExecuteKernel (cx=0x7ffff3a30100, script=..., envChainArg=..., evalInFrame=..., result=...)
    at js/src/vm/Interpreter.cpp:845
#11 0x0000555557579a05 in js::Execute (cx=0x7ffff3a30100, script=..., envChain=..., rval=...)
    at js/src/vm/Interpreter.cpp:877
#12 0x00005555577db9b6 in ExecuteScript (cx=0x7ffff3a30100, envChain=..., script=..., rval=...)
    at js/src/vm/CompilationAndEvaluation.cpp:472
#13 0x00005555577dbb0d in JS_ExecuteScript (cx=0x7ffff3a30100, scriptArg=...)
    at js/src/vm/CompilationAndEvaluation.cpp:496
#14 0x00005555573b6faf in RunFile (cx=0x7ffff3a30100, filename=0x7fffffffea51 "modifiedStuff/crash_2023_04_08_1.js", file=0x7ffff7769b60, 
    compileMethod=CompileUtf8::DontInflate, compileOnly=false, fullParse=false) at js/src/shell/js.cpp:1098
#15 0x00005555573b6855 in Process (cx=0x7ffff3a30100, filename=0x7fffffffea51 "modifiedStuff/crash_2023_04_08_1.js", forceTTY=false, 
    kind=FileScript) at js/src/shell/js.cpp:1697
#16 0x0000555557390638 in ProcessArgs (cx=0x7ffff3a30100, op=0x7fffffffe460) at js/src/shell/js.cpp:10591
#17 0x000055555737f4f3 in Shell (cx=0x7ffff3a30100, op=0x7fffffffe460) at js/src/shell/js.cpp:10815
#18 0x000055555737a516 in main (argc=3, argv=0x7fffffffe6c8) at js/src/shell/js.cpp:11247

Comment 1

[lukas.bernhard]

Attachment: crash_2.js

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1827072 - Prevent test function NewString() from creating invalid strings.

Comment 3

[Steve Fink [:sfink] [:s:]]

Two different problems, both in the additions I made to the NewString testing function. The first can be hit by this simplified test:

newString("", { capacity: 1 });

The problem is that we don't normally create empty strings with malloc buffers. That should just be forbidden.

The second can be hit by:

newString("x", { capacity: 2, tenured: true });

the problem is that JSLinearString::makeExtensible was not updating the memory accounting information for the née-linear-now-extensible string.

The new path followed by newString(..., { capacity: ... }) code is uncomfortably low-level, and it is possible that more things will crop up.

Comment 4

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

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/873ca890a187
Prevent test function NewString() from creating invalid strings. r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for causing nightly as release bustages.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Crunning%2Cpending%2Crunnable&searchStr=nightlyasrelease&fromchange=cc0fc1a741f70ab4172b424677d4f4099f2f8311&selectedTaskRun=T9DD3c0DTxKy3vY_KRa9cA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=412402869&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/45b88f506236d92edca34135c2ca626e863f3c6f

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1cac0c28fbc5
Backed out changeset 873ca890a187 for causing nightly as release bustages. CLOSED TREE

Comment 9

[Steve Fink [:sfink] [:s:]]

Maybe I'm failing to see something obvious, but, uh, wtf?

The error is:
hidden symbol '_ZN14JSLinearString14makeExtensibleEm' isn't defined

The patch here moves the implementation out of line to the StringType.cpp file. If I am understanding correctly, it looks like the initial build and profiling run of firefox must have succeeded, but the pgo link failed for the js, jsapi-tests, and libxul.so targets.

If I look at a similar symbol _ZNK14JSLinearString23dumpRepresentationCharsERN2js14GenericPrinterEi, it seems like it shouldn't be any different. And locally (with a non-pgo build), they're both generated as global .hidden symbols in the .o, linked to produce local .hidden symbols in libxul.so.

Am I doing something wrong?

Comment 10

[Steve Fink [:sfink] [:s:]]

Note that I am able to reproduce the linker error on a pgo build locally, using a somewhat mismatched downloaded profdata.

Comment 11

[Steve Fink [:sfink] [:s:]]

Emilio clued me in by pointing out the obvious—I put the implementation inside of an #ifdef region that I didn't notice.

Comment 12

[Steve Fink [:sfink] [:s:]]

(I still don't understand why it seems like the profile run built successfully. It's somehow using different flags, I guess?)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb122f5af0d7
Prevent test function NewString() from creating invalid strings. r=jandem

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c061ec6c908e
Backed out changeset fb122f5af0d7 for causing SM bustages. CLOSED TREE

Comment 15

[Cosmin Sabou [:CosminS]]

Backed out changeset for causing SM bustages.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=fb122f5af0d770bb83c163a793b43bfb01ef2223&searchStr=SM&selectedTaskRun=e2BtXAmeSW-rYALo57Ue1Q.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=412575065&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/c061ec6c908e819440f660b1cc305149d0732ff9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/911580015303
Prevent test function NewString() from creating invalid strings. r=jandem

Comment 17

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/911580015303

Comment 18

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

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

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

For more information, please visit auto_nag documentation.

Comment 19

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9327683 [details]
Bug 1827072 - Prevent test function NewString() from creating invalid strings.

Beta/Release Uplift Approval Request

• User impact if declined: None. This was a regression from a fix for bug 1825675, which itself appears to be unreachable without the test function being modified here. I am requesting uplift because this will get in the way of fuzzing, pretty badly.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changes a test function only.
• String changes made/needed:
• Is Android affected?: Yes

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9327683 [details]
Bug 1827072 - Prevent test function NewString() from creating invalid strings.

Approved for 113.0b5.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/970d34a3bbbb