Remove special destructor from Runtime::create

Summary:
The `StorageProvider` used to require being specially deleted after the `Runtime` in order
to ensure that the Runtime itself could be destructed.

Since compressed pointers doesn't require this feature anymore, delete the special logic
from the `Runtime::create` function.

Some classes relied on the `StorageProvider` always outliving the Runtime and only took
raw pointers. Have these take `shared_ptr` instead to make sure the provider dies after
everything is destructed.

Reviewed By: davedets

Differential Revision: D19705134

fbshipit-source-id: fa458a5520d2146ea6174bebf0fcf78ef0ec9556

API/hermes/hermes.cpp

@@ -190,12 +190,10 @@ class StackRuntime {
   StackRuntime(const vm::RuntimeConfig &runtimeConfig)
       : thread_(runtimeMemoryThread, this) {
     startup_.get_future().wait();
-    runtime_->emplace(
-        provider_.get(),
-        runtimeConfig.rebuild()
-            .withRegisterStack(nullptr)
-            .withMaxNumRegisters(kMaxNumRegisters)
-            .build());
+    runtime_->emplace(runtimeConfig.rebuild()
+                          .withRegisterStack(nullptr)
+                          .withMaxNumRegisters(kMaxNumRegisters)
+                          .build());
   }
 
   ~StackRuntime() {
@@ -227,7 +225,6 @@ class StackRuntime {
 
     llvm::Optional<::hermes::vm::StackRuntime> rt;
 
-    stack->provider_ = vm::StorageProvider::mmapProvider();
     stack->runtime_ = &rt;
     stack->startup_.set_value();
     stack->shutdown_.get_future().wait();
@@ -247,7 +244,6 @@ class StackRuntime {
   // * Initialize provider_ and runtime_ from that thread
   std::promise<void> startup_;
   std::promise<void> shutdown_;
-  std::unique_ptr<::hermes::vm::StorageProvider> provider_;
   llvm::Optional<::hermes::vm::StackRuntime> *runtime_{nullptr};
   std::thread thread_;
 };

include/hermes/VM/GCBase.h

@@ -455,8 +455,7 @@ class GCBase {
       GCCallbacks *gcCallbacks,
       PointerBase *pointerBase,
       const GCConfig &gcConfig,
-      std::shared_ptr<CrashManager> crashMgr,
-      StorageProvider *provider);
+      std::shared_ptr<CrashManager> crashMgr);
 
   virtual ~GCBase() {}
 

include/hermes/VM/GenGCNC.h

@@ -117,7 +117,7 @@ class GenGC final : public GCBase {
       PointerBase *pointerBase,
       const GCConfig &gcConfig,
       std::shared_ptr<CrashManager> crashMgr,
-      StorageProvider *provider);
+      std::shared_ptr<StorageProvider> provider);
 
   ~GenGC();
 

include/hermes/VM/LogFailStorageProvider.h

@@ -20,12 +20,12 @@ namespace vm {
 /// times allocation requests to its delegate have failed.  Note that this
 /// adapter does not manage the lifetime of its delegate.
 class LogFailStorageProvider final : public StorageProvider {
-  StorageProvider *delegate_;
+  std::shared_ptr<StorageProvider> delegate_;
   size_t numFailedAllocs_{0};
 
  public:
-  explicit LogFailStorageProvider(StorageProvider *provider)
-      : delegate_(provider) {}
+  explicit LogFailStorageProvider(std::shared_ptr<StorageProvider> provider)
+      : delegate_(std::move(provider)) {}
 
   inline size_t numFailedAllocs() const;
 

include/hermes/VM/MallocGC.h

@@ -154,7 +154,7 @@ class MallocGC final : public GCBase {
       PointerBase *pointerBase,
       const GCConfig &gcConfig,
       std::shared_ptr<CrashManager> crashMgr,
-      StorageProvider *provider);
+      std::shared_ptr<StorageProvider> provider);
 
   ~MallocGC();
 

include/hermes/VM/Runtime.h

@@ -805,7 +805,7 @@ class Runtime : public HandleRootOwner,
   /// NOTE: This should only be used by StackRuntime. All other uses should use
   /// Runtime::create.
   explicit Runtime(
-      StorageProvider *provider,
+      std::shared_ptr<StorageProvider> provider,
       const RuntimeConfig &runtimeConfig);
 
 /// @}
@@ -1231,7 +1231,10 @@ class Runtime : public HandleRootOwner,
 /// default creator.
 class StackRuntime final : public Runtime {
  public:
-  StackRuntime(StorageProvider *provider, const RuntimeConfig &config);
+  StackRuntime(const RuntimeConfig &config);
+  StackRuntime(
+      std::shared_ptr<StorageProvider> provider,
+      const RuntimeConfig &config);
 
   // A dummy virtual destructor to avoid problems when StackRuntime is used
   // in compilation units compiled with RTTI.

lib/VM/GCBase.cpp

@@ -38,9 +38,7 @@ GCBase::GCBase(
     GCCallbacks *gcCallbacks,
     PointerBase *pointerBase,
     const GCConfig &gcConfig,
-    std::shared_ptr<CrashManager> crashMgr,
-    // Do nothing with this in the default case, only NCGen needs this.
-    StorageProvider *)
+    std::shared_ptr<CrashManager> crashMgr)
     : metaTable_(metaTable),
       gcCallbacks_(gcCallbacks),
       pointerBase_(pointerBase),

lib/VM/Runtime.cpp

@@ -83,20 +83,8 @@ static const Predefined::Str fixedPropCacheNames[(size_t)PropCacheID::_COUNT] =
 
 /* static */
 std::shared_ptr<Runtime> Runtime::create(const RuntimeConfig &runtimeConfig) {
-  // TODO(T31421960): This can become a unique_ptr with C++14 lambda
-  // initializers.
-  std::shared_ptr<StorageProvider> provider{StorageProvider::mmapProvider()};
-  // When not using the flat address space, allocate runtime normally.
-  Runtime *rt = new Runtime(provider.get(), runtimeConfig);
-  // Return a shared pointer with a custom deleter to delete the underlying
-  // storage of the runtime.
-  return std::shared_ptr<Runtime>{rt, [provider](Runtime *runtime) {
-                                    delete runtime;
-                                    // Provider is only captured to keep it
-                                    // alive until after the Runtime is
-                                    // deleted.
-                                    (void)provider;
-                                  }};
+  return std::shared_ptr<Runtime>{
+      new Runtime(StorageProvider::mmapProvider(), runtimeConfig)};
 }
 
 CallResult<HermesValue> Runtime::getNamed(
@@ -158,7 +146,9 @@ ExecutionStatus Runtime::putNamedThrowOnError(
       .getStatus();
 }
 
-Runtime::Runtime(StorageProvider *provider, const RuntimeConfig &runtimeConfig)
+Runtime::Runtime(
+    std::shared_ptr<StorageProvider> provider,
+    const RuntimeConfig &runtimeConfig)
     // The initial heap size can't be larger than the max.
     : enableEval(runtimeConfig.getEnableEval()),
       verifyEvalIR(runtimeConfig.getVerifyEvalIR()),
@@ -168,7 +158,7 @@ Runtime::Runtime(StorageProvider *provider, const RuntimeConfig &runtimeConfig)
           this,
           runtimeConfig.getGCConfig(),
           runtimeConfig.getCrashMgr(),
-          provider),
+          std::move(provider)),
       jitContext_(runtimeConfig.getEnableJIT(), (1 << 20) * 16, (1 << 20) * 32),
       hasES6Proxy_(runtimeConfig.getES6Proxy()),
       hasES6Symbol_(runtimeConfig.getES6Symbol()),
@@ -1632,10 +1622,13 @@ void Runtime::dumpCallFrames() {
   dumpCallFrames(llvm::errs());
 }
 
+StackRuntime::StackRuntime(const RuntimeConfig &config)
+    : StackRuntime(StorageProvider::mmapProvider(), config) {}
+
 StackRuntime::StackRuntime(
-    StorageProvider *provider,
+    std::shared_ptr<StorageProvider> provider,
     const RuntimeConfig &config)
-    : Runtime(provider, config) {}
+    : Runtime(std::move(provider), config) {}
 
 StackRuntime::~StackRuntime() {}
 

lib/VM/gcs/GenGCNC.cpp

@@ -85,15 +85,14 @@ GenGC::GenGC(
     PointerBase *pointerBase,
     const GCConfig &gcConfig,
     std::shared_ptr<CrashManager> crashMgr,
-    StorageProvider *provider)
+    std::shared_ptr<StorageProvider> provider)
     : GCBase(
           metaTable,
           gcCallbacks,
           pointerBase,
           gcConfig,
-          std::move(crashMgr),
-          provider),
-      storageProvider_(provider),
+          std::move(crashMgr)),
+      storageProvider_(std::move(provider)),
       generationSizes_(Size(gcConfig)),
       // ExpectedPageSize.h defines a static value for the expected page size,
       // which we use in sizing and aligning the metadata components

lib/VM/gcs/MallocGC.cpp

@@ -174,14 +174,13 @@ MallocGC::MallocGC(
     PointerBase *pointerBase,
     const GCConfig &gcConfig,
     std::shared_ptr<CrashManager> crashMgr,
-    StorageProvider *provider)
+    std::shared_ptr<StorageProvider> provider)
     : GCBase(
           metaTable,
           gcCallbacks,
           pointerBase,
           gcConfig,
-          std::move(crashMgr),
-          provider),
+          std::move(crashMgr)),
       pointers_(),
       weakPointers_(),
       maxSize_(Size(gcConfig).max()),

unittests/VMRuntime/StorageProviderTest.cpp

@@ -150,10 +150,11 @@ TEST(StorageProviderTest, LimitedStorageProviderDeleteNull) {
 
 TEST(StorageProviderTest, LogFailStorageProvider) {
   constexpr size_t LIM = 2;
-  LimitedStorageProvider delegate{StorageProvider::mmapProvider(),
-                                  AlignedStorage::size() * LIM};
+  auto delegate =
+      std::unique_ptr<LimitedStorageProvider>{new LimitedStorageProvider{
+          StorageProvider::mmapProvider(), AlignedStorage::size() * LIM}};
 
-  LogFailStorageProvider provider{&delegate};
+  LogFailStorageProvider provider{std::move(delegate)};
 
   constexpr size_t FAILS = 3;
   void *storages[LIM];

unittests/VMRuntime/TestHelpers.cpp

@@ -30,16 +30,20 @@ DummyRuntime::DummyRuntime(
     const GCConfig &gcConfig,
     std::shared_ptr<StorageProvider> storageProvider,
     std::shared_ptr<CrashManager> crashMgr)
-    : gc{metaTable, this, this, gcConfig, crashMgr, storageProvider.get()} {}
+    : gc{metaTable,
+         this,
+         this,
+         gcConfig,
+         crashMgr,
+         std::move(storageProvider)} {}
 
 std::shared_ptr<DummyRuntime> DummyRuntime::create(
     MetadataTableForTests metaTable,
     const GCConfig &gcConfig,
     std::shared_ptr<StorageProvider> provider,
     std::shared_ptr<CrashManager> crashMgr) {
   DummyRuntime *rt = new DummyRuntime(metaTable, gcConfig, provider, crashMgr);
-  return std::shared_ptr<DummyRuntime>{
-      rt, [provider](DummyRuntime *runtime) { delete runtime; }};
+  return std::shared_ptr<DummyRuntime>{rt};
 }
 
 std::shared_ptr<DummyRuntime> DummyRuntime::create(