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.
Created attachment 79724 [details] patch
We're tryign to be more like the other DRTs, not less. :) How do other DRT implementations handle this case?
(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 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.
(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?
(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.
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 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 on attachment 80169 [details] Updated patch: --no-timeout Clearing flags on attachment: 80169 Committed r76704: <http://trac.webkit.org/changeset/76704>
All reviewed patches have been landed. Closing bug.
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.
(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.
Sorry there is a small bug in the last patch.
Created attachment 82588 [details] Fix a small bug
Comment on attachment 82588 [details] Fix a small bug ok
Comment on attachment 82588 [details] Fix a small bug Clearing flags on attachment: 82588 Committed r78685: <http://trac.webkit.org/changeset/78685>