Bug 1350637 - Part 11: Fix a race between parent/child process; r=asuth
authorJan Varga <jan.varga@gmail.com>
Tue, 08 Aug 2017 23:02:57 +0200
changeset 373420 042120b49ceb
parent 373419 64f5a7a51c21
child 373421 b81d58941a44
push id93520
push userjvarga@mozilla.com
push dateTue, 08 Aug 2017 21:35:35 +0000
treeherdermozilla-inbound@042120b49ceb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1350637
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1350637 - Part 11: Fix a race between parent/child process; r=asuth 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,