Make the nested AsyncEvent work more reliably

We must take into consideration that somebody might hold on to a hard
reference to such an object. Thus it is not enough to drop the
references to it from its parent, that is not necessarily enough to
make it destruct. Instead have the parent explicitly cann a function
on the children than generates their end events.

Also add some documentation.

Change-Id: I38180c85072c76af8964c48fa19132b8e1178ee1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/114813
Tested-by: Jenkins
Reviewed-by: Tor Lillqvist <tml@collabora.com>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/114842
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>

2 files changed, 121 insertions, 76 deletions

diff --git a/comphelper/qa/unit/test_traceevent.cxx b/comphelper/qa/unit/test_traceevent.cxx
index 226171b0e097..e170e8aaeb18 100644
--- a/comphelper/qa/unit/test_traceevent.cxx
+++ b/comphelper/qa/unit/test_traceevent.cxx
@@ -31,87 +31,99 @@ namespace
 {
 void trace_event_test()
 {
-    // When we start recording is off and this will not generate any 'X' event when we leave the scope
-    comphelper::ProfileZone aZone0("test0");
+    std::shared_ptr<comphelper::AsyncEvent> pAsync7Locked;
 
-    // This will not generate any 'b' and 'e' events either
-    auto pAsync1(std::make_shared<comphelper::AsyncEvent>("async1"));
-
-    std::weak_ptr<comphelper::AsyncEvent> pAsync2;
     {
-        // No 'X' by this either
-        comphelper::ProfileZone aZone1("block1");
+        // When we start recording is off and this will not generate any 'X' event when we leave the scope
+        comphelper::ProfileZone aZone0("test0");
 
-        // Now we turn on recording
-        comphelper::TraceEvent::startRecording();
+        // This will not generate any 'b' and 'e' events either
+        auto pAsync1(std::make_shared<comphelper::AsyncEvent>("async1"));
 
-        // As this is nested in the parent that was created with recording turned off,
-        // this will not generate any 'b' and 'e' events either even if recording is now on.
-        pAsync2 = comphelper::AsyncEvent::createWithParent("async2in1", pAsync1);
-    }
+        std::weak_ptr<comphelper::AsyncEvent> pAsync2;
+        {
+            // No 'X' by this either
+            comphelper::ProfileZone aZone1("block1");
 
-    // This will generate an 'i' event for instant1
-    comphelper::TraceEvent::addInstantEvent("instant1");
+            // Now we turn on recording
+            comphelper::TraceEvent::startRecording();
 
-    std::shared_ptr<comphelper::AsyncEvent> pAsync25;
-    {
-        comphelper::ProfileZone aZone2("block2");
+            // As this is nested in the parent that was created with recording turned off,
+            // this will not generate any 'b' and 'e' events either even if recording is now on.
+            pAsync2 = comphelper::AsyncEvent::createWithParent("async2in1", pAsync1);
+        }
 
-        // This does not generate any 'e' event as it was created when recording was off
-        // And the nested async2 object will thus not generate anything either
-        pAsync1.reset();
+        // This will generate an 'i' event for instant1
+        comphelper::TraceEvent::addInstantEvent("instant1");
 
-        // This will generate 'b' event and an 'e' event when the pointer is reset or goes out of scope
-        pAsync25 = std::make_shared<comphelper::AsyncEvent>("async2.5");
+        std::shared_ptr<comphelper::AsyncEvent> pAsync25;
+        {
+            comphelper::ProfileZone aZone2("block2");
 
-        // Leaving this scope will generate an 'X' event for block2
-    }
+            // This does not generate any 'e' event as it was created when recording was off
+            // And the nested async2 object will thus not generate anything either
+            pAsync1.reset();
 
-    // Verify that the weak_ptr to pAsync2 has expired as its parent pAsync1 has been finished off
-    CPPUNIT_ASSERT(pAsync2.expired());
+            // This will generate 'b' event and an 'e' event when the pointer is reset or goes out of scope
+            pAsync25 = std::make_shared<comphelper::AsyncEvent>("async2.5");
 
-    // This will generate a 'b' event for async3
-    auto pAsync3(std::make_shared<comphelper::AsyncEvent>("async3"));
+            // Leaving this scope will generate an 'X' event for block2
+        }
 
-    std::weak_ptr<comphelper::AsyncEvent> pAsync4;
+        // Verify that the weak_ptr to pAsync2 has expired as its parent pAsync1 has been finished off
+        CPPUNIT_ASSERT(pAsync2.expired());
 
-    {
-        comphelper::ProfileZone aZone3("block3");
+        // This will generate a 'b' event for async3
+        auto pAsync3(std::make_shared<comphelper::AsyncEvent>("async3"));
 
-        pAsync4 = comphelper::AsyncEvent::createWithParent("async4in3", pAsync3);
+        std::weak_ptr<comphelper::AsyncEvent> pAsync4;
 
-        // Leaving this scope will generate an 'X' event for block3
-    }
+        {
+            comphelper::ProfileZone aZone3("block3");
 
-    // This will generate an 'e' event for async2.5
-    pAsync25.reset();
+            pAsync4 = comphelper::AsyncEvent::createWithParent("async4in3", pAsync3);
 
-    comphelper::ProfileZone aZone4("test2");
+            // Leaving this scope will generate an 'X' event for block3
+        }
 
-    // This will generate an 'i' event for instant2"
-    comphelper::TraceEvent::addInstantEvent("instant2");
+        // This will generate an 'e' event for async2.5
+        pAsync25.reset();
 
-    std::weak_ptr<comphelper::AsyncEvent> pAsync5;
-    {
-        auto pAsync4Locked = pAsync4.lock();
-        CPPUNIT_ASSERT(pAsync4Locked);
-        // This will generate a 'b' event for async5in4
-        pAsync5 = comphelper::AsyncEvent::createWithParent("async5in4", pAsync4Locked);
-    }
+        comphelper::ProfileZone aZone4("test2");
+
+        // This will generate an 'i' event for instant2"
+        comphelper::TraceEvent::addInstantEvent("instant2");
+
+        std::weak_ptr<comphelper::AsyncEvent> pAsync5;
+        {
+            auto pAsync4Locked = pAsync4.lock();
+            CPPUNIT_ASSERT(pAsync4Locked);
+            // This will generate a 'b' event for async5in4
+            pAsync5 = comphelper::AsyncEvent::createWithParent("async5in4", pAsync4Locked);
+        }
 
-    CPPUNIT_ASSERT(!pAsync5.expired());
+        CPPUNIT_ASSERT(!pAsync5.expired());
 
-    // This will generate a 'b' event for async6in5
-    std::weak_ptr<comphelper::AsyncEvent> pAsync6(
-        comphelper::AsyncEvent::createWithParent("async6in5", pAsync5.lock()));
-    CPPUNIT_ASSERT(!pAsync6.expired());
+        // This will generate a 'b' event for async6in5
+        std::weak_ptr<comphelper::AsyncEvent> pAsync6(
+            comphelper::AsyncEvent::createWithParent("async6in5", pAsync5.lock()));
+        CPPUNIT_ASSERT(!pAsync6.expired());
 
-    // This will generate an 'e' event for async6in5 and async5in4
-    pAsync5.lock()->finish();
+        // This will generate an 'e' event for async6in5 and async5in4
+        pAsync5.lock()->finish();
 
-    CPPUNIT_ASSERT(pAsync6.expired());
+        pAsync7Locked = comphelper::AsyncEvent::createWithParent("async7in3", pAsync3).lock();
+
+        CPPUNIT_ASSERT(pAsync6.expired());
+
+        // Leaving this scope will generate 'X' events for test2 and a
+        // 'e' event for async4in3, async7in3, and async3. The
+        // pAsync7Locked pointer will now point to an AsyncEvent
+        // object that has already had its 'e' event generated.
+    }
 
-    // Leaving this scope will generate 'X' events for test2 and a 'e' event for async4in3 and async3
+    // Nothing is generated from this
+    pAsync7Locked.reset();
 }
 }
 
@@ -124,7 +136,7 @@ void TestTraceEvent::test()
         std::cerr << s << "\n";
     }
 
-    CPPUNIT_ASSERT_EQUAL(15, static_cast<int>(aEvents.size()));
+    CPPUNIT_ASSERT_EQUAL(17, static_cast<int>(aEvents.size()));
 
     CPPUNIT_ASSERT(aEvents[0].startsWith("{\"name:\"instant1\",\"ph\":\"i\","));
     CPPUNIT_ASSERT(aEvents[1].startsWith("{\"name\":\"async2.5\",\"ph\":\"b\",\"id\":1,"));
@@ -138,9 +150,11 @@ void TestTraceEvent::test()
     CPPUNIT_ASSERT(aEvents[9].startsWith("{\"name\":\"async6in5\",\"ph\":\"b\",\"id\":2,"));
     CPPUNIT_ASSERT(aEvents[10].startsWith("{\"name\":\"async6in5\",\"ph\":\"e\",\"id\":2,"));
     CPPUNIT_ASSERT(aEvents[11].startsWith("{\"name\":\"async5in4\",\"ph\":\"e\",\"id\":2,"));
-    CPPUNIT_ASSERT(aEvents[12].startsWith("{\"name\":\"test2\",\"ph\":\"X\""));
-    CPPUNIT_ASSERT(aEvents[13].startsWith("{\"name\":\"async4in3\",\"ph\":\"e\",\"id\":2,"));
-    CPPUNIT_ASSERT(aEvents[14].startsWith("{\"name\":\"async3\",\"ph\":\"e\",\"id\":2,"));
+    CPPUNIT_ASSERT(aEvents[12].startsWith("{\"name\":\"async7in3\",\"ph\":\"b\",\"id\":2,"));
+    CPPUNIT_ASSERT(aEvents[13].startsWith("{\"name\":\"test2\",\"ph\":\"X\""));
+    CPPUNIT_ASSERT(aEvents[14].startsWith("{\"name\":\"async4in3\",\"ph\":\"e\",\"id\":2,"));
+    CPPUNIT_ASSERT(aEvents[15].startsWith("{\"name\":\"async7in3\",\"ph\":\"e\",\"id\":2,"));
+    CPPUNIT_ASSERT(aEvents[16].startsWith("{\"name\":\"async3\",\"ph\":\"e\",\"id\":2,"));
 }
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestTraceEvent);
diff --git a/include/comphelper/traceevent.hxx b/include/comphelper/traceevent.hxx
index eed67c0c6f9b..44805d21a6df 100644
--- a/include/comphelper/traceevent.hxx
+++ b/include/comphelper/traceevent.hxx
@@ -12,9 +12,9 @@
 
 #include <sal/config.h>
 
+#include <algorithm>
 #include <atomic>
 #include <memory>
-#include <set>
 #include <vector>
 
 #include <osl/process.h>
@@ -74,7 +74,25 @@ protected:
     }
 };
 
-// An AsyncEvent generates a 'b' (begin) event when constructed and an 'e' (end) event when destructed
+// An AsyncEvent generates a 'b' (begin) event when constructed and an 'e' (end) event when it
+// itself or its nesting parent (if there is one) is destructed (or earlier, if requested)
+
+// There are two kinds of async event pairs: Freestanding ones that are not related to other events
+// at all, and nested ones that have to be nested between the 'b' and 'e' events of a parent Async
+// event.
+
+// To generate a pair of 'b' and 'e' events, create an AsyncEvent object using the AsyncEvent(const
+// char* sName) constructor when you want the 'b' event to be generated, and destroy it when you
+// want the corresponding 'e' event to be generated.
+
+// To generate a pair of 'b' and 'e' events that is nested inside an outer 'b' and 'e' event pair,
+// create an AsyncEvent object using the createWithParent() function. It returns a weak reference
+// (weak_ptr) to the AsyncEvent. The parent keeps a strong reference (shared_ptr) to it.
+
+// The 'e' event will be generated when the parent is about to go away, before the parent's 'e'
+// event. When the parent has gone away, the weak reference will have expired. You can also generate
+// it explicitly by calling the finish() function. (But in that case you could as well have used a
+// freestanding AsyncEvent object, I think.)
 
 class COMPHELPER_DLLPUBLIC AsyncEvent : public NamedEvent,
                                         public std::enable_shared_from_this<AsyncEvent>
@@ -82,7 +100,7 @@ class COMPHELPER_DLLPUBLIC AsyncEvent : public NamedEvent,
     static int s_nIdCounter;
     int m_nId;
     int m_nPid;
-    std::set<std::shared_ptr<AsyncEvent>> m_aChildren;
+    std::vector<std::shared_ptr<AsyncEvent>> m_aChildren;
     std::weak_ptr<AsyncEvent> m_pParent;
     bool m_bBeginRecorded;
 
@@ -119,16 +137,18 @@ class COMPHELPER_DLLPUBLIC AsyncEvent : public NamedEvent,
         }
     }
 
-public:
-    AsyncEvent(const char* sName)
-        : AsyncEvent(sName, s_nIdCounter++)
-    {
-    }
-
-    ~AsyncEvent()
+    void generateEnd()
     {
         if (m_bBeginRecorded)
         {
+            m_bBeginRecorded = false;
+
+            // In case somebody is holding on to a hard reference to a child we need to tell the
+            // children to finish up explicitly, we can't rely on our pointers to them being the
+            // only ones.
+            for (auto& i : m_aChildren)
+                i->generateEnd();
+
             m_aChildren.clear();
 
             long long nNow = getNow();
@@ -153,6 +173,14 @@ public:
         }
     }
 
+public:
+    AsyncEvent(const char* sName)
+        : AsyncEvent(sName, s_nIdCounter++)
+    {
+    }
+
+    ~AsyncEvent() { generateEnd(); }
+
     static std::weak_ptr<AsyncEvent> createWithParent(const char* sName,
                                                       std::shared_ptr<AsyncEvent> pParent)
     {
@@ -161,7 +189,7 @@ public:
         if (s_bRecording && pParent->m_bBeginRecorded)
         {
             pResult.reset(new AsyncEvent(sName, pParent->m_nId));
-            pParent->m_aChildren.insert(pResult);
+            pParent->m_aChildren.push_back(pResult);
             pResult->m_pParent = pParent;
         }
 
@@ -170,13 +198,16 @@ public:
 
     void finish()
     {
-        // This makes sense to call only for a nested AsyncEvent. To finish up a non-nested AsyncEvent you
-        // just need to release your sole owning pointer to it.
+        generateEnd();
+
         auto pParent = m_pParent.lock();
         if (!pParent)
             return;
-        pParent->m_aChildren.erase(shared_from_this());
-        m_aChildren.clear();
+
+        pParent->m_aChildren.erase(std::remove(pParent->m_aChildren.begin(),
+                                               pParent->m_aChildren.end(), shared_from_this()),
+                                   pParent->m_aChildren.end());
+        m_pParent.reset();
     }
 };