Bug 52873

Summary:

Chromium DRT should provide command-line option for specifying timeout to ease debugging

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dglazkov, eric, phnixwxz, tkent, tony

Priority:

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

Xianzhu Wang

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

Attachments
patch (2.38 KB, patch) 2011-01-21 04:50 PST, Xianzhu Wang	tony: review- tony: commit-queue-	Details Formatted Diff Diff
Updated patch: --no-timeout (2.43 KB, patch) 2011-01-25 22:07 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
Fix a small bug (1.69 KB, patch) 2011-02-15 21:47 PST, Xianzhu Wang	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2011-01-21 04:50:38 PST

Created attachment 79724 [details] patch

Eric Seidel (no email)

Comment 2 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?

Xianzhu Wang

Comment 3 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.

Tony Chang

Comment 4 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.

Xianzhu Wang

Comment 5 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?

Tony Chang

Comment 6 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.

Xianzhu Wang

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

Tony Chang

Comment 8 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?

WebKit Commit Bot

Comment 9 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>

WebKit Commit Bot

Comment 10 2011-01-26 11:54:14 PST

All reviewed patches have been landed. Closing bug.

WebKit Commit Bot

Comment 11 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.

Xianzhu Wang

Comment 12 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.

Xianzhu Wang

Comment 13 2011-02-15 21:46:55 PST

Sorry there is a small bug in the last patch.

Xianzhu Wang

Comment 14 2011-02-15 21:47:52 PST

Created attachment 82588 [details] Fix a small bug

Kent Tamura

Comment 15 2011-02-15 21:54:49 PST

Comment on attachment 82588 [details] Fix a small bug ok

WebKit Commit Bot

Comment 16 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>

WebKit Commit Bot

Comment 17 2011-02-15 22:48:23 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.