RESOLVED FIXED 196898
TestRunner::notifyDone() should be safely reentrant
https://bugs.webkit.org/show_bug.cgi?id=196898
Summary TestRunner::notifyDone() should be safely reentrant
Per Arne Vollan
Reported 2019-04-13 10:24:22 PDT
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.
Attachments
Patch (2.45 KB, patch)
2019-04-13 12:22 PDT, Per Arne Vollan
no flags
Patch (3.51 KB, patch)
2019-04-15 10:49 PDT, Per Arne Vollan
no flags
Patch (3.43 KB, patch)
2019-04-15 10:57 PDT, Per Arne Vollan
darin: review+
Per Arne Vollan
Comment 1 2019-04-13 12:22:43 PDT
Jonathan Bedard
Comment 2 2019-04-15 05:59:41 PDT
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
Comment 3 2019-04-15 06:02:58 PDT
(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
Comment 4 2019-04-15 10:44:58 PDT
(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
Comment 5 2019-04-15 10:49:43 PDT
Per Arne Vollan
Comment 6 2019-04-15 10:57:36 PDT
Darin Adler
Comment 7 2019-04-15 11:01:15 PDT
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 8 2019-04-15 11:03:50 PDT
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
Comment 9 2019-04-15 11:15:20 PDT
(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
Comment 10 2019-04-15 11:31:04 PDT
Radar WebKit Bug Importer
Comment 11 2019-04-15 11:31:29 PDT
Jonathan Bedard
Comment 12 2019-04-15 14:30:06 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.