fix(dom): fix multi-thread access event_options conflict

* fix(android): remove vsync on destroy root * fix(dom): fix multi-thread access event_options conflict * fix(ios): fix remove vsync event when release on ios --------- Co-authored-by: maxli <[email protected]> Co-authored-by: etkmao <[email protected]>
Tencent · Sep 27, 2023 · abf6f1d · abf6f1d
1 parent 9c64bb7
commit abf6f1d
Show file tree

Hide file tree

Showing 13 changed files with 45 additions and 68 deletions.
diff --git a/dom/include/dom/animation/animation_manager.h b/dom/include/dom/animation/animation_manager.h
@@ -62,15 +62,6 @@ class AnimationManager
 
   void RemoveVSyncEventListener();
 
-  /**
-   * When destroying, it is necessary to remove the VSync event, but it should not indirectly create RenderNode due to EndBatch,
-   * otherwise there will be multi-threading issues between the main thread and the Dom thread.
-   * Therefore, a new method is needed to differentiate the destruction scenario.
-   * This has occasionally caused crashes on iOS.
-   * In the future, there will be better refactoring solutions for the main thread and Dom thread in the destruction scenario, and this method can be deleted.
-  */
-  void RemoveVSyncEventListenerOnRelease();
-
   void OnDomNodeCreate(const std::vector<std::shared_ptr<DomInfo>>& nodes) override;
   void OnDomNodeUpdate(const std::vector<std::shared_ptr<DomInfo>>& nodes) override;
   void OnDomNodeMove(const std::vector<std::shared_ptr<DomInfo>>& nodes) override;

diff --git a/dom/include/dom/dom_manager.h b/dom/include/dom/dom_manager.h
@@ -106,7 +106,6 @@ class DomManager : public std::enable_shared_from_this<DomManager> {
   static void UpdateAnimation(const std::weak_ptr<RootNode>& weak_root_node,
                        std::vector<std::shared_ptr<DomNode>>&& nodes);
   void EndBatch(const std::weak_ptr<RootNode>& root_node);
-  void EndBatchOnRelease(const std::weak_ptr<RootNode>& root_node);
   // 返回0代表失败，正常id从1开始
   static void AddEventListener(const std::weak_ptr<RootNode>& weak_root_node,
                         uint32_t dom_id,

diff --git a/dom/include/dom/root_node.h b/dom/include/dom/root_node.h
@@ -57,7 +57,6 @@ class RootNode : public DomNode {
   void UpdateAnimation(std::vector<std::shared_ptr<DomNode>>&& nodes);
   void CallFunction(uint32_t id, const std::string& name, const DomArgument& param, const CallFunctionCallback& cb);
   void SyncWithRenderManager(const std::shared_ptr<RenderManager>& render_manager);
-  void SyncWithRenderManagerOnRelease(const std::shared_ptr<RenderManager>& render_manager);
   void DoAndFlushLayout(const std::shared_ptr<RenderManager>& render_manager);
   void AddEvent(uint32_t id, const std::string& event_name);
   void RemoveEvent(uint32_t id, const std::string& event_name);

diff --git a/dom/src/dom/animation/animation_manager.cc b/dom/src/dom/animation/animation_manager.cc
@@ -366,22 +366,6 @@ void AnimationManager::RemoveVSyncEventListener() {
   }
 }
 
-void AnimationManager::RemoveVSyncEventListenerOnRelease() {
-  auto root_node = root_node_.lock();
-  if (!root_node) {
-    return;
-  }
-  auto weak_dom_manager = root_node->GetDomManager();
-  auto dom_manager = weak_dom_manager.lock();
-  if (!dom_manager) {
-    return;
-  }
-  if (dom_manager) {
-    dom_manager->RemoveEventListener(root_node, root_node->GetId(), kVSyncKey, listener_id_);
-    dom_manager->EndBatchOnRelease(root_node_);
-  }
-}
-
 void AnimationManager::UpdateAnimation(const std::shared_ptr<Animation>& animation, uint64_t now,
                                        std::unordered_map<uint32_t, std::shared_ptr<DomNode>>& update_node_map) {
   auto animation_id = animation->GetId();

diff --git a/dom/src/dom/dom_manager.cc b/dom/src/dom/dom_manager.cc
@@ -137,20 +137,6 @@ void DomManager::EndBatch(const std::weak_ptr<RootNode>& weak_root_node) {
   root_node->SyncWithRenderManager(render_manager);
 }
 
-void DomManager::EndBatchOnRelease(const std::weak_ptr<RootNode>& weak_root_node) {
-  auto render_manager = render_manager_.lock();
-  FOOTSTONE_DCHECK(render_manager);
-  if (!render_manager) {
-    return;
-  }
-  auto root_node = weak_root_node.lock();
-  if (!root_node) {
-    return;
-  }
-  FOOTSTONE_DLOG(INFO) << "[Hippy Statistic] total node size = " << root_node->GetChildCount();
-  root_node->SyncWithRenderManagerOnRelease(render_manager);
-}
-
 void DomManager::AddEventListener(const std::weak_ptr<RootNode>& weak_root_node, uint32_t dom_id,
                                   const std::string& name, uint64_t listener_id, bool use_capture,
                                   const EventCallback& cb) {

diff --git a/dom/src/dom/root_node.cc b/dom/src/dom/root_node.cc
@@ -64,9 +64,7 @@ void RootNode::RemoveEventListener(const std::string& name, uint64_t listener_id
   RemoveEvent(GetId(), name);
 }
 
-void RootNode::ReleaseResources() {
-  animation_manager_->RemoveVSyncEventListenerOnRelease();
-}
+void RootNode::ReleaseResources() {}
 
 void RootNode::CreateDomNodes(std::vector<std::shared_ptr<DomInfo>>&& nodes) {
   for (const auto& interceptor : interceptors_) {
@@ -284,24 +282,6 @@ void RootNode::SyncWithRenderManager(const std::shared_ptr<RenderManager>& rende
   render_manager->EndBatch(GetWeakSelf());
 }
 
-void RootNode::SyncWithRenderManagerOnRelease(const std::shared_ptr<RenderManager>& render_manager) {
-  for (auto& event_operation : event_operations_) {
-    const auto& node = GetNode(event_operation.id);
-    if (node == nullptr) {
-      continue;
-    }
-
-    switch (event_operation.op) {
-      case EventOperation::Op::kOpRemove:
-        render_manager->RemoveEventListener(GetWeakSelf(), node, event_operation.name);
-        break;
-      default:
-        break;
-    }
-  }
-  event_operations_.clear();
-}
-
 void RootNode::AddEvent(uint32_t id, const std::string& event_name) {
   event_operations_.push_back({EventOperation::Op::kOpAdd, id, event_name});
 }

diff --git a/framework/examples/ios-demo/HippyDemo/HippyConvenientBridge.mm b/framework/examples/ios-demo/HippyDemo/HippyConvenientBridge.mm
@@ -276,6 +276,7 @@ - (void)dealloc {
         _demoLoader->Terminate();
     }
     if (_rootNode) {
+        _nativeRenderManager->RemoveVSyncEventListener(_rootNode);
         _rootNode->ReleaseResources();
     }
 }

diff --git a/renderer/native/android/src/main/java/com/tencent/renderer/NativeRenderer.java b/renderer/native/android/src/main/java/com/tencent/renderer/NativeRenderer.java
@@ -59,6 +59,7 @@
 import com.tencent.renderer.serialization.Deserializer;
 import com.tencent.renderer.serialization.Serializer;
 import com.tencent.renderer.utils.ArrayUtils;
+import com.tencent.renderer.utils.ChoreographerUtils;
 import com.tencent.renderer.utils.DisplayUtils;
 import com.tencent.renderer.utils.EventUtils.EventType;
 
@@ -404,6 +405,7 @@ public void onResume() {
                 listener.onInstanceResume();
             }
         }
+        ChoreographerUtils.onResume();
     }
 
     @Override
@@ -413,6 +415,7 @@ public void onPause() {
                 listener.onInstancePause();
             }
         }
+        ChoreographerUtils.onPause();
     }
 
     @MainThread
@@ -423,6 +426,7 @@ public void destroyRoot(int rootId) {
                 listener.onInstanceDestroy(rootId);
             }
         }
+        ChoreographerUtils.unregisterDoFrameListener(getInstanceId(), rootId);
         mRenderManager.deleteNode(rootId, rootId);
         mRenderManager.batch(rootId);
     }

diff --git a/renderer/native/android/src/main/java/com/tencent/renderer/utils/ChoreographerUtils.java b/renderer/native/android/src/main/java/com/tencent/renderer/utils/ChoreographerUtils.java
@@ -26,6 +26,7 @@ public class ChoreographerUtils {
 
     public static final String DO_FRAME = "frameUpdate";
     private static boolean sEnablePostFrame = false;
+    private static boolean sInForeground = true;
     private static HashMap<Integer, ArrayList<Integer>> sListeners = null;
 
     private static void handleDoFrameCallback() {
@@ -42,18 +43,28 @@ private static void handleDoFrameCallback() {
     }
 
     private static void doPostFrame() {
-        Choreographer.FrameCallback frameCallback = new Choreographer.FrameCallback() {
-            @Override
-            public void doFrame(long frameTimeNanos) {
+        Choreographer.FrameCallback frameCallback = frameTimeNanos -> {
+            if (sEnablePostFrame && sInForeground) {
                 handleDoFrameCallback();
-                if (sEnablePostFrame) {
-                    doPostFrame();
-                }
+                doPostFrame();
             }
         };
         Choreographer.getInstance().postFrameCallback(frameCallback);
     }
 
+    @MainThread
+    public static void onResume() {
+        sInForeground = true;
+        if (sEnablePostFrame) {
+            doPostFrame();
+        }
+    }
+
+    @MainThread
+    public static void onPause() {
+        sInForeground = false;
+    }
+
     /**
      * Register frame callback listener, should call in ui thread.
      *

diff --git a/renderer/native/ios/renderer/NativeRenderImpl.h b/renderer/native/ios/renderer/NativeRenderImpl.h
@@ -228,6 +228,11 @@ class HippyValue;
            forDomNodeId:(int32_t)node_id
              onRootNode:(std::weak_ptr<hippy::RootNode>)rootNode;
 
+/**
+ * unregister vsync event
+ */
+- (void)removeVSyncEventOnRootNode:(std::weak_ptr<hippy::RootNode>)rootNode;
+
 /**
  * Set root view size changed event callback
  *

diff --git a/renderer/native/ios/renderer/NativeRenderImpl.mm b/renderer/native/ios/renderer/NativeRenderImpl.mm
@@ -1386,6 +1386,11 @@ - (void)removeEventName:(const std::string &)eventName
     }
 }
 
+- (void)removeVSyncEventOnRootNode:(std::weak_ptr<hippy::RootNode>)rootNode {
+    NSString *vsyncKey = [NSString stringWithFormat:@"%p-%d", self, static_cast<int>(rootNode.lock()->GetId())];
+    [[RenderVsyncManager sharedInstance] unregisterVsyncObserverForKey:vsyncKey];
+}
+
 - (void)addPropertyEvent:(const std::string &)name forDomNode:(int32_t)node_id
               onRootNode:(std::weak_ptr<RootNode>)rootNode {
     AssertMainQueue();

diff --git a/renderer/native/ios/renderer/NativeRenderManager.h b/renderer/native/ios/renderer/NativeRenderManager.h
@@ -127,6 +127,11 @@ class NativeRenderManager : public hippy::RenderManager ,public std::enable_shar
      */
     void RemoveEventListener(std::weak_ptr<hippy::RootNode> root_node, std::weak_ptr<hippy::DomNode> dom_node, const std::string &name) override;
 
+    /**
+     * unregister vsync event
+     */
+    void RemoveVSyncEventListener(std::weak_ptr<hippy::RootNode> root_node);
+
     /**
      * invoke function of view
      *

diff --git a/renderer/native/ios/renderer/NativeRenderManager.mm b/renderer/native/ios/renderer/NativeRenderManager.mm
@@ -146,6 +146,13 @@
     }
 }
 
+void NativeRenderManager::RemoveVSyncEventListener(std::weak_ptr<hippy::RootNode> root_node) {
+    @autoreleasepool {
+        HPAssert(renderImpl_, @"renderImpl_ is null, did you forget to call Initialize()?");
+        [renderImpl_ removeVSyncEventOnRootNode:root_node];
+    }
+}
+
 void NativeRenderManager::CallFunction(std::weak_ptr<hippy::RootNode> root_node,
                                        std::weak_ptr<DomNode> dom_node,
                                        const std::string &name,