CFRlogger accessviolation (#706)

* Cleanup CFRLogger

Made some changes to CFRLogger so that the implementation is more consistent with the other ServiceTracker classes in CoreBundleContext.cpp (i.e ServiceHooks.cpp). Signed-off-by The MathWorks Inc. <pelliott@mathworks.com>

* Fixes for unused parameter

Fixes for mac platform. Unused cfrContext parameter. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* CFRLogger changes

Create member variable for BundleContext. Remove IsOpen calls from Log methods for efficiency sake. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Update CFRLogger.cpp

Fix std::move error on mac platform. "error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]" Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* CFRLogger code review feedback

Updated CFRLogger based on code review feedback. Removed unused variable in CFRLogger constructor, moved CFRLogger member variable in CoreBundleContext class so that it will be destroyed before the ServiceListeners member variable (to avoid access violation on Windows on shutdown), removed unused IsOpen method from CFRLogger. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

* Update CoreBundleContext and CFRLogger classes

Add comment to explain why member variable order in CoreBundleContext is important to maintain. Remove bOpen variable from CFRLogger class. Not used anywhere. Signed-off-by The MathWorks, Inc. <pelliott@mathworks.com>

framework/src/bundle/CoreBundleContext.cpp

@@ -87,14 +87,14 @@ std::unordered_map<std::string, Any> InitProperties(
 
 CoreBundleContext::CoreBundleContext(
   const std::unordered_map<std::string, Any>& props,
-  std::ostream* logger)
+  std::ostream* diagLogger)
   : id(globalId++)
   , frameworkProperties(InitProperties(props))
   , workingDir(ref_any_cast<std::string>(
       frameworkProperties.at(Constants::FRAMEWORK_WORKING_DIR)))
-  , logger(nullptr)
   , listeners(this)
   , services(this)
+  , logger(std::make_shared<cppmicroservices::cfrimpl::CFRLogger>())
   , serviceHooks(this)
   , bundleHooks(this)
   , bundleRegistry(this)
@@ -104,7 +104,7 @@ CoreBundleContext::CoreBundleContext(
 {
   auto enableDiagLog =
     any_cast<bool>(frameworkProperties.at(Constants::FRAMEWORK_LOG));
-  std::ostream* diagnosticLogger = (logger) ? logger : &std::clog;
+  std::ostream* diagnosticLogger = (diagLogger) ? diagLogger : &std::clog;
   sink = std::make_shared<detail::LogSink>(diagnosticLogger, enableDiagLog);
   systemBundle = std::shared_ptr<FrameworkPrivate>(new FrameworkPrivate(this));
   DIAG_LOG(*sink) << "created";
@@ -177,9 +177,7 @@ void CoreBundleContext::Init()
 
   bundleRegistry.Load();
 
-  // Initialize the CFRLogger. This is done here rather than in the constructor since
-  // we do not have access to a bundle context until this point.
-  logger = std::make_shared<cppmicroservices::cfrimpl::CFRLogger>(MakeBundleContext(systemBundle->bundleContext.Load().get()));
+  logger->Open();
 
   std::string execPath;
   try {
@@ -224,6 +222,7 @@ void CoreBundleContext::Init()
 void CoreBundleContext::Uninit0()
 {
   DIAG_LOG(*sink) << "uninit";
+  logger->Close();
   serviceHooks.Close();
   systemBundle->UninitSystemBundle();
 }

framework/src/bundle/CoreBundleContext.h

@@ -69,6 +69,18 @@ class FrameworkPrivate;
 class CoreBundleContext
 {
 public:
+  /**
+  * Please note: The order of the member variables in this class is important. When the 
+  * CoreBundleContext object is destroyed, it will call the destructors for the member 
+  * variables in the reverse order in which they are listed here. For example, serviceHooks 
+  * will be destroyed before the logger. The logger will be destroyed before the listeners, etc. 
+  * 
+  * The logger has a ServiceTracker member variable. When it is destroyed, the ServiceTracker::Close
+  * method is called to remove the ServiceListener. The logger object must be destroyed before the 
+  * listeners member variable is destroyed because when the listeners member variable is destroyed 
+  * it leaves the ServiceListeners data structures in an unusable state. If the logger object 
+  * destructor runs after the listeners object destructor, it results in an access violation. 
+  */
   /**
    * Framework id.
    */
@@ -98,14 +110,7 @@ class CoreBundleContext
   */
   std::shared_ptr<detail::LogSink> sink;
 
-  /**
-   * A LogService for logging framework messages via
-   * a default or user-provided LogService that are intended to be
-   * visible outside of the framework.
-   */
-  std::shared_ptr<cppmicroservices::cfrimpl::CFRLogger> logger;
-
-  /**
+   /**
    * Bundle Storage
    */
   std::unique_ptr<BundleStorage> storage;
@@ -125,6 +130,13 @@ class CoreBundleContext
    */
   ServiceRegistry services;
 
+  /**
+   * A LogService for logging framework messages via
+   * a default or user-provided LogService that are intended to be
+   * visible outside of the framework.
+   */
+  std::shared_ptr<cppmicroservices::cfrimpl::CFRLogger> logger;
+
   /**
    * All service hooks.
    */

framework/src/util/CFRLogger.cpp

@@ -21,25 +21,21 @@
  =============================================================================*/
 
 #include "CFRLogger.h"
+#include "CoreBundleContext.h"
+#include "cppmicroservices/GetBundleContext.h"
 
 namespace cppmicroservices {
 namespace cfrimpl {
-CFRLogger::CFRLogger(cppmicroservices::BundleContext context)
-  : cfrContext(std::move(context))
-  , serviceTracker(
-      std::make_unique<cppmicroservices::ServiceTracker<
-        cppmicroservices::logservice::LogService>>(cfrContext, this))
+CFRLogger::CFRLogger()
+  : serviceTracker()
   , logService(nullptr)
 {
-  serviceTracker->Open(); // Start tracking
 }
 
 CFRLogger::~CFRLogger()
 {
   try {
-    if (serviceTracker) {
-      serviceTracker->Close();
-    }
+    this->Close();
   } catch (...) {
   }
 }
@@ -117,5 +113,28 @@ void CFRLogger::Log(const cppmicroservices::ServiceReferenceBase& sr,
     currLogger->Log(sr, level, message, ex);
   }
 }
+
+void CFRLogger::Open()
+{
+  auto l = this->Lock();
+  US_UNUSED(l);
+  cfrContext = GetBundleContext();
+  if (!cfrContext) {
+    return;
+  }
+  serviceTracker = std::make_unique<cppmicroservices::ServiceTracker<cppmicroservices::logservice::LogService>> (cfrContext, this);
+  serviceTracker->Open();
+}
+
+void CFRLogger::Close() 
+{
+  auto l = this->Lock();
+  US_UNUSED(l);
+  if (serviceTracker) {
+    serviceTracker->Close();
+    serviceTracker.reset();
+  }
+}
+
 } // cfrimpl
 } // cppmicroservices

framework/src/util/CFRLogger.h

@@ -46,11 +46,12 @@ namespace cfrimpl {
  */
 class CFRLogger final
   : public cppmicroservices::logservice::LogService
+  , public detail::MultiThreaded<>
   , public cppmicroservices::ServiceTrackerCustomizer<
       cppmicroservices::logservice::LogService>
 {
 public:
-  explicit CFRLogger(cppmicroservices::BundleContext context);
+  CFRLogger();
   CFRLogger(const CFRLogger&) = delete;
   CFRLogger(CFRLogger&&) = delete;
   CFRLogger& operator=(const CFRLogger&) = delete;
@@ -83,6 +84,10 @@ class CFRLogger final
     const ServiceReference<cppmicroservices::logservice::LogService>& reference,
     const std::shared_ptr<cppmicroservices::logservice::LogService>& service)
     override;
+
+  // methods for the CFRLogger class
+  void Open();
+  void Close();
 
 private:
   cppmicroservices::BundleContext cfrContext;