Changeset 95714 in vbox

Timestamp:

Jul 18, 2022 4:13:16 PM (3 years ago)

Author:

vboxsync

Message:

Recording/Main: Object sharing fixes for the per-screen settings by using an internal refcount. This is needed as a peer object could share the same per-screen object when saving / committing settings. bugref:9286

Location:

trunk/src/VBox/Main

Files:

• include/RecordingScreenSettingsImpl.h (modified) (1 diff)
• src-server/RecordingScreenSettingsImpl.cpp (modified) (9 diffs)
• src-server/RecordingSettingsImpl.cpp (modified) (9 diffs)

trunk/src/VBox/Main/include/RecordingScreenSettingsImpl.h

@@ -59 +59 @@
 
     settings::RecordingScreenSettings &i_getData(void);
+
+    int32_t i_reference(void);
+    int32_t i_release(void);
+    int32_t i_getReferences(void);
 
 private:

trunk/src/VBox/Main/src-server/RecordingScreenSettingsImpl.cpp

@@ -42 +42 @@
     Data()
         : pParent(NULL)
+        , cRefs(0)
     { }
 
@@ -47 +48 @@
     const ComObjPtr<RecordingScreenSettings> pPeer;
     uint32_t                                 uScreenId;
+    /** Internal reference count to track sharing of this screen settings object among
+     *  other recording settings objects. */
+    int32_t                                  cRefs;
 
     // use the XML settings structure in the members for simplicity
@@ -146 +150 @@
     m->bd.share(aThat->m->bd);
 
-    HRESULT rc = S_OK;
+    HRESULT hrc = S_OK;
 
     int vrc = i_initInternal();
@@ -156 +160 @@
     {
         autoInitSpan.setFailed();
-        rc = E_UNEXPECTED;
+        hrc = E_UNEXPECTED;
     }
 
     LogFlowThisFuncLeave();
-    return rc;
+    return hrc;
 }
 
@@ -214 +218 @@
 void RecordingScreenSettings::uninit()
 {
-    LogFlowThisFuncEnter();
+    LogThisFunc(("%p\n", this));
 
     /* Enclose the state transition Ready->InUninit->NotReady */
@@ -221 +225 @@
         return;
 
+    /* Make sure nobody holds an internal reference to it anymore. */
+    AssertReturnVoid(m->cRefs == 0);
+
     m->bd.free();
 
@@ -930 +937 @@
 {
     AssertPtrReturn(m, VERR_INVALID_POINTER);
+
+    i_reference();
 
     int vrc = i_parseOptionsString(m->bd->strOptions, *m->bd.data());
@@ -983 +992 @@
 HRESULT RecordingScreenSettings::i_saveSettings(settings::RecordingScreenSettings &data)
 {
+    LogThisFunc(("%p: Screen %RU32\n", this, m ? m->uScreenId : UINT32_MAX));
+
     /* sanity */
     AutoCaller autoCaller(this);
@@ -1071 +1082 @@
     AssertPtr(m);
     return *m->bd.data();
+}
+
+/**
+ * Increments the reference count.
+ *
+ * @returns New reference count.
+ *
+ * @note    Internal reference count, to track object sharing across different recording settings objects
+ *          which share the same screen recording data.
+ */
+int32_t RecordingScreenSettings::i_reference(void)
+{
+    int cNewRefs = ASMAtomicIncS32(&m->cRefs); RT_NOREF(cNewRefs);
+    LogThisFunc(("%p: cRefs -> %RI32\n", this, cNewRefs));
+    return cNewRefs;
+}
+
+/**
+ * Decrements the reference count.
+ *
+ * @returns New reference count.
+ *
+ * @note    Internal reference count, to track object sharing across different recording settings objects
+ *          which share the same screen recording data.
+ */
+int32_t RecordingScreenSettings::i_release(void)
+{
+    int32_t cNewRefs = ASMAtomicDecS32(&m->cRefs); RT_NOREF(cNewRefs);
+    LogThisFunc(("%p: cRefs -> %RI32\n", this, cNewRefs));
+    AssertReturn(cNewRefs >= 0, 0);
+    return cNewRefs;
+}
+
+/**
+ * Returns the current reference count.
+ *
+ * @returns Current reference count.
+ *
+ * @note    Internal reference count, to track object sharing across different recording settings objects
+ *          which share the same screen recording data.
+ */
+int32_t RecordingScreenSettings::i_getReferences(void)
+{
+    return ASMAtomicReadS32(&m->cRefs);
 }
 

trunk/src/VBox/Main/src-server/RecordingSettingsImpl.cpp

@@ -127 +127 @@
 
     m->bd.share(aThat->m->bd);
+
+    /* Make sure to add a reference when sharing the screen objects with aThat. */
+    for (RecordingScreenSettingsObjMap::const_iterator itScreenThat  = aThat->m->mapScreenObj.begin();
+                                                       itScreenThat != aThat->m->mapScreenObj.end();
+                                                     ++itScreenThat)
+        itScreenThat->second->i_reference();
+
     m->mapScreenObj = aThat->m->mapScreenObj;
 
@@ -202 +209 @@
         return;
 
-    /* Note: Do *not* call i_reset() here, as the shared recording configuration
-     *       otherwise gets destructed when this object goes out of scope or is destroyed. */
+    /* Make sure to destroy screen objects attached to this object.
+     * Note: This also decrements the refcount of a screens object, in case it's shared among other recording settings. */
+    i_destroyAllScreenObj(m->mapScreenObj);
 
     m->bd.free();
@@ -369 +377 @@
     AssertReturn(screenSettingsMap.find(idScreen) == screenSettingsMap.end(), VERR_ALREADY_EXISTS);
 
-    LogFlowThisFunc(("Screen %RU32\n", idScreen));
-
     int vrc = VINF_SUCCESS;
 
@@ -391 +397 @@
     }
 
+    LogThisFunc(("%p: Screen %RU32 -> %Rrc\n", recordingScreenSettings.m_p, idScreen, vrc));
     return vrc;
 }
@@ -397 +404 @@
  * Removes a screen settings object from a particular map.
  *
- * @returns IPRT status code. VERR_NOT_FOUND if specified screen was not found.
+ * If the internal reference count hits 0, the screen settings object will be destroyed.
+ * This means that this screen settings object is not being used anymore by other recording settings (as shared data).
+ *
+ * @returns IPRT status code.
+ * @retval  VERR_NOT_FOUND if specified screen was not found.
  * @param   screenSettingsMap   Map to remove screen settings from.
  * @param   idScreen            ID of screen to remove.
@@ -403 +414 @@
 int RecordingSettings::i_destroyScreenObj(RecordingScreenSettingsObjMap &screenSettingsMap, uint32_t idScreen)
 {
-    LogFlowThisFunc(("Screen %RU32\n", idScreen));
-
-    AssertReturn(idScreen > 0, VERR_INVALID_PARAMETER); /* Removing screen 0 isn't a good idea. */
     AssertReturn(screenSettingsMap.find(idScreen) != screenSettingsMap.end(), VERR_NOT_FOUND);
 
@@ -416 +424 @@
     screenSettingsMap.erase(itScreen);
 
-    pScreenSettings.setNull();
+    LogThisFunc(("%p: Screen %RU32, cRefs=%RI32\n", pScreenSettings.m_p, idScreen, pScreenSettings->i_getReferences()));
+
+    pScreenSettings->i_release();
+
+    /* Only destroy the object if nobody else keeps a reference to it anymore. */
+    if (pScreenSettings->i_getReferences() == 0)
+    {
+        LogThisFunc(("%p: Screen %RU32 -> Null\n", pScreenSettings.m_p, idScreen));
+        pScreenSettings.setNull();
+    }
 
     return VINF_SUCCESS;
@@ -431 +448 @@
     LogFlowThisFuncEnter();
 
-    RecordingScreenSettingsObjMap::iterator itScreenObj = screenSettingsMap.begin();
-    while (itScreenObj != screenSettingsMap.end())
-    {
-        /* Make sure to consume the pointer before the one of the
-         * iterator gets released. */
-        ComObjPtr<RecordingScreenSettings> pScreenSettings = itScreenObj->second;
-
-        screenSettingsMap.erase(itScreenObj);
-
-        pScreenSettings->uninit();
-        pScreenSettings.setNull();
-
-        itScreenObj = screenSettingsMap.begin();
+    int vrc = VINF_SUCCESS;
+
+    RecordingScreenSettingsObjMap::iterator itScreen = screenSettingsMap.begin();
+    while (itScreen != screenSettingsMap.end())
+    {
+        vrc = i_destroyScreenObj(screenSettingsMap, itScreen->first);
+        if (RT_FAILURE(vrc))
+            break;
+
+        itScreen = screenSettingsMap.begin();
     }
 
     Assert(screenSettingsMap.size() == 0);
-    return VINF_SUCCESS;
+    return vrc;
 }
 
@@ -748 +762 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    LogThisFunc(("%p: cDisplays=%RU32 vs. %zu\n", this, cDisplays, m->mapScreenObj.size()));
+
     /* If counts match, take a shortcut. */
     if (cDisplays == m->mapScreenObj.size())