Assertion failure: args[0].isObject(), at builtin/intl/NumberFormat.cpp:1258
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox-esr91 | --- | unaffected |
firefox91 | --- | unaffected |
firefox92 | --- | wontfix |
firefox93 | --- | verified |
People
(Reporter: decoder, Assigned: tcampbell)
References
(Regression)
Details
(4 keywords, Whiteboard: [bugmon:update,bisected,confirmed][adv-main93+r])
Attachments
(4 files)
The following testcase crashes on mozilla-central revision 20210824-7857f4c37a92 (debug build, run with --fuzzing-safe --no-threads --fast-warmup):
function main() {
v3 = [0, 13.37];
function v10(v11,v12) {
try {
v15 = v10(-3399895606,..."64",7);
v16 = (-65537).toLocaleString();
} catch(v17) {
v18 = [...v3];
}
}
v10();
}
main();
Backtrace:
received signal SIGSEGV, Segmentation fault.
#0 0x00005555571f1f9d in js::intl_FormatNumber(JSContext*, unsigned int, JS::Value*) ()
#1 0x0000555556c17801 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#2 0x0000555556c16f36 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#3 0x0000555556c18371 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) ()
#4 0x000055555766dd14 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) ()
#5 0x0000385d3548f933 in ?? ()
#6 0x0000000000000000 in ?? ()
rax 0x5555557deb7c 93824994896764
rbx 0x7ffff5a4e800 140737314613248
rcx 0x555558128820 93825038190624
rdx 0x0 0
rsi 0x7ffff7105770 140737338431344
rdi 0x7ffff7104540 140737338426688
rbp 0x7fffffffb6f0 140737488336624
rsp 0x7fffffffb3a0 140737488335776
r8 0x7ffff7105770 140737338431344
r9 0x7ffff7f98840 140737353713728
r10 0x0 0
r11 0x0 0
r12 0x7ffff5a4e800 140737314613248
r13 0x7fffffffb940 140737488337216
r14 0x7ffff6019000 140737320685568
r15 0xffff800000000000 -140737488355328
rip 0x5555571f1f9d <js::intl_FormatNumber(JSContext*, unsigned int, JS::Value*)+6749>
=> 0x5555571f1f9d <_ZN2js17intl_FormatNumberEP9JSContextjPN2JS5ValueE+6749>: movl $0x4ea,0x0
0x5555571f1fa8 <_ZN2js17intl_FormatNumberEP9JSContextjPN2JS5ValueE+6760>: callq 0x555556b06a2a <abort>
Marking this s-s since it requires the JIT and could be a type confusion.
Reporter | ||
Comment 1•3 years ago
|
||
Reporter | ||
Comment 2•3 years ago
|
||
Comment 4•3 years ago
|
||
Bugmon Analysis
Verified bug as reproducible on mozilla-central 20210824094724-7857f4c37a92.
The bug appears to have been introduced in the following build range:
Start: 8c52592cd83801dac261477f24d21c6c0ab31c47 (20210812164534)
End: 4792c28298b34db879dc527667f9f7820fbaffa8 (20210812171257)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8c52592cd83801dac261477f24d21c6c0ab31c47&tochange=4792c28298b34db879dc527667f9f7820fbaffa8
Reporter | ||
Comment 5•3 years ago
|
||
Less reduced testcase that doesn't reproduce without evaluate but within reprl (for investigation):
//evaluate(`
function placeholder(){}
function main() {
const v3 = [13.37,13.37,13.37];
const v5 = [1337];
const v6 = {__proto__:v3,b:v5,constructor:v3,e:"-65537",length:1337,valueOf:v5};
const v7 = {b:-2,c:"-65537",constructor:13.37,toString:v6};
function v10(v11,v12) {
try {
const v15 = v10(-3399895606,v11,..."64",7);
const v16 = (-65537).toLocaleString();
} catch(v17) {
const v18 = [v17,v12,v11,v7,...v3,v10];
}
}
const v19 = v10();
gc();
}
main();
//`);
Comment 6•3 years ago
|
||
I'm able to trigger the assertion already at the parent commit (8c52592cd838) of the patch stack from bug 1725379, so I think the regression range from comment #4 is wrong.
The first test case starts triggering the assertion with https://hg.mozilla.org/integration/autoland/rev/c4541cb343ae0ee222dc89f5f09d7758f503fd89 for me, which doesn't really make sense. The second test case asserts starting with the changes from bug 1688794, which seems like a more plausible candidate.
Comment 7•3 years ago
|
||
I'm unable to reproduce this (both central tip and the revision in comment 0) with these tests. I'll be off Thu/Fri this week but can look at this more on Monday...
Comment 8•3 years ago
|
||
I'm only able to reproduce with opt+debug. I don't get the assertion in non-opt debug builds. Both tests are using recursion, maybe that's also making it system dependent?
Assignee | ||
Comment 9•3 years ago
|
||
Using the rev from Comment 6 I'm now able to reproduce locally as well. I've simplified (for my local build at least) to:
function fn() {
try {
v15 = fn(..." ");
v16 = (0).toLocaleString();
} catch(v17) {
[...[0, 1]];
}
}
fn();
Recursion, iterators, and intl-selfhosting seem to be required. The self-hosted stencil changes may be to blame here since they do special things when resolving some intl self-hosted code
Assignee | ||
Comment 10•3 years ago
|
||
I'm seeing surprising behaviour while debugging and I think this is a real regression from Bug 1688794.
Updated•3 years ago
|
Comment 11•3 years ago
|
||
It could also be an WarpBuilder inlining issue, because I don't get an assertion when passing --ion-inlining=off
.
It looks like somehow the numberFormatCache
is wiped, because I get the following output after applying this patch:
Self-hosted JavaScript assertion info: [Latin 1]"./../builtin/Number.js:34: en-US -> undefined"
diff --git a/js/src/builtin/Number.js b/js/src/builtin/Number.js
--- a/js/src/builtin/Number.js
+++ b/js/src/builtin/Number.js
@@ -27,5 +27,9 @@ function Number_toLocaleString() {
// locales and options.
- if (!intl_IsRuntimeDefaultLocale(numberFormatCache.runtimeDefaultLocale)) {
+ var defaultLocale = numberFormatCache.runtimeDefaultLocale;
+ if (!intl_IsRuntimeDefaultLocale(defaultLocale)) {
numberFormatCache.numberFormat = intl_NumberFormat(locales, options);
numberFormatCache.runtimeDefaultLocale = intl_RuntimeDefaultLocale();
+ } else {
+ assert(defaultLocale === numberFormatCache.runtimeDefaultLocale,
+ `${defaultLocale} -> ${numberFormatCache.runtimeDefaultLocale}`);
}
Assignee | ||
Comment 12•3 years ago
|
||
If I add a re-entrancy check to GetComputedIntrinsic
(which I should have added originally..), then the test case fails in debug builds. With the JITs we seem to trigger re-entrancy that does not occur without them. Still digging..
Assignee | ||
Comment 13•3 years ago
|
||
The good news is that this is not a JIT type-confusion bug. What is happening is that GetComputedIntrinsic
can fail while running JS_ExecuteScript
. In this test case, an unlucky stack-overflow is what causes the failure. This results in only some of the computed self-hosted values being initialized. As a result, when we later try to access one of the uninitialized values it causes GetComputedIntrinsic
to run again which resets the runtimeDefaultLocale
back to an empty object. This is occurs in the middle of running the format code which breaks things. I think that in practice the spectre XOR checks protect us by making this crash.
I'm not sure what the best fix is for this problem. A hack might be to have SetIntrinsic
be a no-op if the value is already initialized. None of this is very elegant though.
Another approach might be to have SetIntrinsic
store to a temporary object and then apply those values to the real holder after successfully executing the top-level.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 14•3 years ago
|
||
Capture SetIntrinsic
values on a special computed-intrinsics-holder. If
running the top-level script fails (for OOM, etc), we can throw away the partial
results before they are exposed. Once the computed-intrinsics-holder if
successfully initialized, we can copy values to the normal intrinsics holder as
needed so there is only one cache to search. There are only about 20 values and
only ones that are actually used exist in both tables, so memory change is
minimal.
Assignee | ||
Comment 15•3 years ago
|
||
Depends on D123994
Assignee | ||
Comment 16•3 years ago
|
||
This bug results in a UndefinedValue
being passed to a C++ internal helper method that assumes it receives ObjectValue
. Due to our spectre mitigations, the JS::Value::toObject
call will convert undefined into a nullptr (32-bit) or outside-address-space (64-bit) pointer that will derefence to a reliable crash. The regression was added to Fx92 and I would like to call this sec-low; mark 92 as wontfix; and land patch today-ish. The fix is a slight design change, so I'd rather ride trains than uplift if spectre mitigations are already protecting us. I will defer landing test case either way.
Andrew, is this reasonable?
Comment 17•3 years ago
|
||
Sure, that sounds fine to me. Thanks for fixing this.
Comment 18•3 years ago
|
||
Use dedicated holder object for JSOp::SetIntrinsic. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b4d43ddeed4fb972dc4c5023c743a234565ac2ff
https://hg.mozilla.org/mozilla-central/rev/b4d43ddeed4f
Comment 19•3 years ago
|
||
Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20210901034805-a14cf6b742de.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.
Updated•3 years ago
|
Updated•2 years ago
|
Comment 20•2 years ago
|
||
Comment 21•2 years ago
|
||
bugherder |
Description
•