TestRunner::notifyDone() should be safely reentrant

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Per Arne Vollan]

It is currently possible that TestRunner::notifyDone() will call itself, since notifyDone() will force a repaint, which can start executing JavaScript, which may call notifyDone() again. This can lead to test failures and flakiness.

[Per Arne Vollan]

Created attachment 367389 [details]
Patch

[Jonathan Bedard]

Any reason we aren't adding this to Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp too?
It also seems like forceImmediateCompletion() would have the same problem.

I need to look through how m_waitToDump is used, but I'm hesitant to say setting it before dump() is Ok. I think the reentrant point is valid, but I'm not convinced that silently making it work is the right approach, seems like doing this should cause an assertion error.

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #2)
> ... like doing this should cause an assertion error.

Or something to that effect...I suppose we would want to surface this error in Release builds too.

[Per Arne Vollan]

(In reply to Jonathan Bedard from comment #2)
> Any reason we aren't adding this to
> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp too?

I haven't observed this issue for modern WebKit, but it is currently happening on Win EWS causing flakiness and many failures on every run.

> It also seems like forceImmediateCompletion() would have the same problem.
> 

Good point! I'll make the same change in this method.

> I need to look through how m_waitToDump is used, but I'm hesitant to say
> setting it before dump() is Ok. I think the reentrant point is valid, but
> I'm not convinced that silently making it work is the right approach, seems
> like doing this should cause an assertion error.

Since it is currently happening on every Win EWS run, we might not be able to assert, since the tests would fail then. Maybe we can print a message to stdout?

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 367425 [details]
Patch

[Per Arne Vollan]

Created attachment 367427 [details]
Patch

[Darin Adler]

Comment on attachment 367427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367427&action=review

> Tools/ChangeLog:3
> +        TestRunner::notifyDone() should not be reentrant

The terminology here is incorrect. If the function is called in a reentrant fashion then it *is* reentrant. What this code change does it make it safe to call it in a reentrant fashion. So the title should be "TestRunner::notifyDone() should be safely reentrant" or something like that.

[Darin Adler]

Comment on attachment 367427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367427&action=review

Guess it’s OK. Lets give it a try.

> Tools/DumpRenderTree/mac/TestRunnerMac.mm:301
> +        fprintf(stderr, "TestRunner::notifyDone() called unexpectedly.");

So I guess it’s OK to do this since it won’t make tests fail? But it will fill our test results with "stderr output"; if a reentrant call is *not* actually unexpected.

[Per Arne Vollan]

(In reply to Darin Adler from comment #8)
> Comment on attachment 367427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367427&action=review
> 
> Guess it’s OK. Lets give it a try.
> 
> > Tools/DumpRenderTree/mac/TestRunnerMac.mm:301
> > +        fprintf(stderr, "TestRunner::notifyDone() called unexpectedly.");
> 
> So I guess it’s OK to do this since it won’t make tests fail? But it will
> fill our test results with "stderr output"; if a reentrant call is *not*
> actually unexpected.

There are only a few tests which makes this happen. The big problem is that they make every subsequent test fail, since the output will be dumped twice, shifting the output of every following test by one.

Thanks for reviewing!

[Per Arne Vollan]

Committed r244273: <https://trac.webkit.org/changeset/244273/webkit>

[Radar WebKit Bug Importer]

<rdar://problem/49910600>

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #2)
> ... I need to look through how m_waitToDump is used, but I'm hesitant to say
> setting it before dump() is Ok.

Looking through the code, we probably always should have set m_waitToDump before dumping.