WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.51 KB, patch)
2019-04-15 10:49 PDT
,
Per Arne Vollan
no flags
Details
Formatted Diff
Diff
Patch
(3.43 KB, patch)
2019-04-15 10:57 PDT
,
Per Arne Vollan
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2019-04-13 12:22:43 PDT
Created
attachment 367389
[details]
Patch
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
Created
attachment 367425
[details]
Patch
Per Arne Vollan
Comment 6
2019-04-15 10:57:36 PDT
Created
attachment 367427
[details]
Patch
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
Committed
r244273
: <
https://trac.webkit.org/changeset/244273/webkit
>
Radar WebKit Bug Importer
Comment 11
2019-04-15 11:31:29 PDT
<
rdar://problem/49910600
>
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.
Top of Page
Format For Printing
XML
Clone This Bug