Bug 192855 - [GTK][WPE] Unify TestController::platformRunUntil() and honor condition flag
Summary: [GTK][WPE] Unify TestController::platformRunUntil() and honor condition flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords: InRadar
Depends on:
Blocks: 167941
  Show dependency treegraph
 
Reported: 2018-12-19 08:26 PST by Adrian Perez
Modified: 2018-12-19 15:45 PST (History)
10 users (show)

See Also:


Attachments
Patch (4.25 KB, patch)
2018-12-19 08:56 PST, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch v2 (4.66 KB, patch)
2018-12-19 13:27 PST, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (4.66 KB, patch)
2018-12-19 15:23 PST, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2018-12-19 08:26:02 PST
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.
Comment 1 Adrian Perez 2018-12-19 08:56:57 PST
Created attachment 357683 [details]
Patch
Comment 2 Michael Catanzaro 2018-12-19 11:59:11 PST
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.
Comment 3 Michael Catanzaro 2018-12-19 12:04:54 PST
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.
Comment 4 Adrian Perez 2018-12-19 13:05:14 PST
(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.
Comment 5 Adrian Perez 2018-12-19 13:06:27 PST
(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().
Comment 6 Michael Catanzaro 2018-12-19 13:18:30 PST
(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.
Comment 7 Adrian Perez 2018-12-19 13:27:07 PST
Created attachment 357712 [details]
Patch v2
Comment 8 Adrian Perez 2018-12-19 14:22:02 PST
(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 9 Michael Catanzaro 2018-12-19 14:48:56 PST
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.
Comment 10 Adrian Perez 2018-12-19 15:13:40 PST
(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!
Comment 11 Adrian Perez 2018-12-19 15:23:21 PST
Created attachment 357737 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-12-19 15:44:54 PST
Comment on attachment 357737 [details]
Patch for landing

Clearing flags on attachment: 357737

Committed r239401: <https://trac.webkit.org/changeset/239401>
Comment 13 WebKit Commit Bot 2018-12-19 15:44:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-12-19 15:45:55 PST
<rdar://problem/46855515>