1593362 - crash near null in [@ nsFrame::HandleDrag]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20191031194708
SourceStamp=4a19c3f74d0cf11a6a2dc143b3b43a88a96f0932

Note: Mouse over the input element to speed up repro with this test case.

==3084==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000d8 (pc 0x7f7316d98cb1 bp 0x7ffed23661b0 sp 0x7ffed2366020 T0)
==3084==The signal is caused by a READ memory access.
==3084==Hint: address points to the zero page.
    #0 0x7f7316d98cb0 in GetDragState src/layout/generic/nsFrameSelection.h:406:38
    #1 0x7f7316d98cb0 in nsFrame::HandleDrag(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*) src/layout/generic/nsFrame.cpp:4791:36
    #2 0x7f7314235424 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:616:18
    #3 0x7f731423a543 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1049:11
    #4 0x7f7316aa946a in mozilla::PresShell::EventHandler::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*) src/layout/base/PresShell.cpp:8281:7
    #5 0x7f7316aa6641 in mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*) src/layout/base/PresShell.cpp:7846:7
    #6 0x7f7316a9c42b in mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) src/layout/base/PresShell.cpp:7778:17
    #7 0x7f7316a9b2d2 in mozilla::PresShell::EventHandler::HandleEventUsingCoordinates(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*, bool) src/layout/base/PresShell.cpp:6737:30
    #8 0x7f7316a98feb in mozilla::PresShell::EventHandler::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:6542:12
    #9 0x7f7316a97a2d in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:6468:23
    #10 0x7f73163e35ce in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) src/view/nsViewManager.cpp:751:18
    #11 0x7f73163e2fed in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) src/view/nsView.cpp:1064:9
    #12 0x7f731645a870 in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) src/widget/PuppetWidget.cpp:381:37
    #13 0x7f7310965faa in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) src/gfx/layers/apz/util/APZCCallbackHelper.cpp:544:21
    #14 0x7f7315ae5463 in DispatchWidgetEventViaAPZ src/dom/ipc/BrowserChild.cpp:1749:10
    #15 0x7f7315ae5463 in mozilla::dom::BrowserChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) src/dom/ipc/BrowserChild.cpp:1688:3
    #16 0x7f7315ae4352 in mozilla::dom::BrowserChild::ProcessPendingCoalescedMouseDataAndDispatchEvents() src/dom/ipc/BrowserChild.cpp:1540:7
    #17 0x7f73169ff79d in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1939:12
    #18 0x7f7316a1049e in TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
    #19 0x7f7316a1049e in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350:7
    #20 0x7f7316a0fffb in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
    #21 0x7f7316a13663 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:807:5
    #22 0x7f7316a13663 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:727:16
    #23 0x7f7316a129c7 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) src/layout/base/nsRefreshDriver.cpp:622:9
    #24 0x7f73172f5c19 in mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) src/layout/ipc/VsyncChild.cpp:65:16
    #25 0x7f730f251d16 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:187:54
    #26 0x7f730ed23541 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
    #27 0x7f730e5e7986 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2208:25
    #28 0x7f730e5e2564 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2130:9
    #29 0x7f730e5e4bb1 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1972:3
    #30 0x7f730e5e5a77 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:2003:13
    #31 0x7f730d390a63 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
    #32 0x7f730d3975c1 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #33 0x7f730e5f0a24 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:110:5
    #34 0x7f730e4eb3a2 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #35 0x7f730e4eb3a2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #36 0x7f730e4eb3a2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #37 0x7f731648a558 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #38 0x7f731a502a46 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #39 0x7f730e4eb3a2 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #40 0x7f730e4eb3a2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #41 0x7f730e4eb3a2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #42 0x7f731a502305 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #43 0x55fc87efecf0 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #44 0x55fc87efecf0 in main src/browser/app/nsBrowserApp.cpp:272:18

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session (using an UBSan build) is available here

The session will expire in 7 days.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

nsIFrame::GetFrameSelection returns nullptr in nsIFrame::HandleDrag, it comes from TextControlState::GetConstFrameSelection, but at the moment, mSelCon is nullptr since it's already been nullified in TextControlState::UnbindFromFrame which is triggered by SyncUpSelectionPropertiesBeforeDestruction call in HTMLInputElement::HandleTypeChange because the script in question changed the input type.

Hey, masayuki what does the right thing to do here? Just adding null check there?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, basically, event handlers should work with the current (latest) state. So, null checking there must be fine.

Comment 4

[Sean Voisen (:svoisen)]

Hiro: Would you be willing to add this null check?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Sure, the test case in bug 1591323 looks easier to be a crash test.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1593362 - Bail out from nsFrame::HandleDrag if the corresponding nsFrameSelection has been already destroyed. r?masayuki

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76a22f7a6a45
Bail out from nsFrame::HandleDrag if the corresponding nsFrameSelection has been already destroyed. r=masayuki

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/76a22f7a6a45

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9114011 [details]
Bug 1593362 - Bail out from nsFrame::HandleDrag if the corresponding nsFrameSelection has been already destroyed. r?masayuki

Beta/Release Uplift Approval Request

• User impact if declined: Crash
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It just adds a null check for an object which has been destroyed under certain conditions
• String changes made/needed: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9114011 [details]
Bug 1593362 - Bail out from nsFrame::HandleDrag if the corresponding nsFrameSelection has been already destroyed. r?masayuki

nullptr check, approved for 72.0b5

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/048ab46aa660