Bug 73366 - [Qt] Last mouse event could be lost if a JS call to eventSender.leapForward is made
Summary: [Qt] Last mouse event could be lost if a JS call to eventSender.leapForward i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 73894
Blocks: 45666
  Show dependency treegraph
 
Reported: 2011-11-29 14:55 PST by Hugo Parente Lima
Modified: 2011-12-12 02:23 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.64 KB, patch)
2011-11-29 15:27 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2011-11-30 14:49 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2011-12-01 06:04 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2011-12-01 06:49 PST, Hugo Parente Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hugo Parente Lima 2011-11-29 14:55:15 PST
Last mouse event could be lost if a JS call to eventSender.leapForward is made
Comment 1 Hugo Parente Lima 2011-11-29 15:27:33 PST
Created attachment 117059 [details]
Patch
Comment 2 Simon Hausmann 2011-11-30 03:14:44 PST
How did you notice this problem? Is there another existing test that is failing? Or is this causing flakyness?
Comment 3 Hugo Parente Lima 2011-11-30 09:34:39 PST
I did notice it when investigating a drag&drop layout test (fast/events/bogus-dropEffect-effectAllowed.html), it fails, but not due to this problem.

I checked all test using leapForward, good candidates to fail without the patch is:

editing/selection/caret-bidi-first-and-last-letters.html

but it fails with and without the patch.


OTOH there are some tests like:

fast/events/selectstart-by-double-triple-clicks.html

that still fail but pass more assertions with the patch.

So I think there's no test covering this issue failing without the patch and passing with it at the moment.
Comment 4 Hugo Parente Lima 2011-11-30 14:49:47 PST
Created attachment 117279 [details]
Patch
Comment 5 Hugo Parente Lima 2011-11-30 14:52:54 PST
Added a small fix on this patch, it was not correctly clearing the processed events delay on event queue.

I notice it when working on bug#45666.
Comment 6 Kenneth Rohde Christiansen 2011-12-01 02:14:26 PST
Comment on attachment 117279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117279&action=review

> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:53
> +static bool replayingEvents;

isReplayingEvents would be a slightly better name (and more inline with WebKit)

> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:417
> +    int startOfQueue = 0;

When you are iterating on this, I am not sure the name is that good.

> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:425
> +        delete ev.m_event;

So we had a memory leak before? Add some more info to the changelog?

> LayoutTests/ChangeLog:12
> +        * fast/events/dont-lost-last-event-expected.txt: Added.

dont-loose! not lost

> LayoutTests/ChangeLog:13
> +        * fast/events/dont-lost-last-event.html: Added.

Having it in event doesn't make that much sense as it is a test of the testing tool instead of a test of webkit events. I wonder where it should be put.

> LayoutTests/fast/events/dont-lost-last-event.html:54
> +        <div id="targetElem" style="border: 1px dashed" onMouseOver="pass()">Pass the mouse over me.<br/><br/></div>

we normally just write "onmouseover"
Comment 7 Hugo Parente Lima 2011-12-01 05:46:09 PST
Comment on attachment 117279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117279&action=review

Thanks for the review Kenny, I'll make the changes and re-submit the patch.

>> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:53
>> +static bool replayingEvents;
> 
> isReplayingEvents would be a slightly better name (and more inline with WebKit)

Agree.

>> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:417
>> +    int startOfQueue = 0;
> 
> When you are iterating on this, I am not sure the name is that good.

I'm iterating on this and moving the start of the queue, depends on the point of view =], ok to rename it to "i" ?

>> Tools/WebKitTestRunner/qt/EventSenderProxyQt.cpp:425
>> +        delete ev.m_event;
> 
> So we had a memory leak before? Add some more info to the changelog?

We hadn't, the previous implementation used postEvent instead of sendEvent, but using postEvent could cause last event to never be delivered if the widget was already deleted while the event was waiting on Qt event queue.

>> LayoutTests/ChangeLog:12

> 
> dont-loose! not lost

Yes sir!

>> LayoutTests/ChangeLog:13
>> +        * fast/events/dont-lost-last-event.html: Added.
> 
> Having it in event doesn't make that much sense as it is a test of the testing tool instead of a test of webkit events. I wonder where it should be put.

I new here, so a suggestion is appreciated :-)

>> LayoutTests/fast/events/dont-lost-last-event.html:54
>> +        <div id="targetElem" style="border: 1px dashed" onMouseOver="pass()">Pass the mouse over me.<br/><br/></div>
> 
> we normally just write "onmouseover"

Ok, this was my camel case syndrome.
Comment 8 Hugo Parente Lima 2011-12-01 06:04:18 PST
Created attachment 117404 [details]
Patch
Comment 9 Kenneth Rohde Christiansen 2011-12-01 06:16:48 PST
Comment on attachment 117404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117404&action=review

Almost there!

> LayoutTests/ChangeLog:12
> +        * fast/events/dont-lost-last-event-expected.txt: Added.

fast/testrunner/dont-loose-last-event?

Remember to update your changelog... check the page in the wiki about using webkit with git, there is a tool that will do it automatically :-)
Comment 10 Hugo Parente Lima 2011-12-01 06:49:34 PST
Created attachment 117410 [details]
Patch
Comment 11 Hugo Parente Lima 2011-12-01 06:50:55 PST
Hope to haven't missed nothing this time...
Comment 12 Kenneth Rohde Christiansen 2011-12-01 06:53:00 PST
Comment on attachment 117410 [details]
Patch

I still think we should move the test
Comment 13 Kenneth Rohde Christiansen 2011-12-01 06:53:32 PST
Simon, what is your take? fast/testrunner ?
Comment 14 Hugo Parente Lima 2011-12-05 09:24:12 PST
So... this can be committed as is or there's a better place to put the layout test?
Comment 15 Kenneth Rohde Christiansen 2011-12-05 12:25:43 PST
I would say it is ok to commit as it is, but it would be nice with a comment in the test making it clear that it is testing the test system it self and maybe even link to a bug url
Comment 16 Hugo Parente Lima 2011-12-05 12:57:38 PST
The test already have such comment:

"This should test if the test driver doesn't eat the last event (...)"
Comment 17 Kenneth Rohde Christiansen 2011-12-05 13:04:52 PST
Comment on attachment 117410 [details]
Patch

Lets commit it then
Comment 18 WebKit Review Bot 2011-12-05 14:39:33 PST
Comment on attachment 117410 [details]
Patch

Clearing flags on attachment: 117410

Committed r102048: <http://trac.webkit.org/changeset/102048>
Comment 19 WebKit Review Bot 2011-12-05 14:39:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Csaba Osztrogonác 2011-12-05 23:53:03 PST
The new test introduced in this bug fails on Qt-WK1. I skipped it on qt-wk1: http://trac.webkit.org/changeset/102103 to make buildbot happier.

Just a question: Are you going to fix this bug on wk1 or not?
Comment 21 Hugo Parente Lima 2011-12-06 06:45:11 PST
Hi;

No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9.
Comment 22 Csaba Osztrogonác 2011-12-06 07:14:16 PST
(In reply to comment #21)
> No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9.
Why is it related to Qt 4.9? AFAIK Qt5 will be shipped with WebKit1 API too.
Comment 23 Kenneth Rohde Christiansen 2011-12-06 07:18:16 PST
(In reply to comment #22)
> (In reply to comment #21)
> > No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9.
> Why is it related to Qt 4.9? AFAIK Qt5 will be shipped with WebKit1 API too.

Yes the current stable WebKit1 API will be a separate module for Qt5 with no new features and which depends on the QtWidget module.
Comment 24 Hugo Parente Lima 2011-12-06 07:38:43 PST
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9.
> > Why is it related to Qt 4.9? AFAIK Qt5 will be shipped with WebKit1 API too.
> 
> Yes the current stable WebKit1 API will be a separate module for Qt5 with no new features and which depends on the QtWidget module.

If you guys think that still valid to fix it on qt-wk1 I could fix it as well.
Comment 26 Philippe Normand 2011-12-12 02:23:19 PST
(In reply to comment #25)
> It appears that GTK has the same bug?
> 
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r102205%20(14274)/fast/events/dont-loose-last-event-pretty-diff.html

Yes, GTK has the same issue, see bug 74276