1420525 - UBSan: load of value which is not a valid value 'bool' [@ mozilla::EventStateManager::UpdateCursor]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

This was found with a Firefox build built with -fsanitize=bool

/dom/events/EventStateManager.cpp:3738:31: runtime error: load of value 163, which is not a valid value for type 'bool'
    #0 mozilla::EventStateManager::UpdateCursor(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsEventStatus*) /dom/events/EventStateManager.cpp:3738:31
    #1 mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*) /dom/events/EventStateManager.cpp:726:5
    #2 mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool) /layout/base/PresShell.cpp:7802:19
    #3 mozilla::PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) /layout/base/PresShell.cpp:7596:10
    #4 mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) /layout/base/PresShell.cpp:7394:12
    #5 nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) /view/nsViewManager.cpp:812:14
    #6 nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) /view/nsView.cpp:1140:9
    #7 mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) /widget/PuppetWidget.cpp:395:35
    #8 mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) /gfx/layers/apz/util/APZCCallbackHelper.cpp:499:21
    #9 mozilla::dom::TabChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /dom/ipc/TabChild.cpp:1810:3
    #10 mozilla::dom::TabChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /dom/ipc/TabChild.cpp:1777:3
    #11 mozilla::dom::TabChild::RecvSynthMouseMoveEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /dom/ipc/TabChild.cpp:1738:8
    #12 mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /objdir-ff-asan-O2/ipc/ipdl/PBrowserChild.cpp:3442:20
    #13 mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /objdir-ff-asan-O2/ipc/ipdl/PContentChild.cpp:4930:28
    #14 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /ipc/glue/MessageChannel.cpp:2114:25
    #15 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /ipc/glue/MessageChannel.cpp:2044:17
    #16 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /ipc/glue/MessageChannel.cpp:1890:5
    #17 mozilla::ipc::MessageChannel::MessageTask::Run() /ipc/glue/MessageChannel.cpp:1923:15
    #18 mozilla::SchedulerGroup::Runnable::Run() /xpcom/threads/SchedulerGroup.cpp:396:25
    #19 nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1037:14
    #20 NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:513:10
    #21 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:97:21
    #22 RunHandler /ipc/chromium/src/base/message_loop.cc:319:3
    #23 MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:299
    #24 nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:159:27
    #25 XRE_RunAppShell() /toolkit/xre/nsEmbedFunctions.cpp:877:22
    #26 RunHandler /ipc/chromium/src/base/message_loop.cc:319:3
    #27 MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:299
    #28 XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:703:34
    #29 content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #30 main /browser/app/nsBrowserApp.cpp:280
    #31 __libc_start_main /build/glibc-CxtIbX/glibc-2.26/csu/../csu/libc-start.c:308
    #32 _start (firefox+0x41efc9)

Comment 1

[Andrew Overholt [:overholt]]

Xidorn wrote much of the Cursor-related code in EventStateManager::UpdateCursor in bug 1200469 so maybe he has an idea of what is going on here?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Could you provide which revision are you using? It seems EventStateManager.cpp:3738 is no longer in EventStateManager::UpdateCursor now, so it's not clear to me what line causes this.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

OK I guess it was just the last revision 4 days ago. At that point, EventStateManager.cpp:3738 points to
> haveHotspot = framecursor.mHaveHotspot;

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

OK, so this is a matter of reading uninitialized value. nsHTMLFramesetFrame::GetCursor doesn't call nsFrame::GetCursor, and it doesn't fill all fields of nsIFrame::Cursor, and thus there are uninitialized values after GetCursor returns.

The fix should be easy. We can just put some initializers into nsIFrame::Cursor.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1420525 - Initialize nsIFrame::Cursor to avoid uninitialized value inside.

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

Comment 6

[Andrew Overholt [:overholt]]

Hsin-Yi, do you know anyone on your team with the time to take this while Xidorn is less available (despite him kindly responding here :)?

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Well, I'm responding :) I may leave for pto a day or two before all hands but most of time I should be around.

I expect this to be something trivial so I think I can handle that myself.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #7)
> Well, I'm responding :) I may leave for pto a day or two before all hands
> but most of time I should be around.
> 
> I expect this to be something trivial so I think I can handle that myself.
Cool! Do feel free to let me know how my team could help when you see needs, Xidorn. :)

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8932659 [details]
Bug 1420525 - Initialize nsIFrame::Cursor to avoid uninitialized value inside.

https://reviewboard.mozilla.org/r/203710/#review209266

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0840b7be4498
Initialize nsIFrame::Cursor to avoid uninitialized value inside. r=tnikkel

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0840b7be4498