| Summary: | [GTK][WPE] Unify TestController::platformRunUntil() and honor condition flag | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||||
| Component: | Tools / Tests | Assignee: | Adrian Perez <aperez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, cgarcia, clopez, commit-queue, lforschler, mcatanzaro, rniwa, simon.fraser, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | Other | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 167941 | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 357683 [details]
Patch
Comment on attachment 357683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357683&action=review > Tools/WebKitTestRunner/wpe/TestControllerWPE.cpp:77 > + while (!done) > + RunLoop::main().run(); This isn't right because now the timeout is no longer respected at all; it can run forever if the job never completes. I'm not even certain what the expected behavior of this function is. Is it: run the loop until condition becomes true OR the timeout expires? That's what I'd assume just from looking at the parameters. The problem here is that there's no way to stop the RunLoop after the condition becomes true before the timeout expires, except by using more smaller timeouts. Or is it: run the loop until condition becomes true, checking whether condition is true every timeout seconds, guaranteeing that it never returns if condition never becomes true? That's what you've implemented. (In reply to Michael Catanzaro from comment #2) > Comment on attachment 357683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357683&action=review > > > Tools/WebKitTestRunner/wpe/TestControllerWPE.cpp:77 > > + while (!done) > > + RunLoop::main().run(); > > This isn't right because now the timeout is no longer respected at all; it > can run forever if the job never completes. Right, what we need is: while (!(done || timedOut)) RunLoop::main().run(); With “timedOut” being set to “false” when the timeout timer is fired. (In reply to Michael Catanzaro from comment #3) > I'm not even certain what the expected behavior of this function is. Is it: > run the loop until condition becomes true OR the timeout expires? That's > what I'd assume just from looking at the parameters. The problem here is > that there's no way to stop the RunLoop after the condition becomes true > before the timeout expires, except by using more smaller timeouts. > > Or is it: run the loop until condition becomes true, checking whether > condition is true every timeout seconds, guaranteeing that it never returns > if condition never becomes true? That's what you've implemented. The run loop is stopped when TimeoutTimer::fired() is invoked, note that it calls RunLoop::main().stop(). (In reply to Adrian Perez from comment #4) > Right, what we need is: > > while (!(done || timedOut)) > RunLoop::main().run(); > > With “timedOut” being set to “false” when the timeout timer is fired. If you make only this change, then the while becomes redundant. You know it has timed out by virtue of having returned from RunLoop::run and you know it doesn't matter whether done is true or not because the timeout has passed. So you need to use another shorter, internal timeout to wake things up more often. Created attachment 357712 [details]
Patch v2
(In reply to Michael Catanzaro from comment #6) > (In reply to Adrian Perez from comment #4) > > Right, what we need is: > > > > while (!(done || timedOut)) > > RunLoop::main().run(); > > > > With “timedOut” being set to “false” when the timeout timer is fired. > > If you make only this change, then the while becomes redundant. You know it > has timed out by virtue of having returned from RunLoop::run [...] Not quite. Returning from RunLoop::run may happen for two reasons: 1. Time out. 2. Calling TestController::notifyDone(). From what I see around, the expectation is that the tests using runUntil() (which internally calls platformRunUntil(), BTW) will always flip the “done” flag to “true” before calling TestController::notifyDone(). On time out, the “done” flag won't be set to “true”. > [...] and you know it > doesn't matter whether done is true or not because the timeout has passed. > So you need to use another shorter, internal timeout to wake things up more > often. I don't understand this part of your comment. I don't see how having a timer that fires at regular short intervals is going to change the two possible exit conditions I wrote about above. Trying to explain things in a different way... At the point when TestController::platformRunUntil(done, timeout) is executed: 1. If “done” is already “true” → the run loop is never started, the function just returns. 2. Otherwise the loop is entered: 2.a.) Some external code, usually a callback dispatched by the run loop, will set the “done” flag to “true” and call TestController::notifyDone() -- which stops the run loop. 2.b.) The operation times out. In this case we want to exit the “while(...) RunLoop::main().run()” loop only, so the ::fired() timer callback sets “timedOut = false” and stops the run loop. The “done” flag is kept untouched (hence set still to “false”. I am 100% sure that v2 of the patch covers these three cases. Comment on attachment 357712 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=357712&action=review OK, you're right. > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:79 > + while (!(done | timeoutTimer.timedOut)) PROBLEM: you meant to write || here in TestControllerGTK.cpp. > Tools/WebKitTestRunner/wpe/TestControllerWPE.cpp:82 > + while (!(done || timeoutTimer.timedOut)) Personally I would write: while (!done && !timeoutTimer.timedOut). One fewer level of parentheses, a bit easier to read IMO. Up to you. (In reply to Michael Catanzaro from comment #9) > Comment on attachment 357712 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357712&action=review > > OK, you're right. > > > Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:79 > > + while (!(done | timeoutTimer.timedOut)) > > PROBLEM: you meant to write || here in TestControllerGTK.cpp. ...aaand this is why we make code reviews, good catch! :) > > Tools/WebKitTestRunner/wpe/TestControllerWPE.cpp:82 > > + while (!(done || timeoutTimer.timedOut)) > > Personally I would write: while (!done && !timeoutTimer.timedOut). One fewer > level of parentheses, a bit easier to read IMO. Up to you. I always doubt between the two styles, so I'll go with your option because “less parens” sounds like a good thing. I'll fix these two issued above before landing. Thanks! Created attachment 357737 [details]
Patch for landing
Comment on attachment 357737 [details] Patch for landing Clearing flags on attachment: 357737 Committed r239401: <https://trac.webkit.org/changeset/239401> All reviewed patches have been landed. Closing bug. |
Currently the TestController::platformRunUntil(bool&, WTF::Seconds) function is different for the GTK+ and WPE ports, which does not really make much sense, and also it's not honoring the condition flag at all. Ignoring the flag means that the following can happen: 1. Some process is started in background which can finish very fast. 2. The background process finishes and sets the flag to “false”, and calls TestController::notifyDone() to stop the event loop. 3. TestController::platformRunUntil() gets called, and the event loop gets started even when the condition was already met. 4. The event loop is never stopped because ::notifyDone() is not called (the early completion already happened), making the test cast timeout. I have experienced the above scenario about 99% of the time while working on the content extensions support for the GTK+ and WPE ports, but it could happen for other kinds of tests as well. I have a strong suspicion that this may be the cause for the flakyness of some of our tests.