Bug 1593690 - Fix some races caused by bug 1591717, and add some debugging code. r=mak
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 05 Nov 2019 22:30:26 +0000
changeset 500733 14830ac12439f75466e0aed13fa89f554e02a1f1
parent 500732 eba0acda89a5e93fa56cf368524d3b0e39e8d101
child 500734 b3988d311be3e180e9b4d3afd678cc76bcbda158
push id99800
push userealvarez@mozilla.com
push dateTue, 05 Nov 2019 23:21:27 +0000
treeherderautoland@14830ac12439 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1593690, 1591717
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1593690 - Fix some races caused by bug 1591717, and add some debugging code. r=mak These were unveiled by the coming patch. We didn't use to call run_next_test for the links that expected no visits before bug 1591717. Differential Revision: https://phabricator.services.mozilla.com/D51620
toolkit/components/places/tests/gtest/mock_Link.h
toolkit/components/places/tests/gtest/places_test_harness_tail.h
toolkit/components/places/tests/gtest/test_IHistory.cpp
--- a/toolkit/components/places/tests/gtest/mock_Link.h
+++ b/toolkit/components/places/tests/gtest/mock_Link.h
@@ -8,16 +8,17 @@
  * This is a mock Link object which can be used in tests.
  */
 
 #ifndef mock_Link_h__
 #define mock_Link_h__
 
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/dom/Link.h"
+#include "mozilla/StaticPrefs_layout.h"
 
 class mock_Link : public mozilla::dom::Link {
  public:
   NS_DECL_ISUPPORTS
 
   typedef void (*Handler)(nsLinkState);
 
   explicit mock_Link(Handler aHandlerFunction, bool aRunNextTest = true)
@@ -37,17 +38,19 @@ class mock_Link : public mozilla::dom::L
     return 0;  // the value shouldn't matter
   }
 
   void NodeInfoChanged(mozilla::dom::Document* aOldDoc) final {}
 
   bool GotNotified() const { return !mDeathGrip; }
 
   void AwaitNewNotification(Handler aNewHandler) {
-    MOZ_ASSERT(!mDeathGrip, "Still waiting for a notification");
+    MOZ_ASSERT(
+        !mDeathGrip || !mozilla::StaticPrefs::layout_css_notify_of_unvisited(),
+        "Still waiting for a notification");
     // Create a cyclic ownership, so that the link will be released only
     // after its status has been updated.  This will ensure that, when it should
     // run the next test, it will happen at the end of the test function, if
     // the link status has already been set before.  Indeed the link status is
     // updated on a separate connection, thus may happen at any time.
     mDeathGrip = this;
     mHandler = aNewHandler;
   }
--- a/toolkit/components/places/tests/gtest/places_test_harness_tail.h
+++ b/toolkit/components/places/tests/gtest/places_test_harness_tail.h
@@ -29,25 +29,35 @@ class RunNextTest : public mozilla::Runn
       test.func();
     }
 
     do_test_finished();
     return NS_OK;
   }
 };
 
+static const bool kDebugRunNextTest = false;
+
 void run_next_test() {
+  if (kDebugRunNextTest) {
+    printf_stderr("run_next_test()\n");
+    nsTraceRefcnt::WalkTheStack(stderr);
+  }
   nsCOMPtr<nsIRunnable> event = new RunNextTest();
   do_check_success(NS_DispatchToCurrentThread(event));
 }
 
 int gPendingTests = 0;
 
 void do_test_pending() {
   NS_ASSERTION(NS_IsMainThread(), "Not running on the main thread?");
+  if (kDebugRunNextTest) {
+    printf_stderr("do_test_pending()\n");
+    nsTraceRefcnt::WalkTheStack(stderr);
+  }
   gPendingTests++;
 }
 
 void do_test_finished() {
   NS_ASSERTION(NS_IsMainThread(), "Not running on the main thread?");
   NS_ASSERTION(gPendingTests > 0, "Invalid pending test count!");
   gPendingTests--;
 }
--- a/toolkit/components/places/tests/gtest/test_IHistory.cpp
+++ b/toolkit/components/places/tests/gtest/test_IHistory.cpp
@@ -161,16 +161,20 @@ void test_visited_notifies() {
   do_check_success(rv);
 
   // Note: test will continue upon notification.
 }
 
 void test_unvisited_does_not_notify_part2() {
   using namespace test_unvisited_does_not_notify;
 
+  if (StaticPrefs::layout_css_notify_of_unvisited()) {
+    SpinEventLoopUntil([&]() { return testLink->GotNotified(); });
+  }
+
   // We would have had a failure at this point had the content node been told it
   // was visited. Therefore, now we change it so that it expects a visited
   // notification, and unregisters itself after addURI.
   testLink->AwaitNewNotification(expect_visit);
   addURI(testURI);
 
   // Clear the stored variables now.
   testURI = nullptr;
@@ -199,17 +203,17 @@ void test_same_uri_notifies_both() {
 }
 
 void test_unregistered_visited_does_not_notify() {
   // This test must have a test that has a successful notification after it.
   // The Link would have been notified by now if we were buggy and notified
   // unregistered Links (due to request serialization).
 
   nsCOMPtr<nsIURI> testURI = new_test_uri();
-  RefPtr<Link> link = new mock_Link(expect_no_visit);
+  RefPtr<Link> link = new mock_Link(expect_no_visit, false);
   nsCOMPtr<IHistory> history(do_get_IHistory());
   nsresult rv = history->RegisterVisitedCallback(testURI, link);
   do_check_success(rv);
 
   // Unregister the Link.
   history->UnregisterVisitedCallback(testURI, link);
 
   // And finally add a visit for the URI.
@@ -233,33 +237,35 @@ void test_new_visit_notifies_waiting_Lin
   RefPtr<mock_Link> link = new mock_Link(expect_no_visit);
 
   // Now, register our content node to be notified.
   nsCOMPtr<nsIURI> testURI = new_test_uri();
   nsCOMPtr<IHistory> history = do_get_IHistory();
   nsresult rv = history->RegisterVisitedCallback(testURI, link);
   do_check_success(rv);
 
-  SpinEventLoopUntil([&]() { return link->GotNotified(); });
+  if (StaticPrefs::layout_css_notify_of_unvisited()) {
+    SpinEventLoopUntil([&]() { return link->GotNotified(); });
+  }
 
   link->AwaitNewNotification(expect_visit);
 
   // Add ourselves to history.
   addURI(testURI);
 
   // Note: test will continue upon notification.
 }
 
 void test_RegisterVisitedCallback_returns_before_notifying() {
   // Add a URI so that it's already in history.
   nsCOMPtr<nsIURI> testURI = new_test_uri();
   addURI(testURI);
 
   // Create our test Link.
-  RefPtr<Link> link = new mock_Link(expect_no_visit);
+  RefPtr<Link> link = new mock_Link(expect_no_visit, false);
 
   // Now, register our content node to be notified.  It should not be notified.
   nsCOMPtr<IHistory> history = do_get_IHistory();
   nsresult rv = history->RegisterVisitedCallback(testURI, link);
   do_check_success(rv);
 
   // Remove ourselves as an observer.  We would have failed if we had been
   // notified.
@@ -337,18 +343,18 @@ void test_observer_topic_dispatched() {
   bool urisEqual;
   nsresult rv = visitedURI->Equals(notVisitedURI, &urisEqual);
   do_check_success(rv);
   do_check_false(urisEqual);
   addURI(visitedURI);
 
   // Need two Link objects as well - one for each URI.
   RefPtr<Link> visitedLink = new mock_Link(expect_visit, false);
+  RefPtr<Link> notVisitedLink = new mock_Link(expect_no_visit, false);
   RefPtr<Link> visitedLinkCopy = visitedLink;
-  RefPtr<Link> notVisitedLink = new mock_Link(expect_no_visit);
 
   // Add the right observers for the URIs to check results.
   bool visitedNotified = false;
   nsCOMPtr<nsIObserver> visitedObs =
       new statusObserver(visitedURI, true, visitedNotified);
   bool notVisitedNotified = false;
   nsCOMPtr<nsIObserver> unvisitedObs =
       new statusObserver(notVisitedURI, false, notVisitedNotified);