Mercurial > hg > orthanc-stone

changeset 1328:fd616c4a5904 broker

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Added mechanism to prevent callbacks from being sent on dead WebAssemblyViewport objects
author Benjamin Golinvaux <bgo@osimis.io>
date Fri, 27 Mar 2020 12:54:27 +0100
parents 4f8db2d202c8
children 8d3e669f01a2
files Framework/Toolbox/GenericToolbox.h Framework/Viewport/WebAssemblyViewport.cpp Framework/Viewport/WebAssemblyViewport.h UnitTestsSources/GenericToolboxTests.cpp
diffstat 4 files changed, 142 insertions(+), 45 deletions(-) [+]
line wrap: on
line diff
--- a/Framework/Toolbox/GenericToolbox.h	Wed Mar 25 14:34:27 2020 +0100
+++ b/Framework/Toolbox/GenericToolbox.h	Fri Mar 27 12:54:27 2020 +0100
@@ -20,10 +20,15 @@
 
 #pragma once
 
+#include <Core/Compatibility.h>
+
+#include <boost/shared_ptr.hpp>
+
 #include <string>
 #include <stdint.h>
 #include <math.h>
 
+#include <memory>
 
 namespace OrthancStone
 {
@@ -281,14 +286,52 @@
     /**
     Same as GetRgbValuesFromString
     */
-    bool GetRgbaValuesFromString(uint8_t& red, uint8_t& green, uint8_t& blue, uint8_t& alpha, const char* text);
+    bool GetRgbaValuesFromString(uint8_t& red,
+                                 uint8_t& green,
+                                 uint8_t& blue,
+                                 uint8_t& alpha,
+                                 const char* text);
 
     /**
     Same as GetRgbValuesFromString
     */
-    inline bool GetRgbaValuesFromString(uint8_t& red, uint8_t& green, uint8_t& blue, uint8_t& alpha, const std::string& text)
+    inline bool GetRgbaValuesFromString(uint8_t& red, 
+                                        uint8_t& green, 
+                                        uint8_t& blue, 
+                                        uint8_t& alpha, 
+                                        const std::string& text)
     {
       return GetRgbaValuesFromString(red, green, blue, alpha, text.c_str());
     }
+
+
+    template<typename Wrappee> 
+    struct HoldingRef
+    {
+      HoldingRef(Wrappee* object)
+      {
+        // a crash here means that the object is not stored in a shared_ptr
+        // using shared_ptr is mandatory for this
+        object_ = object->shared_from_this();
+      }
+      boost::shared_ptr<Wrappee> object_;
+
+      static void* Wrap(Wrappee* object)
+      {
+        std::unique_ptr<HoldingRef<Wrappee > > up(new HoldingRef<Wrappee>(object));
+        void* userData = reinterpret_cast<void*>(up.release());
+        return userData;
+      }
+
+      static boost::shared_ptr<Wrappee> Unwrap(void* userData)
+      {
+        // the stored shared_ptr will be deleted because of wrapping
+        // the data in a RAII
+        std::unique_ptr<HoldingRef<Wrappee > >
+          up(reinterpret_cast<HoldingRef<Wrappee>*>(userData));
+        boost::shared_ptr<Wrappee> object = up->object_;
+        return object;
+      }
+    };
   }
 }
--- a/Framework/Viewport/WebAssemblyViewport.cpp	Wed Mar 25 14:34:27 2020 +0100
+++ b/Framework/Viewport/WebAssemblyViewport.cpp	Fri Mar 27 12:54:27 2020 +0100
@@ -21,9 +21,12 @@
 
 #include "WebAssemblyViewport.h"
 
+#include "../Toolbox/GenericToolbox.h"
+
 #include <Core/OrthancException.h>
 
 #include <boost/make_shared.hpp>
+#include <boost/enable_shared_from_this.hpp>
 
 namespace OrthancStone
 {
@@ -103,12 +106,12 @@
 
   EM_BOOL WebAssemblyViewport::OnRequestAnimationFrame(double time, void *userData)
   {
-    WebAssemblyViewport& that = *reinterpret_cast<WebAssemblyViewport*>(userData);
+    boost::shared_ptr<WebAssemblyViewport> that = GenericToolbox::HoldingRef<WebAssemblyViewport>::Unwrap(userData);
 
-    if (that.compositor_.get() != NULL &&
-        that.controller_ /* should always be true */)
+    if (that->compositor_.get() != NULL &&
+        that->controller_ /* should always be true */)
     {
-      that.Paint(*that.compositor_, *that.controller_);
+      that->Paint(*that->compositor_, *that->controller_);
     }
       
     return true;
@@ -116,12 +119,12 @@
 
   EM_BOOL WebAssemblyViewport::OnResize(int eventType, const EmscriptenUiEvent *uiEvent, void *userData)
   {
-    WebAssemblyViewport& that = *reinterpret_cast<WebAssemblyViewport*>(userData);
+    boost::shared_ptr<WebAssemblyViewport> that = GenericToolbox::HoldingRef<WebAssemblyViewport>::Unwrap(userData);
 
-    if (that.compositor_.get() != NULL)
+    if (that->compositor_.get() != NULL)
     {
-      that.UpdateSize(*that.compositor_);
-      that.Invalidate();
+      that->UpdateSize(*that->compositor_);
+      that->Invalidate();
     }
       
     return true;
@@ -130,20 +133,20 @@
 
   EM_BOOL WebAssemblyViewport::OnMouseDown(int eventType, const EmscriptenMouseEvent *mouseEvent, void *userData)
   {
-    WebAssemblyViewport& that = *reinterpret_cast<WebAssemblyViewport*>(userData);
+    boost::shared_ptr<WebAssemblyViewport> that = GenericToolbox::HoldingRef<WebAssemblyViewport>::Unwrap(userData);
 
-    LOG(INFO) << "mouse down: " << that.GetFullCanvasId();      
+    LOG(INFO) << "mouse down: " << that->GetFullCanvasId();      
 
-    if (that.compositor_.get() != NULL &&
-        that.interactor_.get() != NULL)
+    if (that->compositor_.get() != NULL &&
+        that->interactor_.get() != NULL)
     {
       PointerEvent pointer;
-      ConvertMouseEvent(pointer, *mouseEvent, *that.compositor_);
+      ConvertMouseEvent(pointer, *mouseEvent, *that->compositor_);
 
-      that.controller_->HandleMousePress(*that.interactor_, pointer,
-                                         that.compositor_->GetCanvasWidth(),
-                                         that.compositor_->GetCanvasHeight());        
-      that.Invalidate();
+      that->controller_->HandleMousePress(*that->interactor_, pointer,
+                                         that->compositor_->GetCanvasWidth(),
+                                         that->compositor_->GetCanvasHeight());        
+      that->Invalidate();
     }
 
     return true;
@@ -152,44 +155,41 @@
     
   EM_BOOL WebAssemblyViewport::OnMouseMove(int eventType, const EmscriptenMouseEvent *mouseEvent, void *userData)
   {
-    WebAssemblyViewport& that = *reinterpret_cast<WebAssemblyViewport*>(userData);
+    boost::shared_ptr<WebAssemblyViewport> that = GenericToolbox::HoldingRef<WebAssemblyViewport>::Unwrap(userData);
 
-    if (that.compositor_.get() != NULL &&
-        that.controller_->HasActiveTracker())
+    if (that->compositor_.get() != NULL &&
+        that->controller_->HasActiveTracker())
     {
       PointerEvent pointer;
-      ConvertMouseEvent(pointer, *mouseEvent, *that.compositor_);
-      if (that.controller_->HandleMouseMove(pointer))
+      ConvertMouseEvent(pointer, *mouseEvent, *that->compositor_);
+      if (that->controller_->HandleMouseMove(pointer))
       {
-        that.Invalidate();
+        that->Invalidate();
       }
     }
 
     return true;
   }
+    
+  EM_BOOL WebAssemblyViewport::OnMouseUp(int eventType, const EmscriptenMouseEvent *mouseEvent, void *userData)
+  {
+    boost::shared_ptr<WebAssemblyViewport> that = GenericToolbox::HoldingRef<WebAssemblyViewport>::Unwrap(userData);
+
+    if (that->compositor_.get() != NULL)
+    {
+      PointerEvent pointer;
+      ConvertMouseEvent(pointer, *mouseEvent, *that->compositor_);
+      that->controller_->HandleMouseRelease(pointer);
+      that->Invalidate();
+    }
+
+    return true;
+  }
 
-    
-  EM_BOOL WebAssemblyViewport::OnMouseUp(int eventType, const EmscriptenMouseEvent *mouseEvent, void *userData)
-  {
-    WebAssemblyViewport& that = *reinterpret_cast<WebAssemblyViewport*>(userData);
-
-    if (that.compositor_.get() != NULL)
-    {
-      PointerEvent pointer;
-      ConvertMouseEvent(pointer, *mouseEvent, *that.compositor_);
-      that.controller_->HandleMouseRelease(pointer);
-      that.Invalidate();
-    }
-
-    return true;
-  }
-
-    
   void WebAssemblyViewport::Invalidate()
   {
-    emscripten_request_animation_frame(OnRequestAnimationFrame, this);
+    emscripten_request_animation_frame(OnRequestAnimationFrame, GenericToolbox::HoldingRef<WebAssemblyViewport>::Wrap(this));
   }
-    
 
   void WebAssemblyViewport::AcquireCompositor(ICompositor* compositor /* takes ownership */)
   {
--- a/Framework/Viewport/WebAssemblyViewport.h	Wed Mar 25 14:34:27 2020 +0100
+++ b/Framework/Viewport/WebAssemblyViewport.h	Fri Mar 27 12:54:27 2020 +0100
@@ -36,7 +36,9 @@
 
 namespace OrthancStone
 {
-  class WebAssemblyViewport : public IViewport
+  class WebAssemblyViewport : public IViewport,
+                              public boost::enable_shared_from_this<WebAssemblyViewport>
+
   {
   private:
     class WasmLock;
@@ -84,6 +86,9 @@
 
     virtual ILock* Lock() ORTHANC_OVERRIDE;
 
+    /**
+    This method takes ownership
+    */
     void AcquireInteractor(IViewportInteractor* interactor);
 
     const std::string& GetShortCanvasId() const
--- a/UnitTestsSources/GenericToolboxTests.cpp	Wed Mar 25 14:34:27 2020 +0100
+++ b/UnitTestsSources/GenericToolboxTests.cpp	Fri Mar 27 12:54:27 2020 +0100
@@ -22,6 +22,7 @@
 
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/lexical_cast.hpp>
+#include <boost/enable_shared_from_this.hpp>
 
 #include "gtest/gtest.h"
 
@@ -4243,3 +4244,51 @@
   EXPECT_EQ(0, green);
   EXPECT_EQ(0, blue);
 }
+
+static int Test_GenericToolbox_HoldingRef_ctor_trace = 100;
+static int Test_GenericToolbox_HoldingRef_dtor_trace = 1000;
+
+class Test_GenericToolbox_HoldingRef : public boost::enable_shared_from_this<Test_GenericToolbox_HoldingRef>
+{
+public:
+  Test_GenericToolbox_HoldingRef() : answer_(42) { Test_GenericToolbox_HoldingRef_ctor_trace++; }
+  unsigned int answer_;
+
+  ~Test_GenericToolbox_HoldingRef()
+  {
+    Test_GenericToolbox_HoldingRef_dtor_trace--;
+  }
+};
+
+TEST(GenericToolbox, HoldingRef)
+{
+  using namespace OrthancStone::GenericToolbox;
+
+  void* userData = NULL;
+
+  {
+    boost::shared_ptr<Test_GenericToolbox_HoldingRef> test(new Test_GenericToolbox_HoldingRef() );
+    Test_GenericToolbox_HoldingRef* This = test.get();
+    userData = HoldingRef<Test_GenericToolbox_HoldingRef>::Wrap(This);
+    EXPECT_EQ(42u,This->answer_);
+    This->answer_ = 0xdeadbeef;
+    EXPECT_EQ(0xdeadbeef,This->answer_);
+  }
+  
+  EXPECT_EQ(101,Test_GenericToolbox_HoldingRef_ctor_trace);
+  EXPECT_EQ(1000,Test_GenericToolbox_HoldingRef_dtor_trace);
+  
+  {
+    boost::shared_ptr<Test_GenericToolbox_HoldingRef> test2 = HoldingRef<Test_GenericToolbox_HoldingRef>::Unwrap(userData);
+    EXPECT_EQ(0xdeadbeef,test2->answer_);
+  }
+  
+  EXPECT_EQ(1001,Test_GenericToolbox_HoldingRef_dtor_trace); // make sure wrapper was deleted once
+}
+
+
+
+
+
+
+