1438569 - Assertion failure: obj->isSingleton(), at mozilla-central/js/src/builtin/DataViewObject.cpp:78

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: clusterfuzz-testcase-minimized-6236956762570752.js

This bug was found by Google's OSS-Fuzz running their custom internal JS fuzzer. I am refiling it in our issue tracker.

root@f19cdceaa08f:/src/mozilla-central/js/src# /out/js t.js
Assertion failure: obj->isSingleton(), at /src/mozilla-central/js/src/builtin/DataViewObject.cpp:78
AddressSanitizer:DEADLYSIGNAL
=================================================================
==3283==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000a75fd5 bp 0x7ffd2ff7f5b0 sp 0x7ffd2ff7f320 T0)
==3283==The signal is caused by a WRITE memory access.
==3283==Hint: address points to the zero page.
    #0 0xa75fd4 in js::DataViewObject::create(JSContext*, unsigned int, unsigned int, JS::Handle<js::ArrayBufferObjectMaybeShared*>, JSObject*) /src/mozilla-central/js/src/builtin/DataViewObject.cpp:124:13
    #1 0xa77463 in js::DataViewObject::constructSameCompartment(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /src/mozilla-central/js/src/builtin/DataViewObject.cpp:217:21
    #2 0xa79dea in js::DataViewObject::construct(JSContext*, unsigned int, JS::Value*) /src/mozilla-central/js/src/builtin/DataViewObject.cpp:304:12
    #3 0x9fd3c5 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/mozilla-central/js/src/vm/JSContext-inl.h:291:15
    #4 0x9fd3c5 in js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/mozilla-central/js/src/vm/JSContext-inl.h:324
    #5 0x9c0eb9 in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /src/mozilla-central/js/src/vm/Interpreter.cpp:568:20
    #6 0x98fcd7 in Interpret(JSContext*, js::RunState&) /src/mozilla-central/js/src/vm/Interpreter.cpp:3088:18
    #7 0x97f5fa in js::RunScript(JSContext*, js::RunState&) /src/mozilla-central/js/src/vm/Interpreter.cpp:423:12
    #8 0x9c3ba4 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /src/mozilla-central/js/src/vm/Interpreter.cpp:706:15
    #9 0x9c4bbf in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /src/mozilla-central/js/src/vm/Interpreter.cpp:738:12
    #10 0x1c108b2 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) /src/mozilla-central/js/src/jsapi.cpp:4714:12
    #11 0x1c10ddd in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /src/mozilla-central/js/src/jsapi.cpp:4747:12
    #12 0x5fde63 in RunFile(JSContext*, char const*, _IO_FILE*, bool) /src/mozilla-central/js/src/shell/js.cpp:822:14
    #13 0x5fde63 in Process(JSContext*, char const*, bool, FileKind) /src/mozilla-central/js/src/shell/js.cpp:1175
    #14 0x578d58 in ProcessArgs(JSContext*, js::cli::OptionParser*) /src/mozilla-central/js/src/shell/js.cpp:8470:14
    #15 0x578d58 in Shell(JSContext*, js::cli::OptionParser*, char**) /src/mozilla-central/js/src/shell/js.cpp:8856
    #16 0x578d58 in main /src/mozilla-central/js/src/shell/js.cpp:9313
    #17 0x7f816587782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #18 0x4604f8 in _start (/out/js+0x4604f8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /src/mozilla-central/js/src/builtin/DataViewObject.cpp:124:13 in js::DataViewObject::create(JSContext*, unsigned int, unsigned int, JS::Handle<js::ArrayBufferObjectMaybeShared*>, JSObject*)
==3283==ABORTING

Comment 1

[Jan de Mooij [:jandem]]

Thanks for filing this. Regression from bug 1437471; pretty sure it's just a bogus assert.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug 1437471 removed the "use singleton type for big DataViews" check in DataViewNewObjectKind, that means we can simplify DataViewObject::create a bit.

Comment 3

[André Bargull [:anba]]

Comment on attachment 8951556 [details] [diff] [review]
Patch

Review of attachment 8951556 [details] [diff] [review]:
-----------------------------------------------------------------

The change looks good to me, but now I'm wondering two things:

1. Should we inline DataViewNewObjectKind into its only caller to avoid two calls to |cx->currentScript(...)|?
2. |ObjectGroup::useSingletonForAllocationSite(script, pc, &DataViewObject::class_)| will always return |false| because of [1]. Does it make sense to change this part to an assertion?


[1] https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/js/src/vm/ObjectGroup.cpp#219

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

You're right, this code can be simplified a lot:

* The useSingletonForAllocationSite call can be removed, it's a no-op as you said and we don't optimize DataView objects in the JIT.

* The setAllocationSiteObjectGroup call is also unnecessary. What that does is give the object an ObjectGroup specific for that allocation site (instead of a group determined by proto + clasp), but for DataViews that offers no benefits at all. Allocation site groups are useful for arrays (for example) because we can distinguish arrays with different element types, but DataViews usually don't have extra properties anyway so the complexity/overhead is likely not worth it. Note that we don't use allocation site groups for most other objects either.

I think this also applies to typed arrays..

Comment 6

[André Bargull [:anba]]

Comment on attachment 8952391 [details] [diff] [review]
Patch

Review of attachment 8952391 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this code was originally for bug 762561, but if we don't (or no longer?) need it, it makes sense to remove it, because it's not a really cheap operation (see bug 1296859, comment #1 for typed arrays).

For example your patch makes this simple µ-benchmarks twice as fast! \o/
---
var ab = new ArrayBuffer(8);
var t = dateNow();
for (var i = 0; i < 1000000; ++i) {
    new DataView(ab);
}
print(dateNow() - t);
---

Comment 7

[Jan de Mooij [:jandem]]

Nice, thanks for benchmarking. I think we should consider doing the same for typed arrays; I don't think allocation site groups are very useful there either because the clasp is all we need to optimize typed arrays in Ion.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b585bd1a5168
Simplify/optimize DataViewObject::create and remove a bogus assert. r=anba

Comment 9

[Raul Gurzau (:RaulG)]

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