Bug 18464 – incorrect translation of wxMouse event to webkit mouse event

• Component: WebKit wx
• Status: RESOLVED FIXED

[Alexander Vassilev]

The constructor of PlatformMouseEvent in the wx port does not set the m_button member of the PlatformMouseEvent object on mouse button release event. THis causes assertion failure in WebCore which os observer as an access violation at address 0xbbadbeef when the yser clicks inside the client area of the wxBrowser test application. THis is observed only in the mingw port, in the msvc port doesnt happen, probably because the assertion is disabled there. THe attached patch fixes this as well as does more complete event translation from wxMouseEvent to webkit mouse event

[Alexander Vassilev]

Created attachment 20502 [details]
patch to MouseEventWx.cpp

[Alexander Vassilev]

Created attachment 20503 [details]
fixed patch

oops, the patch was reversed, this is the fixed patch

[Kevin Ollivier]

Could you provide more information on the assertion you get, and any stack trace you have? I think we should first try to understand what is causing the problem on MinGW, because in my reading of the original code, the code to set the button is run regardless of the event type. Your code does add a timestamp, and explicitly sets the click count for zero for a mouse move event, and perhaps either of these somehow fixes the problem? 

Also, the assertions are not disabled under MSVC8. They are under MSVC7 but wxWebKit is actually used in Windows applications built using MSVC8, and they do not cause this assertion. So there is something specific with the MinGW port happening, and I would like to figure out exactly what is triggering this problem before approving a fix. 

[Kevin Ollivier]

Mouse event crash stack trace (moved from bug #18465:

Program received signal SIGSEGV, Segmentation fault.
0x0050c42e in WebCore::EventTargetNode::dispatchMouseEvent (this=0xfc56f30, 
    event=@0x22f57c, eventType=@0x1086dcc, detail=1, relatedTarget=0x0)
    at dom/EventTargetNode.cpp:195
195         ASSERT(event.eventType() == MouseEventMoved || button != NoButton);
Current language:  auto; currently c++
(gdb) bt
#0  0x0050c42e in WebCore::EventTargetNode::dispatchMouseEvent (
    this=0xfc56f30, event=@0x22f57c, eventType=@0x1086dcc, detail=1, 
    relatedTarget=0x0) at dom/EventTargetNode.cpp:195
#1  0x0043b602 in WebCore::EventHandler::dispatchMouseEvent (this=0xfc3de70, 
    eventType=@0x1086dcc, targetNode=0xfc56f30, cancelable=true, clickCount=1, 
    mouseEvent=@0x22f57c, setUnder=false) at page/EventHandler.cpp:1262
#2  0x0043a4df in WebCore::EventHandler::handleMouseReleaseEvent (
    this=0xfc3de70, mouseEvent=@0x22f57c) at page/EventHandler.cpp:1079
#3  0x0040cbf7 in wxWebView::OnMouseEvents (this=0xfc3c638, event=@0x22f6bc)
    at WebView.cpp:483
#4  0x6cec7285 in wxEvtHandler::ProcessEventIfMatches (entry=@0x1085328, 
    handler=0xfc3c638, event=@0x22f6bc) at ../include/wx/app.h:287
#5  0x6cec75dc in wxEventHashTable::HandleEvent (this=0x1, event=@0x22f6bc, 
    self=0xfc3c638) at ../include/wx/event.h:2319
#6  0x6cec85d9 in wxEvtHandler::ProcessEvent (this=0xfc3c638, event=@0x22f6bc)
    at ../src/common/event.cpp:1287
#7  0x628d6996 in wxWindow::HandleMouseEvent (this=0xfc3c638, msg=167, x=167, 
    y=167, flags=167) at ../include/wx/window.h:612
#8  0x628d95c1 in wxWindow::MSWWindowProc (this=0xfc3c638, message=514, 
    wParam=0, lParam=9109751) at ../src/msw/window.cpp:2860
#9  0x628d1830 in wxWndProc (hWnd=0x7a0b20, message=514, wParam=0, 
    lParam=9109751) at ../src/msw/window.cpp:2594
#10 0x7e418734 in USER32!GetDC () from /cygdrive/c/WINDOWS/system32/user32.dll
#11 0x007a0b20 in WebCore::HTMLTokenizer::HTMLTokenizer (this=0x628d1780, 
    doc=0x7a0b20, reportErrors=220) at html/HTMLTokenizer.cpp:169
#12 0x7e418816 in USER32!GetDC () from /cygdrive/c/WINDOWS/system32/user32.dll
#13 0x628d1780 in wxWindowCreationHook::~wxWindowCreationHook ()
    at ../src/msw/window.cpp:2566
#14 0x7e4189cd in USER32!GetWindowLongW ()
---Type <return> to continue, or q <return> to quit---
   from /cygdrive/c/WINDOWS/system32/user32.dll
#15 0x00000000 in ?? ()
(gdb) 

[Kevin Ollivier]

Looking at the stack trace you posted (please make sure to post the trace to the relevant bug in the future, as this is a record of all our debugging work, etc.), it looks to me like the assertion is fired due to a bug in your patch.

handleMouseReleaseEvent is triggering ASSERT(event.eventType() == MouseEventMoved || button != NoButton). Since it's a mouse release event, the eventType() should not be MouseEventMoved, so that leaves button != NoButton. And in your code, you do not set a button for the wxEVT_*_DCLICK case, so it makes sense that this assertion would get triggered. Are you getting this crash on an unmodified copy of MouseEventWx.cpp, or on your modified copy?

[Alexander Vassilev]

(In reply to comment #5)
> Looking at the stack trace you posted (please make sure to post the trace to
> the relevant bug in the future, as this is a record of all our debugging work,
> etc.), it looks to me like the assertion is fired due to a bug in your patch.
> 
> handleMouseReleaseEvent is triggering ASSERT(event.eventType() ==
> MouseEventMoved || button != NoButton). Since it's a mouse release event, the
> eventType() should not be MouseEventMoved, so that leaves button != NoButton.
> And in your code, you do not set a button for the wxEVT_*_DCLICK case, so it
> makes sense that this assertion would get triggered. Are you getting this crash
> on an unmodified copy of MouseEventWx.cpp, or on your modified copy?
> 
No no, I am getting the crash on an unmodified copy! After my patch I get no more crashes any more, and all works fine! Here is the original code:

PlatformMouseEvent::PlatformMouseEvent(const wxMouseEvent& event, const wxPoint& globalPoint)
    : m_position(event.GetPosition())
    , m_globalPosition(globalPoint)
    , m_clickCount(event.ButtonDClick() ? 2 : 1)
    , m_shiftKey(event.ShiftDown())
    , m_ctrlKey(event.CmdDown())
    , m_altKey(event.AltDown())
    , m_metaKey(event.MetaDown()) // FIXME: We'll have to test other browsers
{
    wxEventType type = event.GetEventType();
    m_eventType = MouseEventMoved;
    
    if (type == wxEVT_LEFT_DOWN || type == wxEVT_MIDDLE_DOWN || type == wxEVT_RIGHT_DOWN)
        m_eventType = MouseEventPressed;
    
    else if (type == wxEVT_LEFT_UP || type == wxEVT_MIDDLE_UP || type == wxEVT_RIGHT_UP || 
                type == wxEVT_LEFT_DCLICK || type == wxEVT_MIDDLE_DCLICK || type == wxEVT_RIGHT_DCLICK)
        m_eventType = MouseEventReleased;

    else if (type == wxEVT_MOTION)
        m_eventType = MouseEventMoved;

    if (event.LeftIsDown())
        m_button = LeftButton;
    else if (event.RightIsDown())
        m_button = RightButton;
    else if (event.MiddleIsDown())
        m_button = MiddleButton;
}

So you can see that m_button is set ONLY on mouse down events, not on mouse release or dclick events! What triggers the crash is exactly mouse release or doubleclick event.  You are completely right that my patch leaved m_button unset on mouse doubleclick, I figured out this few days ago and fixed it as well, but didn't want to flood with patches. So now I will attach my complete patch

[Alexander Vassilev]

Created attachment 20626 [details]
new fixed MouseEventWx.cpp patch

Here is the new fixed patch to MouseEventWx.cpp that should handle all types of mouse events correctly

[Alexander Vassilev]

Comment on attachment 20626 [details]
new fixed MouseEventWx.cpp patch

--- ./webkit_mingw/WebCore/platform/wx/MouseEventWx.cpp	2008-04-04 15:02:52.968750000 +0300
+++ ./webkit_clean/WebCore/platform/wx/MouseEventWx.cpp	2008-04-14 05:04:15.191625000 +0300
@@ -25,7 +25,7 @@
 
 #include "config.h"
 #include "PlatformMouseEvent.h"
-
+#include "SystemTime.h"
 #include <wx/defs.h>
 #include <wx/event.h>
 
@@ -34,7 +34,6 @@
 PlatformMouseEvent::PlatformMouseEvent(const wxMouseEvent& event, const wxPoint& globalPoint)
     : m_position(event.GetPosition())
     , m_globalPosition(globalPoint)
-    , m_clickCount(event.ButtonDClick() ? 2 : 1)
     , m_shiftKey(event.ShiftDown())
     , m_ctrlKey(event.CmdDown())
     , m_altKey(event.AltDown())
@@ -43,22 +42,58 @@
     wxEventType type = event.GetEventType();
     m_eventType = MouseEventMoved;
     
-    if (type == wxEVT_LEFT_DOWN || type == wxEVT_MIDDLE_DOWN || type == wxEVT_RIGHT_DOWN)
-        m_eventType = MouseEventPressed;
-    
-    else if (type == wxEVT_LEFT_UP || type == wxEVT_MIDDLE_UP || type == wxEVT_RIGHT_UP || 
-                type == wxEVT_LEFT_DCLICK || type == wxEVT_MIDDLE_DCLICK || type == wxEVT_RIGHT_DCLICK)
-        m_eventType = MouseEventReleased;
+//strange, the compiler doeswnt allow us to use a switch here,  with wxEVT_xxx cases, even though they are of the enum type of the 'type' variable
+//so we use if-s instead
+	if (type == wxEVT_LEFT_DOWN)
+	  {
+		m_eventType = MouseEventPressed;
+		m_button = LeftButton;
+	  }
+	else if (type == wxEVT_LEFT_UP)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = LeftButton; 
+	  }
+	else if (type == wxEVT_RIGHT_DOWN)
+	  {
+		m_eventType = MouseEventPressed;
+		m_button = RightButton;
+	  }
+	else if (type == wxEVT_RIGHT_UP)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = RightButton;
+	  }
+	else if (type == wxEVT_MIDDLE_DOWN)
+	  {
+		m_eventType = MouseEventPressed;
+		m_button = MiddleButton;
+	  }
+    else if (type == wxEVT_MIDDLE_UP)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = MiddleButton;
+	  }
+	else if (type == wxEVT_LEFT_DCLICK)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = LeftButton;
+	  }
+	else if (type == wxEVT_MIDDLE_DCLICK)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = MiddleButton;
+	  }	
+	else if (type == wxEVT_RIGHT_DCLICK)
+	  {
+		m_eventType = MouseEventReleased;
+		m_button = RightButton;
+	  }
+	if (m_eventType == MouseEventMoved)
+		m_clickCount = 0;
+	  else
+		m_clickCount = event.ButtonDClick() ? 2 : 1;
 
-    else if (type == wxEVT_MOTION)
-        m_eventType = MouseEventMoved;
-
-    if (event.LeftIsDown())
-        m_button = LeftButton;
-    else if (event.RightIsDown())
-        m_button = RightButton;
-    else if (event.MiddleIsDown())
-        m_button = MiddleButton;
+	m_timestamp = WebCore::currentTime();
 }
-
 }

[Alexander Vassilev]

The patch is in Comment #8, sorry I re-posted the old patch by mistake in Comment #7, so I marked it obsolete (too much stuff to track already, so hence the mistake)

[Kevin Ollivier]

Sorry, you're right, I was looking at what I thought the code should have been doing rather than what it does. :-) Testing now...

[Kevin Ollivier]

Created attachment 20685 [details]
Comment #8 reformatted as patch

In the future, please make sure to always attach updates to the patch tracker as a new patch. To land any patch it needs to get an r+ first, but I can't do that if there is no patch. :)

Also, please make sure patches adhere to the WebKit style guidelines. The guidelines are specified here: 

http://webkit.org/coding/coding-style.html

I've updated this current patch for you so because I'd like to go ahead and land it, but in the future, please make sure it adheres to the guidelines before submitting.

Thanks!

[Kevin Ollivier]

Sorry, one more comment. In the future, also please add a ChangeLog entry. You can use the script WebKitTools/Scripts/prepare-ChangeLog <files or directories> to automatically generate a basic ChangeLog entry for your changes. Then just write a description and make sure it's included with the patch. Thanks!

[Kevin Ollivier]

Actually, looking at this code again, before I commit I want to ask you - did event.Button() not work for you? I think the real mistake in the original was to check which button was down when setting it, rather than to just check the Button() parameter. This would allow us to avoid having to write all those long if statements and just change the if (event.LeftIsDown()) statements into if (event.Button() == wxMOUSE_BTN_LEFT). 

Could you let me know if this works for you? Thanks!

[Alexander Vassilev]

I don't quite understand what you mean - there is no event.LeftIsDown() call in the code I propose. All checks are done against the event type. Looking at the wx documentation of wxMouseEvent::Button() I see your point. But this would give us only the button - left, middle or right, and no info whather it was released pressed or doubleclicked - and we need also this code - note that in the ifs we set not only m_button to left right or middle, but also m_eventType which cahn be pressed released or dclicked. So I think we can't avoid the if's here. 
Thank you for the advice and the formatting of the patch - the best way to teach somebody how to do something is to give example :)

[Kevin Ollivier]

Created attachment 20700 [details]
Fix that uses event.Button() to set the button.

Here is an example of a 'complete' patch that also illustrates what I was saying about using event.Button() instead of event.XYZIsDown(). Unfortunately, since I haven't actually found a way to reproduce your assert, you'll have to let me know if this new patch works for you.

[Alexander Vassilev]

Your example patch deals with one of 2 things necessary - set the button type (left, right, or middle). But what about m_eventType? was the right, left or middle button pressed, released or doubleclicked? THis is why I think we can't do without all the if's - they do both tasks at the same time -  setting the button kind (left right middle) AND the event type (press release dclick)

[Kevin Ollivier]

(In reply to comment #16)
> Your example patch deals with one of 2 things necessary - set the button type
> (left, right, or middle). But what about m_eventType? was the right, left or
> middle button pressed, released or doubleclicked? THis is why I think we can't
> do without all the if's - they do both tasks at the same time -  setting the
> button kind (left right middle) AND the event type (press release dclick)
> 

My patch doesn't deal with m_eventType because there's already code that sets m_eventType in WebKit trunk. See (http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/platform/wx/MouseEventWx.cpp) 

What I've submitted is a patch to the existing code, not a re-write of it. I'm still not sure what the code in trunk + this patch doesn't do that your code does. If we can do the same thing in 20 lines rather than 50, I'd much rather do that because it's easier to debug and maintain.

[Alexander Vassilev]

Ahh, ok ok, now I see what you were trying to say with the last posts. Ok I will test it when I have time, I tihink your proposed patch will fix the assertion failure. THE reason I rewrote the whole method is: efficiency - consider the checks that have to be actually done in the old code and in my proposed code. Also another reason was - unification - I do repeated similar checks, even though the code takes more lines its symmetric and I think simpler to grasp - its just one type of check repeated many times, so more lines but less cpu cycles and simpler C constructs used. But the last is just my opinion, I'm far from the idea of imposing it. So I will post an update here if your patch fixes the assertion.

[Eric Seidel (no email)]

Comment on attachment 20700 [details]
Fix that uses event.Button() to set the button.

Looks good to me.

[Kevin Ollivier]

Landed in r32798, thanks Alexander and Eric! :)

[Kevin Ollivier]

This fix wasn't complete. It doesn't properly handle mouse up cases, like the previous fix did. We need to check for both button changes and button down (where the button is pressed but its state didn't change from the last event).

[Kevin Watters]

Created attachment 22136 [details]
Patch to use wxMouseEvent::LeftIsDown and related functions

With the above changes, text selection isn't working on MSW. WebKit is expecting m_button to reflect a currently depressed button as well as if a button is changing state--so wxMouseEvent::Button isn't enough, since it only shows button changes.

The attached patch includes calls to wxMouseEvent::LeftIsDown() and sibling functions to check the current button state.

[Kevin Ollivier]

Landed in r35066, thanks! :-)