Bug 73366 – [Qt] Last mouse event could be lost if a JS call to eventSender.leapForward is made

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hugo Parente Lima]

Last mouse event could be lost if a JS call to eventSender.leapForward is made

[Hugo Parente Lima]

Created attachment 117059 [details]
Patch

[Simon Hausmann]

How did you notice this problem? Is there another existing test that is failing? Or is this causing flakyness?

[Hugo Parente Lima]

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.

[Hugo Parente Lima]

Created attachment 117279 [details]
Patch

[Hugo Parente Lima]

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.

[Kenneth Rohde Christiansen]

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"

[Hugo Parente Lima]

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.

[Hugo Parente Lima]

Created attachment 117404 [details]
Patch

[Kenneth Rohde Christiansen]

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 :-)

[Hugo Parente Lima]

Created attachment 117410 [details]
Patch

[Hugo Parente Lima]

Hope to haven't missed nothing this time...

[Kenneth Rohde Christiansen]

Comment on attachment 117410 [details]
Patch

I still think we should move the test

[Kenneth Rohde Christiansen]

Simon, what is your take? fast/testrunner ?

[Hugo Parente Lima]

So... this can be committed as is or there's a better place to put the layout test?

[Kenneth Rohde Christiansen]

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

[Hugo Parente Lima]

The test already have such comment:

"This should test if the test driver doesn't eat the last event (...)"

[Kenneth Rohde Christiansen]

Comment on attachment 117410 [details]
Patch

Lets commit it then

[WebKit Review Bot]

Comment on attachment 117410 [details]
Patch

Clearing flags on attachment: 117410

Committed r102048: <http://trac.webkit.org/changeset/102048>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Hugo Parente Lima]

Hi;

No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9.

[Csaba Osztrogonác]

(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.

[Kenneth Rohde Christiansen]

(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.

[Hugo Parente Lima]

(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.

[Ryosuke Niwa]

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

[Philippe Normand]

(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