more review comments draft default tip lsng-comments-1
authorAndrew Sutherland <asutherland@asutherland.org>
Sun, 18 Nov 2018 13:54:31 -0500
changeset 481734 64a7417dc109fb913b0c4ac76b4a857be336cc7d
parent 481733 093f401c66911a6f517e315f3d89dfd4998b0a24
push id10
push userbugmail@asutherland.org
push dateSun, 18 Nov 2018 18:57:42 +0000
milestone65.0a1
more review comments
dom/localstorage/ActorsChild.h
dom/localstorage/LSObject.h
dom/localstorage/LSObserver.h
dom/localstorage/PBackgroundLSDatabase.ipdl
dom/localstorage/PBackgroundLSObserver.ipdl
dom/localstorage/PBackgroundLSRequest.ipdl
dom/localstorage/PBackgroundLSSimpleRequest.ipdl
--- a/dom/localstorage/ActorsChild.h
+++ b/dom/localstorage/ActorsChild.h
@@ -26,16 +26,34 @@ namespace dom {
 class LocalStorageManager2;
 class LSDatabase;
 class LSObject;
 class LSObserver;
 class LSRequestChildCallback;
 class LSSimpleRequestChildCallback;
 class LSSnapshot;
 
+/**
+ * Minimal glue actor with standard IPC-managed new/delete existence that exists
+ * primarily to track the continued existence of the LSDatabase in the child.
+ * Most of the interesting bits happen via PBackgroundLSSnapshot.
+ *
+ * Mutual raw pointers are maintained between LSDatabase and this class that are
+ * cleared at either (expected) when the child starts the deletion process
+ * (SendDeleteMeInternal) or unexpected actor death (ActorDestroy).
+ *
+ * See `PBackgroundLSDatabase.ipdl` for more information.
+ *
+ *
+ * ## Low-Level Lifecycle ##
+ * - Created by LSObject::EnsureDatabase if it had to create a database.
+ * - Deletion begun by LSDatabase's destructor invoking SendDeleteMeInternal
+ *   which will result in the parent sending __delete__ which destroys the
+ *   actor.
+ */
 class LSDatabaseChild final
   : public PBackgroundLSDatabaseChild
 {
   friend class mozilla::ipc::BackgroundChildImpl;
   friend class LSDatabase;
   friend class LSObject;
 
   LSDatabase* mDatabase;
@@ -73,16 +91,24 @@ private:
                                   const int64_t& aMinSize,
                                   LSSnapshotInitInfo* aInitInfo) override;
 
   bool
   DeallocPBackgroundLSSnapshotChild(PBackgroundLSSnapshotChild* aActor)
                                     override;
 };
 
+/**
+ * Minimal IPC-managed (new/delete) actor that exists to receive and relay
+ * "storage" events from changes to LocalStorage that take place in other
+ * processes as their Snapshots are checkpointed to the canonical Datastore in
+ * the parent process.
+ *
+ * See `PBackgroundLSObserver.ipdl` for more info.
+ */
 class LSObserverChild final
   : public PBackgroundLSObserverChild
 {
   friend class mozilla::ipc::BackgroundChildImpl;
   friend class LSObserver;
   friend class LSObject;
 
   LSObserver* mObserver;
@@ -114,16 +140,27 @@ private:
   RecvObserve(const PrincipalInfo& aPrinciplaInfo,
               const uint32_t& aPrivateBrowsingId,
               const nsString& aDocumentURI,
               const nsString& aKey,
               const nsString& aOldValue,
               const nsString& aNewValue) override;
 };
 
+/**
+ * Minimal glue IPC-managed (new/delete) actor that is used by LSObject and its
+ * RequestHelper to perform synchronous requests on top of an asynchronous
+ * protocol.
+ *
+ * Takes an `LSReuestChildCallback` to be invoked when a response is received
+ * via __delete__.
+ *
+ * See `PBackgroundLSRequest.ipdl`, `LSObject`, and `RequestHelper` for more
+ * info.
+ */
 class LSRequestChild final
   : public PBackgroundLSRequestChild
 {
   friend class LSObject;
   friend class LocalStorageManager2;
 
   RefPtr<LSRequestChildCallback> mCallback;
 
@@ -167,16 +204,25 @@ public:
   virtual void
   OnResponse(const LSRequestResponse& aResponse) = 0;
 
 protected:
   virtual ~LSRequestChildCallback()
   { }
 };
 
+/**
+ * Minimal glue IPC-managed (new/delete) actor used by `LocalStorageManager2` to
+ * issue asynchronous requests in an asynchronous fashion.
+ *
+ * Takes an `LSSimpleRequestChildCallback` to be invoked when a response is
+ * received via __delete__.
+ *
+ * See `PBackgroundLSSimpleRequest.ipdl` for more info.
+ */
 class LSSimpleRequestChild final
   : public PBackgroundLSSimpleRequestChild
 {
   friend class LocalStorageManager2;
 
   RefPtr<LSSimpleRequestChildCallback> mCallback;
 
   NS_DECL_OWNINGTHREAD
@@ -211,16 +257,26 @@ public:
   virtual void
   OnResponse(const LSSimpleRequestResponse& aResponse) = 0;
 
 protected:
   virtual ~LSSimpleRequestChildCallback()
   { }
 };
 
+/**
+ * Minimal IPC-managed (new/delete) actor that lasts as long as its owning
+ * LSSnapshot.
+ *
+ * Mutual raw pointers are maintained between LSSnapshot and this class that are
+ * cleared at either (expected) when the child starts the deletion process
+ * (SendDeleteMeInternal) or unexpected actor death (ActorDestroy).
+ *
+ * See `PBackgroundLSSnapshot.ipdl` and `LSSnapshot` for more info.
+ */
 class LSSnapshotChild final
   : public PBackgroundLSSnapshotChild
 {
   friend class LSDatabase;
   friend class LSSnapshot;
 
   LSSnapshot* mSnapshot;
 
--- a/dom/localstorage/LSObject.h
+++ b/dom/localstorage/LSObject.h
@@ -217,22 +217,40 @@ private:
                          LSRequestResponse& aResponse);
 
   nsresult
   EnsureDatabase();
 
   void
   DropDatabase();
 
+  /**
+   * Invoked by nsGlobalWindowInner whenever a new "storage" event listener is
+   * added to the window in order to ensure that "storage" events are received
+   * from other processes.  (`LSObject::OnChange` directly invokes
+   * `Storage::NotifyChange` to notify in-process listeners.)
+   *
+   * If this is the first request in the process for an observer for this
+   * origin, this will trigger a RequestHelper-mediated synchronous LSRequest
+   * to register the.
+   */
   nsresult
   EnsureObserver();
 
+  /**
+   * Invoked by nsGlobalWindowInner whenever its last "storage" event listener
+   * is removed.
+   */
   void
   DropObserver();
 
+  /**
+   * Internal helper method used by mutation methods that wraps the call to
+   * Storage::NotifyChange to generate same-process "storage" events.
+   */
   void
   OnChange(const nsAString& aKey,
            const nsAString& aOldValue,
            const nsAString& aNewValue);
 
   nsresult
   EndExplicitSnapshotInternal();
 
--- a/dom/localstorage/LSObserver.h
+++ b/dom/localstorage/LSObserver.h
@@ -7,16 +7,34 @@
 #ifndef mozilla_dom_localstorage_LSObserver_h
 #define mozilla_dom_localstorage_LSObserver_h
 
 namespace mozilla {
 namespace dom {
 
 class LSObserverChild;
 
+/**
+ * Effectively just a refcounted life-cycle management wrapper around
+ * LSObserverChild which exists to receive "storage" event information from
+ * other processes.  (Same-process events are handled within the process, see
+ * `LSObject::OnChange`.)
+ *
+ * ## Lifecycle ##
+ * - Created by LSObject::EnsureObserver via synchronous LSRequest idiom
+ *   whenever the first window's origin adds a "storage" event.  Placed in the
+ *   gLSObservers LSObserverHashtable for subsequent LSObject's via
+ *   LSObserver::Get lookup.
+ * - The LSObserverChild directly handles "Observe" messages, shunting them
+ *   directly to Storage::NotifyChange which does all the legwork of notifying
+ *   windows about "storage" events.
+ * - Destroyed when refcount goes to zero due to all owning LSObjects being
+ *   destroyed or having their `LSObject::DropObserver` methods invoked due to
+ *   the last "storage" event listener being removed from the owning window.
+ */
 class LSObserver final
 {
   friend class LSObject;
 
   LSObserverChild* mActor;
 
   const nsCString mOrigin;
 
--- a/dom/localstorage/PBackgroundLSDatabase.ipdl
+++ b/dom/localstorage/PBackgroundLSDatabase.ipdl
@@ -115,16 +115,19 @@ parent:
    */
   sync PBackgroundLSSnapshot(nsString documentURI,
                              bool increasePeakUsage,
                              int64_t requestedSize,
                              int64_t minSize)
     returns (LSSnapshotInitInfo initInfo);
 
 child:
+  /**
+   * Only sent by the parent in response to the child's DeleteMe request.
+   */
   async __delete__();
 
   /**
    * Request to close the LSDatabase, checkpointing and finishing any
    * outstanding snapshots so no state is lost.  This request is issued when
    * QuotaManager is shutting down or is aborting operations for an origin or
    * process.  Once the snapshot has cleaned up, AllowToClose will be sent to
    * the parent.
--- a/dom/localstorage/PBackgroundLSObserver.ipdl
+++ b/dom/localstorage/PBackgroundLSObserver.ipdl
@@ -4,26 +4,52 @@
 
 include protocol PBackground;
 
 include PBackgroundSharedTypes;
 
 namespace mozilla {
 namespace dom {
 
+/**
+ * The observer protocol sends "storage" event notifications for changes to
+ * LocalStorage that take place in other processes as their Snapshots are
+ * Checkpointed to the canonical Datastore in the parent process.  Same-process
+ * notifications are generated as mutations happen.
+ *
+ * Note that mutations are never generated for redundant mutations.  Setting the
+ * key "foo" to have value "bar" when it already has value "bar" will never
+ * result in a "storage" event.
+ */
 async protocol PBackgroundLSObserver
 {
   manager PBackground;
 
 parent:
+  /**
+   * Sent by the LSObserver's destructor when it's going away.  Any Observe
+   * messages received after this is sent will be ignored.  Which is fine,
+   * because there should be nothing around left to hear.  In the event a new
+   * page came into existence, its Observer creation will happen (effectively)
+   * synchronously.
+   */
   async DeleteMe();
 
 child:
+  /**
+   * Only sent by the parent in response to a deletion request.
+   */
   async __delete__();
 
+  /**
+   * Sent by the parent process as Snapshots from other processes are
+   * Checkpointed, applying their mutations.  The child actor currently directly
+   * shunts these to Storage::NotifyChange to generate "storage" events for
+   * immediate dispatch.
+   */
   async Observe(PrincipalInfo principalInfo,
                 uint32_t privateBrowsingId,
                 nsString documentURI,
                 nsString key,
                 nsString oldValue,
                 nsString newValue);
 };
 
--- a/dom/localstorage/PBackgroundLSRequest.ipdl
+++ b/dom/localstorage/PBackgroundLSRequest.ipdl
@@ -33,22 +33,19 @@ union LSRequestResponse
   LSRequestPrepareObserverResponse;
 };
 
 /**
  * An asynchronous protocol for issuing requests that are used in a synchronous
  * fashion by LocalStorage via LSObject's RequestHelper mechanism.  This differs
  * from LSSimpleRequest which is implemented and used asynchronously.
  *
- *
- * The life-cycle of the request looks like so:
- * - Child sends the constructor.
- * -
- *
- *
+ * See `PBackgroundLSSharedTypes.ipdlh` for more on the request types, the
+ * response types above for their corresponding responses, and `RequestHelper`
+ * for more on the usage and lifecycle of this mechanism.
  */
 protocol PBackgroundLSRequest
 {
   manager PBackground;
 
 parent:
   // The Cancel message is used to avoid a possible dead lock caused by a CPOW
   // sending a synchronous message from the main thread in the chrome process
--- a/dom/localstorage/PBackgroundLSSimpleRequest.ipdl
+++ b/dom/localstorage/PBackgroundLSSimpleRequest.ipdl
@@ -12,17 +12,18 @@ namespace dom {
  * origin was preloaded.
  */
 struct LSSimpleRequestPreloadedResponse
 {
   bool preloaded;
 };
 
 /**
- * Discriminated union where `nsresult` conveys an error code
+ * Discriminated union where `nsresult` conveys an error code or the simple
+ * request result.
  */
 union LSSimpleRequestResponse
 {
   nsresult;
   LSSimpleRequestPreloadedResponse;
 };
 
 /**