1603289 - call to function through pointer to incorrect function type in src/dom/bindings/BindingUtils.cpp:3151

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect, P2)

Description

[Tyson Smith [:tsmith]]

Found with m-c 20191211-b823b005f00e
This is triggered on start up with an UBSan build. To enable this check add the following to your mozconfig:

ac_add_options --enable-undefined-sanitizer="function"

src/dom/bindings/BindingUtils.cpp:3151:13: runtime error: call to function mozilla::dom::ChromeMessageBroadcaster_Binding::loadFrameScript(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ChromeMessageBroadcaster*, JSJitMethodCallArgs const&) through pointer to incorrect function type 'bool (*)(JSContext *, JS::Handle<JSObject *>, void *, const JSJitMethodCallArgs &)'
src/objdir-ff-ubsan/dom/bindings/MessageManagerBinding.cpp:564: note: mozilla::dom::ChromeMessageBroadcaster_Binding::loadFrameScript(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ChromeMessageBroadcaster*, JSJitMethodCallArgs const&) defined here
    #0 0x7f308b8b3316 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3151:13
    #1 0x7f309247c400 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:457:13
    #2 0x7f309247c400 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:549:12
    #3 0x7f309247d40a in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:618:10
    #4 0x7f3092466b46 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3116:16
    #5 0x7f3092449be5 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:424:10
    #6 0x7f309247c248 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) src/js/src/vm/Interpreter.cpp:590:13
    #7 0x7f309247d40a in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) src/js/src/vm/Interpreter.cpp:618:10
    #8 0x7f309247d5fd in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) src/js/src/vm/Interpreter.cpp:635:8
    #9 0x7f30926cb8fe in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2690:10
    #10 0x7f30881494e8 in nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:956:17
    #11 0x7f30859f2a50 in PrepareAndDispatch src/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:125:37
    #12 0x7f30859f182a in SharedStub (src/objdir-ff-ubsan/dist/bin/libxul.so+0x2374982a)
    #13 0x7f309217da25 in nsAppStartupNotifier::NotifyObservers(char const*) src/toolkit/xre/nsAppStartupNotifier.cpp:52:31
    #14 0x7f309216dba4 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4406:3
    #15 0x7f309216f828 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4737:8
    #16 0x7f30921703eb in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4818:21
    #17 0x5615d3ff0df2 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:217:22
    #18 0x5615d3ff0500 in main src/browser/app/nsBrowserApp.cpp:339:16

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes, this is known and purposeful. The ABI is the same, and the other option would be to have all the binding methods take a void* and immediately static-cast it to the right type, which is what ends up happening anyway in practice. We could make that change, but it's not clear what the benefit would be.

Can we somehow suppress these reports?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Talked to Tyson offline; given that this is undefined behavior and adding suppressions would likely be pretty annoying, and ot's not that hard to add the relevant casts to codegen, we can just fix this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1603289. Stop calling binding functions that take some T* via a pointer to a function type that takes void*. r=edar

We can just take void* and then cast it explicitly, which avoids the undefined-behavior issue here.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3619b209e77
Stop calling binding functions that take some T* via a pointer to a function type that takes void*.  r=edgar

Comment 5

[Cristina Coroiu [:ccoroiu]]

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