Summary: | [Qt] Last mouse event could be lost if a JS call to eventSender.leapForward is made | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hugo Parente Lima <hugo.lima> | ||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | hausmann, kenneth, ossy, pnormand, rniwa, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 73894 | ||||||||||||
Bug Blocks: | 45666 | ||||||||||||
Attachments: |
|
Description
Hugo Parente Lima
2011-11-29 14:55:15 PST
Created attachment 117059 [details]
Patch
How did you notice this problem? Is there another existing test that is failing? Or is this causing flakyness? 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. Created attachment 117279 [details]
Patch
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 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 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. Created attachment 117404 [details]
Patch
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 :-) Created attachment 117410 [details]
Patch
Hope to haven't missed nothing this time... Comment on attachment 117410 [details]
Patch
I still think we should move the test
Simon, what is your take? fast/testrunner ? So... this can be committed as is or there's a better place to put the layout test? 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 The test already have such comment: "This should test if the test driver doesn't eat the last event (...)" Comment on attachment 117410 [details]
Patch
Lets commit it then
Comment on attachment 117410 [details] Patch Clearing flags on attachment: 117410 Committed r102048: <http://trac.webkit.org/changeset/102048> All reviewed patches have been landed. Closing bug. 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? Hi; No, I don't have plans to fix this on qt-wk1 since there will be no qt 4.9. (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. (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. (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. 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 (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 |