Bug 192855 – [GTK][WPE] Unify TestController::platformRunUntil() and honor condition flag

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

[Adrian Perez]

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.

[Adrian Perez]

Created attachment 357683 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Adrian Perez]

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

[Adrian Perez]

(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().

[Michael Catanzaro]

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

[Adrian Perez]

Created attachment 357712 [details]
Patch v2

[Adrian Perez]

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

[Michael Catanzaro]

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.

[Adrian Perez]

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

[Adrian Perez]

Created attachment 357737 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 357737 [details]
Patch for landing

Clearing flags on attachment: 357737

Committed r239401: <https://trac.webkit.org/changeset/239401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46855515>