Bug 1350637 - Part 11: Fix a race between parent/child process. r=asuth, a=lizzard
authorJan Varga <jan.varga@gmail.com>
Tue, 08 Aug 2017 23:02:57 +0200
changeset 421304 6a2167880016
parent 421303 a8582d18b511
child 421305 86e3368b6058
push id7647
push userryanvm@gmail.com
push dateMon, 21 Aug 2017 19:15:39 +0000
treeherdermozilla-beta@6a2167880016 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth, lizzard
bugs1350637
milestone56.0
Bug 1350637 - Part 11: Fix a race between parent/child process. r=asuth, a=lizzard When a storage child actor gets the xpcom-shutdown notification, it releases the actor singleton which asynchronously sends __delete__ to the parent and destroys the child actor immediatelly. However, at the same time parent can be sending a message to the child which results in a routing error since the child actor doesn't exist anymore. The routing error causes a crash. This patch prevents the race by doing the teardown in two steps. We send a message to the parent DeleteMe and parent then sends __delete__ to the child.
dom/storage/PBackgroundStorage.ipdl
dom/storage/StorageIPC.cpp
dom/storage/StorageIPC.h
--- a/dom/storage/PBackgroundStorage.ipdl
+++ b/dom/storage/PBackgroundStorage.ipdl
@@ -17,17 +17,17 @@ namespace dom {
 /* This protocol bridges async access to the database thread running on the
  * parent process and caches running on the child process.
  */
 sync protocol PBackgroundStorage
 {
   manager PBackground;
 
 parent:
-  async __delete__();
+  async DeleteMe();
 
   sync Preload(nsCString originSuffix,
                nsCString originNoSuffix,
                uint32_t alreadyLoadedCount)
     returns (nsString[] keys, nsString[] values, nsresult rv);
 
   async AsyncPreload(nsCString originSuffix, nsCString originNoSuffix,
                      bool priority);
@@ -47,16 +47,18 @@ parent:
   // exist on a separate, privileged interface, but PBackgroundStorage is
   // already insecure.
   async Startup();
   async ClearAll();
   async ClearMatchingOrigin(nsCString originNoSuffix);
   async ClearMatchingOriginAttributes(OriginAttributesPattern pattern);
 
 child:
+  async __delete__();
+
   async Observe(nsCString topic,
                 nsString originAttributesPattern,
                 nsCString originScope);
   async OriginsHavingData(nsCString[] origins);
   async LoadItem(nsCString originSuffix, nsCString originNoSuffix, nsString key,
                  nsString value);
   async LoadDone(nsCString originSuffix, nsCString originNoSuffix, nsresult rv);
   async LoadUsage(nsCString scope, int64_t usage);
--- a/dom/storage/StorageIPC.cpp
+++ b/dom/storage/StorageIPC.cpp
@@ -8,16 +8,17 @@
 
 #include "LocalStorageManager.h"
 
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/ipc/BackgroundChild.h"
 #include "mozilla/ipc/BackgroundParent.h"
 #include "mozilla/ipc/PBackgroundChild.h"
+#include "mozilla/ipc/PBackgroundParent.h"
 #include "mozilla/Unused.h"
 #include "nsIDiskSpaceWatcher.h"
 #include "nsThreadUtils.h"
 
 namespace mozilla {
 namespace dom {
 
 namespace {
@@ -47,35 +48,16 @@ public:
 
 private:
   ~ShutdownObserver()
   {
     MOZ_ASSERT(NS_IsMainThread());
   }
 };
 
-NS_IMPL_ADDREF(StorageDBChild)
-
-NS_IMETHODIMP_(MozExternalRefCountType) StorageDBChild::Release(void)
-{
-  NS_PRECONDITION(0 != mRefCnt, "dup release");
-  nsrefcnt count = --mRefCnt;
-  NS_LOG_RELEASE(this, count, "StorageDBChild");
-  if (count == 1 && mIPCOpen) {
-    Send__delete__(this);
-    return 0;
-  }
-  if (count == 0) {
-    mRefCnt = 1;
-    delete this;
-    return 0;
-  }
-  return count;
-}
-
 void
 StorageDBChild::AddIPDLReference()
 {
   MOZ_ASSERT(!mIPCOpen, "Attempting to retain multiple IPDL references");
   mIPCOpen = true;
   AddRef();
 }
 
@@ -400,16 +382,18 @@ ShutdownObserver::Observe(nsISupports* a
     return NS_ERROR_FAILURE;
   }
 
   Unused << observerService->RemoveObserver(this, "xpcom-shutdown");
 
   if (sStorageChild) {
     sStorageChildDown = true;
 
+    MOZ_ALWAYS_TRUE(sStorageChild->PBackgroundStorageChild::SendDeleteMe());
+
     NS_RELEASE(sStorageChild);
     sStorageChild = nullptr;
   }
 
   return NS_OK;
 }
 
 // ----------------------------------------------------------------------------
@@ -597,16 +581,28 @@ StorageDBParent::NewCache(const nsACStri
 
 void
 StorageDBParent::ActorDestroy(ActorDestroyReason aWhy)
 {
   // Implement me! Bug 1005169
 }
 
 mozilla::ipc::IPCResult
+StorageDBParent::RecvDeleteMe()
+{
+  AssertIsOnBackgroundThread();
+
+  IProtocol* mgr = Manager();
+  if (!PBackgroundStorageParent::Send__delete__(this)) {
+    return IPC_FAIL_NO_REASON(mgr);
+  }
+  return IPC_OK();
+}
+
+mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncPreload(const nsCString& aOriginSuffix,
                                   const nsCString& aOriginNoSuffix,
                                   const bool& aPriority)
 {
   StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
--- a/dom/storage/StorageIPC.h
+++ b/dom/storage/StorageIPC.h
@@ -39,18 +39,17 @@ public:
   explicit StorageDBChild(LocalStorageManager* aManager);
 
   static StorageDBChild*
   Get();
 
   static StorageDBChild*
   GetOrCreate();
 
-  NS_IMETHOD_(MozExternalRefCountType) AddRef(void);
-  NS_IMETHOD_(MozExternalRefCountType) Release(void);
+  NS_INLINE_DECL_REFCOUNTING(StorageDBChild);
 
   void AddIPDLReference();
   void ReleaseIPDLReference();
 
   virtual nsresult Init();
   virtual nsresult Shutdown();
 
   virtual void AsyncPreload(LocalStorageCacheBridge* aCache,
@@ -106,19 +105,16 @@ private:
                                        const nsresult& aRv);
   mozilla::ipc::IPCResult RecvOriginsHavingData(nsTArray<nsCString>&& aOrigins);
   mozilla::ipc::IPCResult RecvLoadUsage(const nsCString& aOriginNoSuffix,
                                         const int64_t& aUsage);
   mozilla::ipc::IPCResult RecvError(const nsresult& aRv);
 
   nsTHashtable<nsCStringHashKey>& OriginsHavingData();
 
-  ThreadSafeAutoRefCnt mRefCnt;
-  NS_DECL_OWNINGTHREAD
-
   // Held to get caches to forward answers to.
   RefPtr<LocalStorageManager> mManager;
 
   // Origins having data hash, for optimization purposes only
   nsAutoPtr<nsTHashtable<nsCStringHashKey>> mOriginsHavingData;
 
   // List of caches waiting for preload.  This ensures the contract that
   // AsyncPreload call references the cache for time of the preload.
@@ -224,16 +220,18 @@ public:
     nsCOMPtr<nsISerialEventTarget> mOwningEventTarget;
     RefPtr<StorageDBParent> mParent;
     nsCString mOriginScope;
   };
 
 private:
   // IPC
   virtual void ActorDestroy(ActorDestroyReason aWhy) override;
+  mozilla::ipc::IPCResult RecvDeleteMe() override;
+
   mozilla::ipc::IPCResult RecvAsyncPreload(const nsCString& aOriginSuffix,
                                            const nsCString& aOriginNoSuffix,
                                            const bool& aPriority) override;
   mozilla::ipc::IPCResult RecvPreload(const nsCString& aOriginSuffix,
                                       const nsCString& aOriginNoSuffix,
                                       const uint32_t& aAlreadyLoadedCount,
                                       InfallibleTArray<nsString>* aKeys,
                                       InfallibleTArray<nsString>* aValues,