Bug 34069 - REGRESSION: editing/selection/doubleclick-beside-cr-span.html times out on Windows
Summary: REGRESSION: editing/selection/doubleclick-beside-cr-span.html times out on Wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-24 23:18 PST by Eric Seidel (no email)
Modified: 2010-02-01 16:08 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (1.21 KB, patch)
2010-02-01 03:32 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-01-24 23:18:02 PST
REGRESSION: editing/selection/doubleclick-beside-cr-span.html times out on Windows

This seems to have started today.  The windows bots were broken between r53763  and r53783, and I believe it the timeouts started happening during that down time.

Added 6 days ago:
http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/doubleclick-beside-cr-span.html
Comment 1 Shinichiro Hamaji 2010-01-26 04:45:54 PST
Hmm... I'm not sure if this is a regression. I ran the test with r53763 and r53783. I didn't see significant differences between the results of them. I've also noticed this test is very slow on Windows (~24 secs on my slow machine). It seems eventSender.leapForward() actually waits on Windows? So, I'm guessing somehow windows bots become slow during the downtime, but I'm not sure at all.

Anyway, if we want to make the bot green, we can change from leapForward(100) to leapForward(50). But, of course, if this is indicating an actual regression, this change just hides the regression...
Comment 2 MORITA Hajime 2010-01-26 07:25:10 PST
Thanks for the investigation, Hamaji-san.
I don't have windows box...

> It seems eventSender.leapForward() actually waits on Windows? 
At a glance, the code appears to literally sleep.

in WebKitTools/DumpRenderTree/win/EventSender.cpp,

----
static JSValueRef leapForwardCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
{
    if (argumentCount > 0) {
        msgQueue[endOfQueue].delay = JSValueToNumber(context, arguments[0], exception);
    }
...
}

and 

void replaySavedEvents(HRESULT* oleDragAndDropReturnValue)
{
...
    if (msgQueue[startOfQueue].delay) {
        ::Sleep(msgQueue[startOfQueue].delay);
        msgQueue[startOfQueue].delay = 0;
    }
...
}

----

I guess it would take long time to run the test case,
and the case would be around timeout border,
timed out recently with small overhead added somewhere.

For this reason, I think making sleep time shorter is reasonable at this time, 
although further investigation to speedup windows layout test is required.
Comment 3 Shinichiro Hamaji 2010-02-01 03:32:19 PST
Created attachment 47828 [details]
Patch v1
Comment 4 Shinichiro Hamaji 2010-02-01 03:35:42 PST
I ran the tests with r53762 on build bot and it timed out. So, I guess the timeout happened even before r53762 (or buildbot somehow becomes slower during the windows bots outage?).

http://build.webkit.org/builders/Windows%20Debug%20%28Tests%29/builds/9253
Comment 5 Eric Seidel (no email) 2010-02-01 05:17:21 PST
Comment on attachment 47828 [details]
Patch v1

leapForward does not actually sleep (at least not on the mac).  Instead it just queues the next event with a different time during event playback.
Comment 6 Shinichiro Hamaji 2010-02-01 05:29:29 PST
(In reply to comment #5)
> (From update of attachment 47828 [details])
> leapForward does not actually sleep (at least not on the mac).  Instead it just
> queues the next event with a different time during event playback.

Yeah, on Mac, leapForward doesn't sleep. However, on Windows, it seems leapForward sleeps as Morita-san said.
Comment 7 Darin Adler 2010-02-01 08:14:10 PST
Comment on attachment 47828 [details]
Patch v1

The whole idea of leapForward was to not require sleeping, but I suppose it's OK to make this change.
Comment 8 Shinichiro Hamaji 2010-02-01 16:08:47 PST
Comment on attachment 47828 [details]
Patch v1

Clearing flags on attachment: 47828

Committed r54166: <http://trac.webkit.org/changeset/54166>
Comment 9 Shinichiro Hamaji 2010-02-01 16:08:55 PST
All reviewed patches have been landed.  Closing bug.