Bug 1286798 - Review fix 1 - Do not tunnel event target through IPC. r=janv draft
authorAndrew Sutherland <asutherland@asutherland.org>
Mon, 05 Nov 2018 14:04:39 -0500
changeset 481731 79b6f70b08cb98fc9524136abe0346b4598865f0
parent 481730 0b53500e43327a1f6e929652aa3fed7589be2063
child 481732 0f321e587e0c6bcbbc51e5d5e02934eb19ce767b
push id10
push userbugmail@asutherland.org
push dateSun, 18 Nov 2018 18:57:42 +0000
reviewersjanv
bugs1286798
milestone65.0a1
Bug 1286798 - Review fix 1 - Do not tunnel event target through IPC. r=janv
dom/localstorage/ActorsParent.cpp
dom/localstorage/ActorsParent.h
dom/localstorage/LSObject.cpp
dom/localstorage/LSObject.h
dom/localstorage/LocalStorageManager2.cpp
dom/storage/StorageNotifierService.h
dom/webidl/Storage.webidl
ipc/glue/BackgroundChildImpl.cpp
ipc/glue/BackgroundChildImpl.h
ipc/glue/BackgroundParentImpl.cpp
ipc/glue/BackgroundParentImpl.h
ipc/glue/PBackground.ipdl
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ActorsParent.h"
 
 #include "LocalStorageCommon.h"
+#include "LSObject.h"
 #include "mozIStorageConnection.h"
 #include "mozIStorageFunction.h"
 #include "mozIStorageService.h"
 #include "mozStorageCID.h"
 #include "mozStorageHelper.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/Unused.h"
 #include "mozilla/dom/ContentParent.h"
@@ -3190,33 +3191,31 @@ DeallocPBackgroundLSObserverParent(PBack
   // Transfer ownership back from IPDL.
   RefPtr<Observer> actor = dont_AddRef(static_cast<Observer*>(aActor));
 
   return true;
 }
 
 PBackgroundLSRequestParent*
 AllocPBackgroundLSRequestParent(PBackgroundParent* aBackgroundActor,
-                                const uintptr_t& aMainEventTarget,
                                 const LSRequestParams& aParams)
 {
   AssertIsOnBackgroundThread();
 
   if (NS_WARN_IF(QuotaClient::IsShuttingDownOnBackgroundThread())) {
     return nullptr;
   }
 
-  if (NS_WARN_IF(BackgroundParent::IsOtherProcessActor(aBackgroundActor) &&
-                 aMainEventTarget)) {
-    ASSERT_UNLESS_FUZZING();
-    return nullptr;
-  }
-
-  nsCOMPtr<nsIEventTarget> mainEventTarget =
-    reinterpret_cast<nsIEventTarget*>(aMainEventTarget);
+  // If we're in the same process as the actor, we directly get the target event
+  // queue from the current RequestHelper.
+  nsCOMPtr<nsIEventTarget> mainEventTarget;
+  if (!BackgroundParent::IsOtherProcessActor(aBackgroundActor)) {
+    mainEventTarget = LSObject::GetSyncLoopEventTarget();
+  }
+
 
   RefPtr<LSRequestBase> actor;
 
   switch (aParams.type()) {
     case LSRequestParams::TLSRequestPrepareDatastoreParams: {
       RefPtr<ContentParent> contentParent =
         BackgroundParent::GetContentParent(aBackgroundActor);
 
@@ -3249,17 +3248,16 @@ AllocPBackgroundLSRequestParent(PBackgro
   }
 
   // Transfer ownership to IPDL.
   return actor.forget().take();
 }
 
 bool
 RecvPBackgroundLSRequestConstructor(PBackgroundLSRequestParent* aActor,
-                                    const uintptr_t& aMainEventTarget,
                                     const LSRequestParams& aParams)
 {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
   MOZ_ASSERT(aParams.type() != LSRequestParams::T__None);
   MOZ_ASSERT(!QuotaClient::IsShuttingDownOnBackgroundThread());
 
   // The actor is now completely built.
--- a/dom/localstorage/ActorsParent.h
+++ b/dom/localstorage/ActorsParent.h
@@ -55,22 +55,20 @@ RecvPBackgroundLSObserverConstructor(PBa
                                      const uint64_t& aObservereId);
 
 bool
 DeallocPBackgroundLSObserverParent(PBackgroundLSObserverParent* aActor);
 
 PBackgroundLSRequestParent*
 AllocPBackgroundLSRequestParent(
                               mozilla::ipc::PBackgroundParent* aBackgroundActor,
-                              const uintptr_t& aMainEventTarget,
                               const LSRequestParams& aParams);
 
 bool
 RecvPBackgroundLSRequestConstructor(PBackgroundLSRequestParent* aActor,
-                                    const uintptr_t& aMainEventTarget,
                                     const LSRequestParams& aParams);
 
 bool
 DeallocPBackgroundLSRequestParent(PBackgroundLSRequestParent* aActor);
 
 PBackgroundLSSimpleRequestParent*
 AllocPBackgroundLSSimpleRequestParent(const LSSimpleRequestParams& aParams);
 
--- a/dom/localstorage/LSObject.cpp
+++ b/dom/localstorage/LSObject.cpp
@@ -78,16 +78,26 @@ public:
 
   void
   AssertIsOnOwningThread() const
   {
     MOZ_ASSERT(NS_IsMainThread());
     MOZ_ASSERT(IsOnOwningThread());
   }
 
+  // Used for requests from the parent process to the parent process; in that
+  // case we want ActorsParent to know our event-target and this is better than
+  // trying to tunnel the pointer through IPC.
+  nsCOMPtr<nsIEventTarget>
+  GetSyncLoopEventTarget() const
+  {
+    MOZ_ASSERT(XRE_IsParentProcess());
+    return mNestedEventTarget;
+  }
+
   nsresult
   StartAndReturnResponse(LSRequestResponse& aResponse);
 
   nsresult
   CancelOnAnyThread();
 
 private:
   ~RequestHelper()
@@ -257,16 +267,35 @@ LSObject::CreateForPrincipal(nsPIDOMWind
   object->mOrigin = origin;
   object->mDocumentURI = aDocumentURI;
 
   object.forget(aObject);
   return NS_OK;
 }
 
 // static
+already_AddRefed<nsIEventTarget>
+LSObject::GetSyncLoopEventTarget()
+{
+  RefPtr<RequestHelper> helper;
+
+  {
+    StaticMutexAutoLock lock(gRequestHelperMutex);
+    helper = gRequestHelper;
+  }
+
+  nsCOMPtr<nsIEventTarget> target;
+  if (helper) {
+    target = helper->GetSyncLoopEventTarget();
+  }
+
+  return target.forget();
+}
+
+// static
 void
 LSObject::CancelSyncLoop()
 {
   RefPtr<RequestHelper> helper;
 
   {
     StaticMutexAutoLock lock(gRequestHelperMutex);
     helper = gRequestHelper;
@@ -286,26 +315,17 @@ LSObject::StartRequest(nsIEventTarget* a
 
   PBackgroundChild* backgroundActor =
     BackgroundChild::GetOrCreateForCurrentThread(aMainEventTarget);
   if (NS_WARN_IF(!backgroundActor)) {
     return nullptr;
   }
 
   LSRequestChild* actor = new LSRequestChild(aCallback);
-
-  uintptr_t mainEventTarget;
-  if (XRE_IsParentProcess()) {
-    mainEventTarget = reinterpret_cast<uintptr_t>(aMainEventTarget);
-  } else {
-    mainEventTarget = 0;
-  }
-
   backgroundActor->SendPBackgroundLSRequestConstructor(actor,
-                                                       mainEventTarget,
                                                        aParams);
 
   return actor;
 }
 
 Storage::StorageType
 LSObject::Type() const
 {
--- a/dom/localstorage/LSObject.h
+++ b/dom/localstorage/LSObject.h
@@ -58,16 +58,24 @@ public:
 
   static nsresult
   CreateForPrincipal(nsPIDOMWindowInner* aWindow,
                      nsIPrincipal* aPrincipal,
                      const nsAString& aDocumentURI,
                      bool aPrivate,
                      LSObject** aObject);
 
+  /**
+   * Used for requests from the parent process to the parent process; in that
+   * case we want ActorsParent to know our event-target and this is better than
+   * trying to tunnel the pointer through IPC.
+   */
+  static already_AddRefed<nsIEventTarget>
+  GetSyncLoopEventTarget();
+
   static void
   CancelSyncLoop();
 
   void
   AssertIsOnOwningThread() const
   {
     NS_ASSERT_OWNINGTHREAD(LSObject);
   }
@@ -138,16 +146,18 @@ public:
   void
   BeginExplicitSnapshot(nsIPrincipal& aSubjectPrincipal,
                         ErrorResult& aError) override;
 
   void
   EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal,
                       ErrorResult& aError) override;
 
+  //////////////////////////////////////////////////////////////////////////////
+
   NS_DECL_ISUPPORTS_INHERITED
   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(LSObject, Storage)
 
 private:
   LSObject(nsPIDOMWindowInner* aWindow,
            nsIPrincipal* aPrincipal);
 
   ~LSObject();
--- a/dom/localstorage/LocalStorageManager2.cpp
+++ b/dom/localstorage/LocalStorageManager2.cpp
@@ -290,17 +290,16 @@ LocalStorageManager2::StartRequest(Promi
     return NS_ERROR_FAILURE;
   }
 
   RefPtr<RequestResolver> resolver = new RequestResolver(aPromise);
 
   auto actor = new LSRequestChild(resolver);
 
   if (!backgroundActor->SendPBackgroundLSRequestConstructor(actor,
-                                                            0,
                                                             aParams)) {
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
 }
 
 nsresult
--- a/dom/storage/StorageNotifierService.h
+++ b/dom/storage/StorageNotifierService.h
@@ -9,16 +9,23 @@
 
 class nsPIDOMWindowInner;
 
 namespace mozilla {
 namespace dom {
 
 class StorageEvent;
 
+/**
+ * Enables the StorageNotifierService to check whether an observer is interested
+ * in receiving events for the given principal before calling the method, an
+ * optimization refactoring.
+ *
+ * Expected to only be implemented by nsGlobalWindowObserver or its succesor.
+ */
 class StorageNotificationObserver
 {
 public:
   NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING
 
   virtual void
   ObserveStorageNotification(StorageEvent* aEvent,
                              const char16_t* aStorageType,
@@ -29,16 +36,25 @@ public:
 
   virtual nsIPrincipal*
   GetPrincipal() const = 0;
 
   virtual nsIEventTarget*
   GetEventTarget() const = 0;
 };
 
+/**
+ * A specialized version of the observer service that uses the custom
+ * StorageNotificationObserver so that principal checks can happen in this class
+ * rather than in the nsIObserver::observe method where they used to happen.
+ *
+ * The only expected consumers are nsGlobalWindowInner instances via their
+ * nsGlobalWindowObserver helper that avoids being able to use the window as an
+ * nsIObserver.
+ */
 class StorageNotifierService final
 {
 public:
   NS_INLINE_DECL_REFCOUNTING(StorageNotifierService)
 
   static StorageNotifierService*
   GetOrCreate();
 
--- a/dom/webidl/Storage.webidl
+++ b/dom/webidl/Storage.webidl
@@ -29,21 +29,34 @@ interface Storage {
 
   [Throws, NeedsSubjectPrincipal]
   void clear();
 
   [ChromeOnly]
   readonly attribute boolean isSessionOnly;
 };
 
-// Testing only.
+/**
+ * Testing methods that exist only for the benefit of automated glass-box
+ * testing.  Will never be exposed to content at large and unlikely to be useful
+ * in a WebDriver context.
+ */
 partial interface Storage {
+  /**
+   * Does a security-check and ensures the underlying database has been opened
+   * without actually calling any database methods.  (Read-only methods will
+   * have a similar effect but also impact the state of the snapshot.)
+   */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void open();
 
+  /**
+   * Automatically ends any explicit snapshot and drops the reference to the
+   * underlying database, but does not otherwise perturb the database.
+   */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void close();
 
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void beginExplicitSnapshot();
 
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void endExplicitSnapshot();
--- a/ipc/glue/BackgroundChildImpl.cpp
+++ b/ipc/glue/BackgroundChildImpl.cpp
@@ -275,17 +275,16 @@ BackgroundChildImpl::DeallocPBackgroundL
   MOZ_ASSERT(aActor);
 
   delete aActor;
   return true;
 }
 
 BackgroundChildImpl::PBackgroundLSRequestChild*
 BackgroundChildImpl::AllocPBackgroundLSRequestChild(
-                                              const uintptr_t& aMainEventTarget,
                                               const LSRequestParams& aParams)
 {
   MOZ_CRASH("PBackgroundLSRequestChild actor should be manually constructed!");
 }
 
 bool
 BackgroundChildImpl::DeallocPBackgroundLSRequestChild(
                                               PBackgroundLSRequestChild* aActor)
--- a/ipc/glue/BackgroundChildImpl.h
+++ b/ipc/glue/BackgroundChildImpl.h
@@ -90,18 +90,17 @@ protected:
   virtual PBackgroundLSObserverChild*
   AllocPBackgroundLSObserverChild(const uint64_t& aObserverId) override;
 
   virtual bool
   DeallocPBackgroundLSObserverChild(PBackgroundLSObserverChild* aActor)
                                     override;
 
   virtual PBackgroundLSRequestChild*
-  AllocPBackgroundLSRequestChild(const uintptr_t& aMainEventTarget,
-                                 const LSRequestParams& aParams) override;
+  AllocPBackgroundLSRequestChild(const LSRequestParams& aParams) override;
 
   virtual bool
   DeallocPBackgroundLSRequestChild(PBackgroundLSRequestChild* aActor) override;
 
   virtual PBackgroundLSSimpleRequestChild*
   AllocPBackgroundLSSimpleRequestChild(const LSSimpleRequestParams& aParams)
                                        override;
 
--- a/ipc/glue/BackgroundParentImpl.cpp
+++ b/ipc/glue/BackgroundParentImpl.cpp
@@ -370,39 +370,35 @@ BackgroundParentImpl::DeallocPBackground
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
 
   return mozilla::dom::DeallocPBackgroundLSObserverParent(aActor);
 }
 
 BackgroundParentImpl::PBackgroundLSRequestParent*
 BackgroundParentImpl::AllocPBackgroundLSRequestParent(
-                                              const uintptr_t& aMainEventTarget,
                                               const LSRequestParams& aParams)
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
 
   return mozilla::dom::AllocPBackgroundLSRequestParent(this,
-                                                       aMainEventTarget,
                                                        aParams);
 }
 
 mozilla::ipc::IPCResult
 BackgroundParentImpl::RecvPBackgroundLSRequestConstructor(
                                              PBackgroundLSRequestParent* aActor,
-                                             const uintptr_t& aMainEventTarget,
                                              const LSRequestParams& aParams)
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
 
   if (!mozilla::dom::RecvPBackgroundLSRequestConstructor(aActor,
-                                                         aMainEventTarget,
                                                          aParams)) {
     return IPC_FAIL_NO_REASON(this);
   }
   return IPC_OK();
 }
 
 bool
 BackgroundParentImpl::DeallocPBackgroundLSRequestParent(
--- a/ipc/glue/BackgroundParentImpl.h
+++ b/ipc/glue/BackgroundParentImpl.h
@@ -99,22 +99,20 @@ protected:
   RecvPBackgroundLSObserverConstructor(PBackgroundLSObserverParent* aActor,
                                        const uint64_t& aObserverId) override;
 
   virtual bool
   DeallocPBackgroundLSObserverParent(PBackgroundLSObserverParent* aActor)
                                      override;
 
   virtual PBackgroundLSRequestParent*
-  AllocPBackgroundLSRequestParent(const uintptr_t& aMainEventTarget,
-                                  const LSRequestParams& aParams) override;
+  AllocPBackgroundLSRequestParent(const LSRequestParams& aParams) override;
 
   virtual mozilla::ipc::IPCResult
   RecvPBackgroundLSRequestConstructor(PBackgroundLSRequestParent* aActor,
-                                      const uintptr_t& aMainEventTarget,
                                       const LSRequestParams& aParams) override;
 
   virtual bool
   DeallocPBackgroundLSRequestParent(PBackgroundLSRequestParent* aActor)
                                     override;
 
   virtual PBackgroundLSSimpleRequestParent*
   AllocPBackgroundLSSimpleRequestParent(const LSSimpleRequestParams& aParams)
--- a/ipc/glue/PBackground.ipdl
+++ b/ipc/glue/PBackground.ipdl
@@ -122,18 +122,17 @@ parent:
   async PBackgroundSDBConnection(PrincipalInfo principalInfo);
 
   async PBackgroundLSDatabase(PrincipalInfo principalInfo,
                               uint32_t privateBrowsingId,
                               uint64_t datastoreId);
 
   async PBackgroundLSObserver(uint64_t observerId);
 
-  async PBackgroundLSRequest(uintptr_t mainEventTarget,
-                             LSRequestParams params);
+  async PBackgroundLSRequest(LSRequestParams params);
 
   async PBackgroundLSSimpleRequest(LSSimpleRequestParams params);
 
   async PBackgroundLocalStorageCache(PrincipalInfo principalInfo,
                                      nsCString originKey,
                                      uint32_t privateBrowsingId);
 
   async PBackgroundStorage(nsString profilePath);