TestRunner::notifyDone() should be safely reentrant
authorpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 18:27:06 +0000 (18:27 +0000)
committerpvollan@apple.com <pvollan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 18:27:06 +0000 (18:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196898

Reviewed by Darin Adler.

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.
Fix this by setting the m_waitToDump flag before calling the dump() method.

* DumpRenderTree/mac/TestRunnerMac.mm:
(TestRunner::notifyDone):
(TestRunner::forceImmediateCompletion):
* DumpRenderTree/win/TestRunnerWin.cpp:
(TestRunner::notifyDone):
(TestRunner::forceImmediateCompletion):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244273 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/DumpRenderTree/mac/TestRunnerMac.mm
Tools/DumpRenderTree/win/TestRunnerWin.cpp

index b475fea..35f7526 100644 (file)
@@ -1,3 +1,22 @@
+2019-04-15  Per Arne Vollan  <pvollan@apple.com>
+
+        TestRunner::notifyDone() should be safely reentrant
+        https://bugs.webkit.org/show_bug.cgi?id=196898
+
+        Reviewed by Darin Adler.
+
+        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.
+        Fix this by setting the m_waitToDump flag before calling the dump() method.
+
+        * DumpRenderTree/mac/TestRunnerMac.mm:
+        (TestRunner::notifyDone):
+        (TestRunner::forceImmediateCompletion):
+        * DumpRenderTree/win/TestRunnerWin.cpp:
+        (TestRunner::notifyDone):
+        (TestRunner::forceImmediateCompletion):
+
 2019-04-15  Philippe Normand  <pnormand@igalia.com>
 
         [GTK][WPE] Add enable-media websetting
index 742517c..d27a35d 100644 (file)
@@ -293,16 +293,22 @@ size_t TestRunner::webHistoryItemCount()
 
 void TestRunner::notifyDone()
 {
-    if (m_waitToDump && !topLoadingFrame && !DRT::WorkQueue::singleton().count())
-        dump();
-    m_waitToDump = false;
+    if (m_waitToDump) {
+        m_waitToDump = false;
+        if (!topLoadingFrame && !DRT::WorkQueue::singleton().count())
+            dump();
+    } else
+        fprintf(stderr, "TestRunner::notifyDone() called unexpectedly.");
 }
 
 void TestRunner::forceImmediateCompletion()
 {
-    if (m_waitToDump && !DRT::WorkQueue::singleton().count())
-        dump();
-    m_waitToDump = false;
+    if (m_waitToDump) {
+        m_waitToDump = false;
+        if (!DRT::WorkQueue::singleton().count())
+            dump();
+    } else
+        fprintf(stderr, "TestRunner::forceImmediateCompletion() called unexpectedly.");
 }
 
 static inline std::string stringFromJSString(JSStringRef jsString)
index 6061aa1..8b799ed 100644 (file)
@@ -304,17 +304,23 @@ size_t TestRunner::webHistoryItemCount()
 void TestRunner::notifyDone()
 {
     // Same as on mac.  This can be shared.
-    if (m_waitToDump && !topLoadingFrame && !DRT::WorkQueue::singleton().count())
-        dump();
-    m_waitToDump = false;
+    if (m_waitToDump) {
+        m_waitToDump = false;
+        if (!topLoadingFrame && !DRT::WorkQueue::singleton().count())
+            dump();
+    } else
+        fprintf(stderr, "TestRunner::notifyDone() called unexpectedly.");
 }
 
 void TestRunner::forceImmediateCompletion()
 {
     // Same as on mac. This can be shared.
-    if (m_waitToDump && !DRT::WorkQueue::singleton().count())
-        dump();
-    m_waitToDump = false;
+    if (m_waitToDump) {
+        m_waitToDump = false;
+        if (!DRT::WorkQueue::singleton().count())
+            dump();
+    } else
+        fprintf(stderr, "TestRunner::forceImmediateCompletion() called unexpectedly.");
 }
 
 static wstring jsStringRefToWString(JSStringRef jsStr)