800947 - Uninitialised value use in nsDOMUIEvent::GetMovementPoint

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Julian Seward [:jseward]]

m-c of yesterday, x86_64-linux,
TEST_PATH=/content/html/content/test/test_bug500885.html

Error as below.  The source of the uninitialised value is listed as a
stack allocation in
nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor&) which is
pretty big, so that isn't a huge help.

Note however that the stack leading to the uninitialised value use
contains nsHTMLInputElement::PostHandleEvent, and the call out of it
is pretty close to the start of the procedure, so I'd conclude that
the uninitialised value is created by stack allocation somewhere in
between the start of nsHTMLInputElement::PostHandleEvent
(nsHTMLInputElement.cpp:2169) and line 2223 of the same function

2223        rv = shell->HandleDOMEventWithTarget(this, &actEvent, &status);

The obvious candidate is 'actEvent', line 2217.

Looking at the use point in nsDOMUIEvent::GetMovementPoint() and
reading the assembly code generated, I would suggest it is 'widget'
that is uninitialised.

  if (!mEvent ||
      !((nsGUIEvent*)mEvent)->widget ||         <<----------HERE
       (mEvent->eventStructType != NS_MOUSE_EVENT &&
        mEvent->eventStructType != NS_POPUP_EVENT &&
        mEvent->eventStructType != NS_MOUSE_SCROLL_EVENT &&
        mEvent->eventStructType != NS_WHEEL_EVENT &&
        mEvent->eventStructType != NS_DRAG_EVENT &&
        mEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT)) {
    return nsIntPoint(0, 0);
  }

Note the above is all (educated) guesswork.  #include <disclaimer.h> etc.


Conditional jump or move depends on uninitialised value(s)
   at 0x6669989: nsDOMUIEvent::GetMovementPoint() (content/events/src/nsDOMUIEvent.cpp:112)
   by 0x6669B85: nsDOMUIEvent::DuplicatePrivateData() (content/events/src/nsDOMUIEvent.cpp:373)
   by 0x6673848: nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) (content/events/src/nsEventDispatcher.cpp:656)
   by 0x63A06D0: PresShell::HandleDOMEventWithTarget(nsIContent*, nsEvent*, nsEventStatus*) (layout/base/nsPresShell.cpp:6607)
   by 0x66C0B9B: nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor&) (content/html/content/src/nsHTMLInputElement.cpp:2223)
   by 0x6671A88: nsEventTargetChainItem::PostHandleEvent(nsEventChainPostVisitor&, nsCxPusher*) (content/events/src/nsEventDispatcher.cpp:261)
   by 0x6671E38: nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) (content/events/src/nsEventDispatcher.cpp:342)
   by 0x6671F95: nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) (content/events/src/nsEventDispatcher.cpp:371)
   by 0x6673A58: nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) (content/events/src/nsEventDispatcher.cpp:634)
   by 0x63A2BF2: PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) (layout/base/nsPresShell.cpp:6439)
   by 0x63A3275: PresShell::HandleEventWithTarget(nsEvent*, nsIFrame*, nsIContent*, nsEventStatus*) (layout/base/nsPresShell.cpp:6224)
   by 0x665DB19: nsEventStateManager::CheckForAndDispatchClick(nsPresContext*, nsMouseEvent*, nsEventStatus*) (content/events/src/nsEventStateManager.cpp:4526)
   by 0x66632E0: nsEventStateManager::PostHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*) (content/events/src/nsEventStateManager.cpp:3283)
   by 0x63A286E: PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) (layout/base/nsPresShell.cpp:6463)
   by 0x63A3375: PresShell::HandlePositionedEvent(nsIFrame*, nsGUIEvent*, nsEventStatus*) (layout/base/nsPresShell.cpp:6209)
   by 0x63A44DD: PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) (layout/base/nsPresShell.cpp:6011)
   by 0x63A3D82: PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) (layout/base/nsPresShell.cpp:5726)
   by 0x6787323: nsViewManager::DispatchEvent(nsGUIEvent*, nsIView*, nsEventStatus*) (view/src/nsViewManager.cpp:767)
   by 0x6783EA1: nsView::HandleEvent(nsGUIEvent*, bool) (view/src/nsView.cpp:1062)
   by 0x6D88240: nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) (widget/gtk2/nsWindow.cpp:458)

 Uninitialised value was created by a stack allocation
   at 0x66C09DD: nsHTMLInputElement::PostHandleEvent(nsEventChainPostVisitor&) (content/html/content/src/nsHTMLInputElement.cpp:2169)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Ooops, better to make sure the struct type is actually nsGUIEvent.

Comment 2

[Julian Seward [:jseward]]

Comment on attachment 680986 [details] [diff] [review]
patch

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

With the patch in place, I no longer get the warning from valgrind.
So +1 from me.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 680986 [details] [diff] [review]
patch

r=me

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

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

Hmm, what's the NS_POPUP_EVENT? Looks like it's an obsoleted event:
http://mxr.mozilla.org/mozilla-central/search?string=NS_POPUP_EVENT&filter=[Nn]S_POPUP_EVENT

If you also think so, I'll file a bug.

And there are over 200 static_cast<ns*Event*> in m-c. I believe that almost all of the casting code are safe but the style is scary. Cannot we make safer "query event" way instead of case? If it failed and returned nullptr, that would make safer crash.