1436240 - UBSan: load of value which is not a valid value for type 'bool' in /layout/style/MediaQueryList.cpp:78

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Tyson Smith [:tsmith]]

This seems to be triggered after a few minutes with regular browsing.

Found in mozilla-central changeset: 402372:3df7961bad2c. Built with -fsanitize=bool

I am guessing this is actually uninitialized memory but I'm not 100%.

/layout/style/MediaQueryList.cpp:78:10: runtime error: load of value 228, which is not a valid value for type 'bool'
    #0 0x7fa22e2ef382 in mozilla::dom::MediaQueryList::Matches() /layout/style/MediaQueryList.cpp:78:10
    #1 0x7fa22aa4686c in mozilla::dom::MediaQueryListBinding::get_matches(JSContext*, JS::Handle<JSObject*>, mozilla::dom::MediaQueryList*, JSJitGetterCallArgs) /objdir-ff-ubsan/dom/bindings/MediaQueryListBinding.cpp:66:21
    #2 0x7fa22bfe64aa in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /dom/bindings/BindingUtils.cpp:2906:13
    #3 0x7fa23446a88e in CallJSNative /js/src/jscntxtinlines.h:291:15
    #4 0x7fa23446a88e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /js/src/vm/Interpreter.cpp:473
    #5 0x7fa23446b4cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) /js/src/vm/Interpreter.cpp:522:12
    #6 0x7fa23446b6aa in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:541:10
    #7 0x7fa23446c898 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:656:12
    #8 0x7fa2353fc5f5 in CallGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.cpp:2145:16
    #9 0x7fa2353dc4c1 in GetExistingProperty<js::AllowGC::CanGC> /js/src/vm/NativeObject.cpp:2198:12
    #10 0x7fa2353dc4c1 in NativeGetPropertyInline<js::AllowGC::CanGC> /js/src/vm/NativeObject.cpp:2401
    #11 0x7fa2353dc4c1 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.cpp:2437
    #12 0x7fa2343d2eb4 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /js/src/vm/NativeObject.h:1630:12
    #13 0x7fa2343d2cc7 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) /js/src/jsobj.h:822:12
    #14 0x7fa234472593 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:4405:12
    #15 0x7fa234482647 in GetPropertyOperation(JSContext*, js::InterpreterFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:219:12
    #16 0x7fa23443f114 in Interpret(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:2815:10
    #17 0x7fa234433428 in js::RunScript(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:423:12
    #18 0x7fa23446a779 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /js/src/vm/Interpreter.cpp:495:15
    #19 0x7fa23446b4cf in InternalCall(JSContext*, js::AnyInvokeArgs const&) /js/src/vm/Interpreter.cpp:522:12
    #20 0x7fa23446b6aa in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /js/src/vm/Interpreter.cpp:541:10
    #21 0x7fa234d5f64e in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /js/src/jit/VMFunctions.cpp:112:12
    #22 0x7fa234d60125 in js::jit::InvokeFromInterpreterStub(JSContext*, js::jit::InterpreterStubExitFrameLayout*) /js/src/jit/VMFunctions.cpp:141:10
    #23 0x7fa1c8b5df76  (<unknown module>)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1436240: Make sure to always initialize mMatches to something meaningful.

Review commit: https://reviewboard.mozilla.org/r/218272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218272/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8948878 [details]
Bug 1436240: Make sure to always initialize mMatches to something meaningful.

https://reviewboard.mozilla.org/r/218272/#review224060

May make more sense to have it be `Maybe<bool>`, but keeping them separate is probably fine as well, especially given there is code depending on their being separate.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

But actually, the question is why would this happen at all? The value is protected by mMatchesValid, so we shouldn't be reading mMatches as bool when mMatchesValid is false.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> Comment on attachment 8948878 [details]
> Bug 1436240: Make sure to always initialize mMatches to something meaningful.
> 
> https://reviewboard.mozilla.org/r/218272/#review224060
> 
> May make more sense to have it be `Maybe<bool>`, but keeping them separate
> is probably fine as well, especially given there is code depending on their
> being separate.

Yeah, maybe... I went for the cheap fix here I admit :)

Comment 5

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> But actually, the question is why would this happen at all? The value is
> protected by mMatchesValid, so we shouldn't be reading mMatches as bool when
> mMatchesValid is false.

The key is that RecomputeMatches bails out in a bunch of places without setting mMatches, then don't bother checking, like in ::Matches()

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8948878 [details]
Bug 1436240: Make sure to always initialize mMatches to something meaningful.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218272/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d71fadfe8e78
Make sure to always initialize mMatches to something meaningful. r=xidorn

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> > But actually, the question is why would this happen at all? The value is
> > protected by mMatchesValid, so we shouldn't be reading mMatches as bool when
> > mMatchesValid is false.
> 
> The key is that RecomputeMatches bails out in a bunch of places without
> setting mMatches, then don't bother checking, like in ::Matches()

Oh, hmmm, that makes sense then.

Comment 9

[Dorel Luca [:dluca]]

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