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

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 79724 [details]
patch

[Eric Seidel (no email)]

We're tryign to be more like the other DRTs, not less. :)  How do other DRT implementations handle this case?

[Xianzhu Wang]

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

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

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

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 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 on attachment 80169 [details]
Updated patch: --no-timeout

Clearing flags on attachment: 80169

Committed r76704: <http://trac.webkit.org/changeset/76704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80169 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Xianzhu Wang]

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

Sorry there is a small bug in the last patch.

[Xianzhu Wang]

Created attachment 82588 [details]
Fix a small bug

[Kent Tamura]

Comment on attachment 82588 [details]
Fix a small bug

ok

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.