Bug 52873

Summary: Chromium DRT should provide command-line option for specifying timeout to ease debugging
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, eric, phnixwxz, tkent, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
tony: review-, tony: commit-queue-
Updated patch: --no-timeout
none
Fix a small bug none

Description Xianzhu Wang 2011-01-21 01:29:10 PST
When debugging DRT, we need to specify a larger timeout so that DRT won't timeout during debugging. Though we can specify timeout in test-shell mode, sometimes the non-test-shell mode is more convenient to use in debugging.
Comment 1 Xianzhu Wang 2011-01-21 04:50:38 PST
Created attachment 79724 [details]
patch
Comment 2 Eric Seidel (no email) 2011-01-24 15:29:46 PST
We're tryign to be more like the other DRTs, not less. :)  How do other DRT implementations handle this case?
Comment 3 Xianzhu Wang 2011-01-25 05:47:43 PST
(In reply to comment #2)
> We're tryign to be more like the other DRTs, not less. :)  How do other DRT implementations handle this case?

Other implementations seem to also have the timeout issue.

I just asked a colleague how he prevent timeout when debugging DRT, he answered he just modifies DRT to disable timeout. Wondering how others do.

If you think the feature is useful, I'd be glad to add it into other ports.
Comment 4 Tony Chang 2011-01-25 09:45:30 PST
Comment on attachment 79724 [details]
patch

I think it's OK to add this, however I think it would be better if it were just a flag like --disable-time-out.  I don't see the benefit in being able to specify a timeout.
Comment 5 Xianzhu Wang 2011-01-25 17:01:57 PST
(In reply to comment #4)
> (From update of attachment 79724 [details])
> I think it's OK to add this, however I think it would be better if it were just a flag like --disable-time-out.  I don't see the benefit in being able to specify a timeout.

For --disable-time-out, the implementation will be 'shell.setLayoutTestTimeout(INT_MAX)' instead of changing all of the TestShellXXX.cpp to actually disable timeout. Would this look good to you?
Comment 6 Tony Chang 2011-01-25 17:11:24 PST
(In reply to comment #5)
> For --disable-time-out, the implementation will be 'shell.setLayoutTestTimeout(INT_MAX)' instead of changing all of the TestShellXXX.cpp to actually disable timeout. Would this look good to you?

Yes, I think that's ok.
Comment 7 Xianzhu Wang 2011-01-25 22:07:53 PST
Created attachment 80169 [details]
Updated patch: --no-timeout

I just investigated non-Chromium DRTs and found that they are different from Chromium DRTs about timeout. Chromium DRTs will call 'exit()' on timeout, while non-Chromium DRTs call notifyDone() which will cause exiting in the next message loop. So for the latter timeout is not a big issue for debugging.
Comment 8 Tony Chang 2011-01-26 10:30:25 PST
Comment on attachment 80169 [details]
Updated patch: --no-timeout

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

At some point, we should see if we can get Chromium DRT to do the same as Mac DRT and call notifyDone on timeout.

> Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:191
> +            // 0x20000000ms is big enough for the purpose to avoid timeout in debugging.
> +            shell.setLayoutTestTimeout(0x20000000);

Why did you decide against INT_MAX from climits?
Comment 9 WebKit Commit Bot 2011-01-26 11:54:10 PST
Comment on attachment 80169 [details]
Updated patch: --no-timeout

Clearing flags on attachment: 80169

Committed r76704: <http://trac.webkit.org/changeset/76704>
Comment 10 WebKit Commit Bot 2011-01-26 11:54:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Commit Bot 2011-01-26 12:04:22 PST
The commit-queue encountered the following flaky tests while processing attachment 80169 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 Xianzhu Wang 2011-01-26 17:25:08 PST
(In reply to comment #8)

Thanks Tony for review.

> (From update of attachment 80169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80169&action=review
> 
> At some point, we should see if we can get Chromium DRT to do the same as Mac DRT and call notifyDone on timeout.
> 

Non-Chromium DRT also has their problems:
1. Sometimes NRWT must kill the DRT process if it wants a timeout shorter than 30s. For Chromium DRT, the script can specify timeout for each test in test-shell mode.

2. If the DRT process is busy in a loop, it can't quit by itself and can only be killed.

> > Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:191
> > +            // 0x20000000ms is big enough for the purpose to avoid timeout in debugging.
> > +            shell.setLayoutTestTimeout(0x20000000);
> 
> Why did you decide against INT_MAX from climits?

I'm afraid if INT_MAX as a timeout was added to some value it would overflow, for example, some port might need 1 more second for timeout.
Comment 13 Xianzhu Wang 2011-02-15 21:46:55 PST
Sorry there is a small bug in the last patch.
Comment 14 Xianzhu Wang 2011-02-15 21:47:52 PST
Created attachment 82588 [details]
Fix a small bug
Comment 15 Kent Tamura 2011-02-15 21:54:49 PST
Comment on attachment 82588 [details]
Fix a small bug

ok
Comment 16 WebKit Commit Bot 2011-02-15 22:48:17 PST
Comment on attachment 82588 [details]
Fix a small bug

Clearing flags on attachment: 82588

Committed r78685: <http://trac.webkit.org/changeset/78685>
Comment 17 WebKit Commit Bot 2011-02-15 22:48:23 PST
All reviewed patches have been landed.  Closing bug.