Review code comments. draft
authorAndrew Sutherland <asutherland@asutherland.org>
Tue, 06 Nov 2018 22:55:14 -0500
changeset 481732 0f321e587e0c6bcbbc51e5d5e02934eb19ce767b
parent 481731 79b6f70b08cb98fc9524136abe0346b4598865f0
child 481733 093f401c66911a6f517e315f3d89dfd4998b0a24
push id10
push userbugmail@asutherland.org
push dateSun, 18 Nov 2018 18:57:42 +0000
milestone65.0a1
Review code comments.
dom/interfaces/storage/nsIDOMStorageManager.idl
dom/localstorage/ActorsParent.cpp
dom/localstorage/LSDatabase.h
dom/localstorage/LSObject.cpp
dom/localstorage/LSObject.h
dom/localstorage/LSSnapshot.h
dom/localstorage/LocalStorageCommon.h
dom/localstorage/LocalStorageManager2.cpp
dom/localstorage/LocalStorageManager2.h
dom/localstorage/PBackgroundLSDatabase.ipdl
dom/localstorage/PBackgroundLSRequest.ipdl
dom/localstorage/PBackgroundLSSimpleRequest.ipdl
dom/localstorage/PBackgroundLSSnapshot.ipdl
dom/localstorage/nsILocalStorageManager.idl
dom/storage/Storage.h
dom/webidl/Storage.webidl
ipc/glue/PBackground.ipdl
--- a/dom/interfaces/storage/nsIDOMStorageManager.idl
+++ b/dom/interfaces/storage/nsIDOMStorageManager.idl
@@ -44,16 +44,21 @@ interface nsIDOMStorageManager : nsISupp
    * @param aPrivate
    *    Whether the demanding document is running in Private Browsing mode or not.
    */
   Storage createStorage(in mozIDOMWindow aWindow,
                         in nsIPrincipal aPrincipal,
                         in AString aDocumentURI,
                         [optional] in bool aPrivate);
   /**
+   * DEPRECATED.  The only good reason to use this was if you were writing a
+   * test and wanted to hackily determine if a preload happened.  That's now
+   * covered by `nsILocalStorageManager.isPreloaded` and you should use that if
+   * that's what you want.  If LSNG is in use, this will throw.
+   *
    * Returns instance of DOM storage object for given principal.
    * If there is no storage managed for the scope, then null is returned and
    * no object is created.  Otherwise, an object (new) for the existing storage
    * scope is returned.
    *
    * @param aWindow
    *    The parent window.
    * @param aPrincipal
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -925,16 +925,25 @@ DetachShadowDatabase(mozIStorageConnecti
 
   return NS_OK;
 }
 
 /*******************************************************************************
  * Non-actor class declarations
  ******************************************************************************/
 
+/**
+ * Per-Datastore coalescing manipulation queue.
+ *
+ * The old implementation had a similar but less sophisticated PendingOperations
+ * mechanism that differed in that it was:
+ * - A single update queue over all origins.
+ * - Strictly about optimizing writes.
+ *
+ */
 class WriteOptimizer final
 {
   class WriteInfo;
   class AddItemInfo;
   class UpdateItemInfo;
   class RemoveItemInfo;
   class ClearInfo;
 
@@ -1456,19 +1465,37 @@ private:
 class Datastore final
 {
   RefPtr<DirectoryLock> mDirectoryLock;
   RefPtr<Connection> mConnection;
   RefPtr<QuotaObject> mQuotaObject;
   nsCOMPtr<nsIRunnable> mCompleteCallback;
   nsTHashtable<nsPtrHashKey<PrepareDatastoreOp>> mPrepareDatastoreOps;
   nsTHashtable<nsPtrHashKey<PreparedDatastore>> mPreparedDatastores;
+  /**
+   * A database is live (and in this hashtable) if it has a live LSDatabase
+   * actor.  There is at most one Database per origin per content process.  Each
+   * Databse corresponds to an LSDatabase in its associated content process.
+   */
   nsTHashtable<nsPtrHashKey<Database>> mDatabases;
+  /**
+   * A database is active if it has a non-null `mSnapshot`.  As long as there
+   * are any active databases final deltas can't be calculated and
+   * `UpdateUsage()` can't be invoked.
+   */
   nsTHashtable<nsPtrHashKey<Database>> mActiveDatabases;
+  /**
+   * Non-authoritative hashtable representation of mOrderedItems for efficient
+   * lookup.
+   */
   nsDataHashtable<nsStringHashKey, nsString> mValues;
+  /**
+   * The authoritative ordered state of the Datastore; mValue also exists as an
+   * unordered hashtable for efficient lookup.
+   */
   nsTArray<LSItemInfo> mOrderedItems;
   nsTArray<int64_t> mPendingUsageDeltas;
   WriteOptimizer mWriteOptimizer;
   const nsCString mOrigin;
   const uint32_t mPrivateBrowsingId;
   int64_t mUsage;
   int64_t mUpdateBatchUsage;
   int64_t mSizeOfKeys;
@@ -1501,16 +1528,19 @@ public:
   PrivateBrowsingId() const
   {
     return mPrivateBrowsingId;
   }
 
   bool
   IsPersistent() const
   {
+    // Private-browsing is forbidden from touching disk, but
+    // StorageAccess::eSessionScoped is allowed to touch disk because
+    // QuotaManager's storage for such origins is wiped at shutdown.
     return mPrivateBrowsingId == 0;
   }
 
   void
   Close();
 
   bool
   IsClosed() const
@@ -1566,16 +1596,27 @@ public:
                       LSSnapshot::LoadState& aLoadState);
 
   void
   GetItem(const nsString& aKey, nsString& aValue) const;
 
   void
   GetKeys(nsTArray<nsString>& aKeys) const;
 
+  //////////////////////////////////////////////////////////////////////////////
+  // Mutation Methods
+  //
+  // These are only called during Snapshot::RecvCheckpoint
+
+
+  /**
+   * Used by Snapshot::RecvCheckpoint to set a key/value pair as part of a an
+   * explicit batch
+   *
+   */
   void
   SetItem(Database* aDatabase,
           const nsString& aDocumentURI,
           const nsString& aKey,
           const nsString& aOldValue,
           const nsString& aValue);
 
   void
@@ -1855,34 +1896,113 @@ private:
                                        const int64_t& aMinSize,
                                        LSSnapshotInitInfo* aInitInfo) override;
 
   bool
   DeallocPBackgroundLSSnapshotParent(PBackgroundLSSnapshotParent* aActor)
                                      override;
 };
 
+/**
+ * Attempts to capture the state of the the underlying Datastore at the time of
+ * its creation so run-to-completion semantics can be honored.
+ *
+ * Rather than simply duplicate the contents of `DataStore::mValues` and
+ * `Datastore::mOrderedItems` at the time of their creation, the Snapshot tracks
+ * mutations to the Datastore as they happen, saving off the state of values as
+ * they existed when the Snapshot was created.  In other words, given an initial
+ * Datastore state of { foo: 'bar', bar: 'baz' }, the Snapshot won't store those
+ * values until it hears via `SaveItem` that "foo" is being over-written.  At
+ * that time, it will save off foo='bar' in mValues.
+ *
+ * ## Quota Allocation ##
+ *
+ * ## States ##
+ *
+ */
 class Snapshot final
   : public PBackgroundLSSnapshotParent
 {
+  /**
+   * The Database that owns this snapshot.  There is a 1:1 relationship between
+   * snapshots and databases.
+   */
   RefPtr<Database> mDatabase;
   RefPtr<Datastore> mDatastore;
+  /**
+   * The set of keys for which values have been sent to the child LSSnapshot.
+   * Cleared once all values have been sent as indicated by
+   * mLoadedItems.Count()==mTotalLength and therefore mLoadedAllItems should be
+   * true.  No requests should be received for keys already in this set, and
+   * this is enforced by fatal IPC error (unless fuzzing).
+   */
   nsTHashtable<nsStringHashKey> mLoadedItems;
+  /**
+   * The set of keys for which a RecvLoadItem request was received but there
+   * was no such key, and so null was returned.  The child LSSnapshot will also
+   * cache these values, so redundant requests are also handled with fatal
+   * process termination just like for mLoadedItems.  Also cleared when
+   * mLoadedAllItems becomes true because then the child can infer that all
+   * other values must be null.  (Note: this could also be done when
+   * mLoadKeysReceived is true as a further optimization, but is not.)
+   */
   nsTHashtable<nsStringHashKey> mUnknownItems;
+  /**
+   * Values that have changed in mDatastore as reported by SaveItem
+   * notifications that are not yet known to the child LSSnapshot.
+   *
+   * The naive way to snapshot the state of mDatastore would be to duplicate its
+   * internal mValues at the time of our creation, but that is wasteful if few
+   * changes changes are made to the Datastore's state.  So we only track values
+   * that are changed/evicted from the Datastore as they happen, as reported to
+   * us by SaveItem notifications.
+   */
   nsDataHashtable<nsStringHashKey, nsString> mValues;
+  /**
+   * Latched state of mDatastore's keys during a SaveItem notification with
+   * aAffectsOrder=true.  The ordered keys needed to be saved off so that a
+   * consistent ordering could be presented to the child LSSnapshot when it asks
+   * for them via RecvLoadKeys.
+   */
   nsTArray<nsString> mKeys;
   nsString mDocumentURI;
+  /**
+   * The number of key/value pairs that were present in the Datastore at the
+   * time the checkpoint was created.  Once we have sent this many values to
+   * the child LSSnapshot, we can infer that it has received all of the
+   * keys/values and set mLoadedAllItems to true and clear mLoadedItems and
+   * mUnknownItems.  Note that knowing the keys/values is not the same as
+   * knowing their ordering and so mKeys may be retained.
+   */
   uint32_t mTotalLength;
   int64_t mUsage;
   int64_t mPeakUsage;
+  /**
+   * True if SaveItem has saved mDatastore's keys into mKeys because a SaveItem
+   * notification with aAffectsOrder=true was received.
+   */
   bool mSavedKeys;
   bool mActorDestroyed;
   bool mFinishReceived;
   bool mLoadedReceived;
+  /**
+   * True if LSSnapshot's mLoadState should be LoadState::AllOrderedItems or
+   * LoadState::AllUnorderedItems.  It will be AllOrderedItems if the initial
+   * snapshot contained all the data or if the state was AllOrderedKeys and
+   * successive RecvLoadItem requests have resulted in the LSSnapshot being told
+   * all of the key/value pairs.  It will be AllUnorderedItems if the state was
+   * LoadState::Partial and successive RecvLoadItem requests got all the
+   * keys/values but the key ordering was not retrieved.
+   */
   bool mLoadedAllItems;
+  /**
+   * True if LSSnapshot's mLoadState should be LoadState::AllOrderedItems or
+   * AllOrderedKeys.  This can occur because of the initial snapshot, or because
+   * a RecvLoadKeys request was received.
+   */
   bool mLoadKeysReceived;
   bool mSentMarkDirty;
 
 public:
   // Created in AllocPBackgroundLSSnapshotParent.
   Snapshot(Database* aDatabase,
            const nsAString& aDocumentURI);
 
@@ -1910,16 +2030,21 @@ public:
       mLoadKeysReceived = true;
     } else if (aLoadState == LSSnapshot::LoadState::AllOrderedItems) {
       mLoadedReceived = true;
       mLoadedAllItems = true;
       mLoadKeysReceived = true;
     }
   }
 
+  /**
+   * Called via NotifySnapshots by Datastore whenever it is updating its
+   * internal state so that snapshots can save off the state of a value at the
+   * time of their creation.
+   */
   void
   SaveItem(const nsAString& aKey,
            const nsAString& aOldValue,
            bool aAffectsOrder);
 
   void
   MarkDirty();
 
@@ -4962,16 +5087,18 @@ Datastore::NotifyObservers(Database* aDa
 
   nsTArray<Observer*>* array;
   if (!gObservers->Get(mOrigin, &array)) {
     return;
   }
 
   MOZ_ASSERT(array);
 
+  // We do not want to send information about events back to the content process
+  // that caused the change.  They already were
   PBackgroundParent* databaseBackgroundActor = aDatabase->Manager();
 
   for (Observer* observer : *array) {
     if (observer->Manager() != databaseBackgroundActor) {
       observer->Observe(aDatabase, aDocumentURI, aKey, aOldValue, aNewValue);
     }
   }
 }
--- a/dom/localstorage/LSDatabase.h
+++ b/dom/localstorage/LSDatabase.h
@@ -8,16 +8,19 @@
 #define mozilla_dom_localstorage_LSDatabase_h
 
 namespace mozilla {
 namespace dom {
 
 class LSDatabaseChild;
 class LSSnapshot;
 
+/**
+ *
+ */
 class LSDatabase final
 {
   LSDatabaseChild* mActor;
 
   LSSnapshot* mSnapshot;
 
   const nsCString mOrigin;
 
--- a/dom/localstorage/LSObject.cpp
+++ b/dom/localstorage/LSObject.cpp
@@ -23,36 +23,95 @@ namespace dom {
 
 namespace {
 
 class RequestHelper;
 
 StaticMutex gRequestHelperMutex;
 RequestHelper* gRequestHelper = nullptr;
 
+/**
+ * Main-thread helper that implements the blocking logic required by
+ * LocalStorage's synchronous semantics.  StartAndReturnResponse pushes an
+ * event queue which is a new event target and spins its nested event loop until
+ * a result is received or an abort is necessary due to a PContent-managed sync
+ * IPC message being received.  Note that because the event queue is its own
+ * event target, there is no re-entrancy.  Normal main-thread runnables will not
+ * get a chance to run.  See StartAndReturnResponse() for info on this choice.
+ *
+ * The normal life-cycle of this method looks like:
+ * - Main Thread: LSObject::DoRequestSynchronously creates a RequestHelper and
+ *   invokes StartAndReturnResponse().  It pushes the event queue and Dispatches
+ *   the RequestHelper to the DOM File Thread.
+ * - DOM File Thread: RequestHelper::Run is called, invoking Start() which
+ *   invokes LSObject::StartRequest, which gets-or-creates the PBackground actor
+ *   if necessary (which may dispatch a runnable to the nested event queue on
+ *   the main thread), sends LSRequest constructor which is provided with a
+ *   callback reference to the RequestHelper. State advances to ResponsePending.
+ * - DOM File Thread:: LSRequestChild::Recv__delete__ is received, which invokes
+ *   RequestHelepr::OnResponse, advancing the state to Finishing and dispatching
+ *   RequestHelper to its own nested event target.
+ * - Main Thread: RequestHelper::Run is called, invoking Finish() which advances
+ *   the state to Complete and sets mWaiting to false, allowing the nested event
+ *   loop being spun by StartAndReturnResponse to cease spinning and return the
+ *   received response.
+ *
+ * See LocalStorageCommon.h for high-level context and method comments for
+ * low-level details.
+ */
 class RequestHelper final
   : public Runnable
   , public LSRequestChildCallback
 {
   enum class State
   {
+    /**
+     * The RequestHelper has been created and dispatched to the DOM File Thread.
+     */
     Initial,
+    /**
+     * Start() has been invoked on the DOM File Thread and
+     * LSObject::StartRequest has been invoked from there, sending an IPC
+     * message to PBackground to service the request.  We stay in this state
+     * until a response is received.
+     */
     ResponsePending,
+    /**
+     * A response has been received and RequestHelper has been dispatched back
+     * to the nested event loop to call Finish().
+     */
     Finishing,
+    /**
+     * Finish() has been called on the main thread.  The nested event loop will
+     * terminate imminently and the received response returned to the caller of
+     * StartAndReturnResponse.
+     */
     Complete
   };
 
+  // The object we are issuing a request on behalf of.  Present because of the
+  // need to invoke LSObject::StartRequest off the main thread.  Dropped on
+  // return to the main-thread in Finish().
   RefPtr<LSObject> mObject;
+  // The thread the RequestHelper was created on.  This should be the main
+  // thread.
   nsCOMPtr<nsIEventTarget> mOwningEventTarget;
+  // The pushed event queue that we use to spin the event loop without
+  // processing any of the events dispatched at the mOwningEventTarget (which
+  // would result in re-entrancy and violate LocalStorage semantics).
   nsCOMPtr<nsIEventTarget> mNestedEventTarget;
+  // The IPC actor handling the request with standard IPC allocation rules.
+  // Our reference is nulled in OnResponse which corresponds to the actor's
+  // __destroy__ method.
   LSRequestChild* mActor;
   const LSRequestParams mParams;
   LSRequestResponse mResponse;
   nsresult mResultCode;
   State mState;
+  // Control flag for the nested event loop; once set to false, the loop ends.
   bool mWaiting;
 
 public:
   RequestHelper(LSObject* aObject,
                 const LSRequestParams& aParams)
     : Runnable("dom::RequestHelper")
     , mObject(aObject)
     , mOwningEventTarget(GetCurrentThreadEventTarget())
@@ -347,16 +406,24 @@ LSObject::IsForkOf(const Storage* aStora
   return static_cast<const LSObject*>(aStorage)->mOrigin == mOrigin;
 }
 
 int64_t
 LSObject::GetOriginQuotaUsage() const
 {
   AssertIsOnOwningThread();
 
+  // It's not necessary to return an actual value here.  This method is
+  // implemented only because the SessionStore currently needs it to cap the
+  // amount of data it persists to disk (via nsIDOMWindowUtils.getStorageUsage).
+  // Any callers that want to know about storage usage should be asking
+  // QuotaManager directly.
+  //
+  // Note: This may change as LocalStorage is repurposed to be the new
+  // SessionStorage backend.
   return 0;
 }
 
 uint32_t
 LSObject::GetLength(nsIPrincipal& aSubjectPrincipal,
                     ErrorResult& aError)
 {
   AssertIsOnOwningThread();
--- a/dom/localstorage/LSObject.h
+++ b/dom/localstorage/LSObject.h
@@ -28,16 +28,43 @@ namespace dom {
 class LSDatabase;
 class LSObjectChild;
 class LSObserver;
 class LSRequestChild;
 class LSRequestChildCallback;
 class LSRequestParams;
 class LSRequestResponse;
 
+/**
+ * Backs the WebIDL `Storage` binding; all content LocalStorage calls are
+ * handled by this class.
+ *
+ * ## Semantics under e10s / multi-process ##
+ *
+ * A snapshot mechanism used in conjuction with stable points ensures that JS
+ * run-to-completion semantics are experienced even if the same origin is
+ * concurrently accessing LocalStorage across multiple content processes.
+ *
+ * ### Snapshot Consistency ###
+ *
+ * An LSSnapshot is created locally whenever the contents of LocalStorage are
+ * about to be read or written (including length).  This synchronously
+ * establishes a corresponding Snapshot in PBackground in the parent process.
+ * An effort is made to send as much data from the parent process as possible,
+ * so sites using a small/reasonable amount of LocalStorage data will have it
+ * sent to the content process for immediate access.  Sites with greater
+ * LocalStorage usage may only have some of the information relayed.  In that
+ * case, the parent Snapshot will ensure that it retains the exact state of the
+ * parent Datastore at the moment the Snapshot was created.
+ *
+ * ### Quota ###
+ *
+ *
+ *
+ */
 class LSObject final
   : public Storage
 {
   typedef mozilla::ipc::PrincipalInfo PrincipalInfo;
 
   friend nsGlobalWindowInner;
 
   nsAutoPtr<PrincipalInfo> mPrincipalInfo;
@@ -47,35 +74,56 @@ class LSObject final
 
   uint32_t mPrivateBrowsingId;
   nsCString mOrigin;
   nsString mDocumentURI;
 
   bool mInExplicitSnapshot;
 
 public:
+  /**
+   * The normal creation path invoked by nsGlobalWindowInner.
+   */
   static nsresult
   CreateForWindow(nsPIDOMWindowInner* aWindow,
                   Storage** aStorage);
 
+  /**
+   * nsIDOMStorageManager creation path for use in testing logic.  Supports the
+   * system principal where CreateForWindow does not.  This is also why aPrivate
+   * exists separate from the principal; because the system principal can never
+   * be mutated to have a private browsing id even though it can be used in a
+   * window/document marked as private browsing.  That's a legacy issue that is
+   * being dealt with, but it's why it exists here.
+   */
   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();
 
+  /**
+   * Helper invoked by ContentChild::OnChannelReceivedMessage when a sync IPC
+   * message is received.  This will be invoked on the IPC I/O thread and it's
+   * necessary to unblock the main thread when this happens to avoid the
+   * potential for browser deadlock.  This should only occur in (ugly) testing
+   * scenarios where CPOWs are in use.
+   *
+   * Cancellation will result in the underlying LSRequest being explicitly
+   * canceled, resulting in the parent sending an NS_ERROR_FAILURE result.
+   */
   static void
   CancelSyncLoop();
 
   void
   AssertIsOnOwningThread() const
   {
     NS_ASSERT_OWNINGTHREAD(LSObject);
   }
@@ -130,16 +178,18 @@ public:
   RemoveItem(const nsAString& aKey,
              nsIPrincipal& aSubjectPrincipal,
              ErrorResult& aError) override;
 
   void
   Clear(nsIPrincipal& aSubjectPrincipal,
         ErrorResult& aError) override;
 
+  //////////////////////////////////////////////////////////////////////////////
+  // Testing Methods: See Storage.h
   void
   Open(nsIPrincipal& aSubjectPrincipal,
        ErrorResult& aError) override;
 
   void
   Close(nsIPrincipal& aSubjectPrincipal,
         ErrorResult& aError) override;
 
--- a/dom/localstorage/LSSnapshot.h
+++ b/dom/localstorage/LSSnapshot.h
@@ -11,26 +11,64 @@ namespace mozilla {
 namespace dom {
 
 class LSDatabase;
 class LSNotifyInfo;
 class LSSnapshotChild;
 class LSSnapshotInitInfo;
 class LSWriteInfo;
 
+/**
+ *
+ */
 class LSSnapshot final
   : public nsIRunnable
 {
 public:
+  /**
+   * The LoadState expresses what subset of information a snapshot has from the
+   * authoritative Datastore in the parent process.  The initial snapshot is
+   * populated heuristically based on the size of the keys and size of the items
+   * (inclusive of the key value; item is key+value, not just value) of the
+   * entire datastore relative to the configured prefill limit (via pref
+   * "dom.storage.snapshot_prefill" exposed as gSnapshotPrefill in bytes).
+   *
+   * If there's less data than the limit, we send both keys and values and end
+   * up as AllOrderedItems.  If there's enough room for all the keys but not
+   * all the values, we end up as AllOrderedKeys with as many values present as
+   * would fit.  If there's not enough room for all the keys, then we end up as
+   * Partial with as many key-value pairs as will fit.
+   *
+   * The state AllUnorderedItems can only be reached by code getting items one
+   * by one.
+   */
   enum class LoadState
   {
+    /**
+     * Class constructed, Init(LSSnapshotInitInfo) has not been invoked yet.
+     */
     Initial,
+    /**
+     * Some keys and their values are known.
+     */
     Partial,
+    /**
+     * All the keys are known in order, but some values
+     */
     AllOrderedKeys,
     AllUnorderedItems,
+    /**
+     * All keys and their values are known and are present in their canonical
+     * order.  This is everything, and is the preferred case.  The initial
+     * population will send this info when the size of all items is less than
+     * the prefill threshold.
+     *
+     * mValues will contain all keys and values, mLoadedItems and mUnknownItems
+     * are unused.
+     */
     AllOrderedItems,
     EndGuard
   };
 
 private:
   RefPtr<LSSnapshot> mSelfRef;
 
   RefPtr<LSDatabase> mDatabase;
--- a/dom/localstorage/LocalStorageCommon.h
+++ b/dom/localstorage/LocalStorageCommon.h
@@ -174,24 +174,31 @@
  *
  * Local storage manager
  * ~~~~~~~~~~~~~~~~~~~~~
  *
  * The local storage manager exposes some of the features that need to be
  * available only in the chrome code or tests. The manager is represented by
  * the "LocalStorageManager2" class that implements the "nsIDOMStorageManager"
  * interface.
- */;
+ */
 
 namespace mozilla {
 namespace dom {
 
 extern const char16_t* kLocalStorageType;
 
-class LSNotifyInfo
+/**
+ * Convenience data-structure to make it easier to track whether a value has
+ * changed and what its previous value was for notification purposes.  Instances
+ * are created on the stack by LSObject and passed to LSDatabase which in turn
+ * passes them onto LSSnapshot for final updating/population.  LSObject then
+ * generates an event, if appropriate.
+ */
+class MOZ_STACK_CLASS LSNotifyInfo
 {
   bool mChanged;
   nsString mOldValue;
 
 public:
   LSNotifyInfo()
     : mChanged(false)
   { }
@@ -216,18 +223,25 @@ public:
 
   nsString&
   oldValue()
   {
     return mOldValue;
   }
 };
 
+/**
+ * Main-thread-only check of LSNG being enabled, the value is latched once
+ * initialized so changing the preference during runtime has no effect.
+ */
 bool
 NextGenLocalStorageEnabled();
 
+/**
+ * Cached any-thread version of NextGenLocalStorageEnabled().
+ */
 bool
 CachedNextGenLocalStorageEnabled();
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_localstorage_LocalStorageCommon_h
--- a/dom/localstorage/LocalStorageManager2.cpp
+++ b/dom/localstorage/LocalStorageManager2.cpp
@@ -128,16 +128,21 @@ NS_IMPL_ISUPPORTS(LocalStorageManager2,
 NS_IMETHODIMP
 LocalStorageManager2::PrecacheStorage(nsIPrincipal* aPrincipal,
                                       Storage** _retval)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aPrincipal);
   MOZ_ASSERT(_retval);
 
+  // This method was created as part of the e10s-ification of the old LS
+  // implementation to perform a preload in the content/current process.  That's
+  // not how things work in LSNG.  Instead everything happens in the parent
+  // process, triggered by the official preloading spot,
+  // ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild.
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 LocalStorageManager2::CreateStorage(mozIDOMWindow* aWindow,
                                     nsIPrincipal* aPrincipal,
                                     const nsAString& aDocumentURI,
                                     bool aPrivate,
@@ -177,29 +182,32 @@ LocalStorageManager2::GetStorage(mozIDOM
 }
 
 NS_IMETHODIMP
 LocalStorageManager2::CloneStorage(Storage* aStorageToCloneFrom)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aStorageToCloneFrom);
 
+  // Cloning is specific to sessionStorage; state is forked when a new tab is
+  // opened from an existing tab.
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 LocalStorageManager2::CheckStorage(nsIPrincipal* aPrincipal,
                                    Storage *aStorage,
                                    bool* _retval)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aPrincipal);
   MOZ_ASSERT(aStorage);
   MOZ_ASSERT(_retval);
 
+  // Only used by sessionStorage.
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 LocalStorageManager2::GetNextGenLocalStorageEnabled(bool* aResult)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aResult);
--- a/dom/localstorage/LocalStorageManager2.h
+++ b/dom/localstorage/LocalStorageManager2.h
@@ -12,34 +12,55 @@
 
 namespace mozilla {
 namespace dom {
 
 class LSRequestParams;
 class LSSimpleRequestParams;
 class Promise;
 
+/**
+ * Under LSNG this is basically just a place for test logic that doesn't make
+ * sense to put directly on the Storage WebIDL interface.  Previously,
+ * the nsIDOMStorageManager XPCOM interface was also used by nsGlobalWindowInner
+ * to interact with LocalStorage, but in these de-XPCOM days, we've moved to
+ * just directly reference the relevant concrete classes (ex: LSObject)
+ * directly.
+ *
+ * Note that testing methods are now also directly exposed on the Storage WebIDL
+ * interface for simplicity/sanity.
+ */
 class LocalStorageManager2 final
   : public nsIDOMStorageManager
   , public nsILocalStorageManager
 {
 public:
   LocalStorageManager2();
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIDOMSTORAGEMANAGER
   NS_DECL_NSILOCALSTORAGEMANAGER
 
 private:
   ~LocalStorageManager2();
 
+  /**
+   * Helper to trigger an LSRequest and resolve/reject the provided promise when
+   * the result comes in.  This routine is notable because the LSRequest
+   * mechanism is normally used synchronously from content, but here it's
+   * exposed asynchronously.
+   */
   nsresult
   StartRequest(Promise* aPromise,
                const LSRequestParams& aParams);
 
+  /**
+   * Helper to trigger an LSSimpleRequst and resolve/reject the provided promise
+   * when the result comes in.
+   */
   nsresult
   StartSimpleRequest(Promise* aPromise,
                      const LSSimpleRequestParams& aParams);
 };
 
 } // namespace dom
 } // namespace mozilla
 
--- a/dom/localstorage/PBackgroundLSDatabase.ipdl
+++ b/dom/localstorage/PBackgroundLSDatabase.ipdl
@@ -8,31 +8,77 @@ include protocol PBackgroundLSSnapshot;
 include "mozilla/dom/localstorage/SerializationHelpers.h";
 
 using mozilla::dom::LSSnapshot::LoadState
   from "mozilla/dom/LSSnapshot.h";
 
 namespace mozilla {
 namespace dom {
 
+/**
+ * LocalStorage key/value pair wire representations.  `value` may be void in
+ * cases where there is a value but it is not being sent for memory/bandwidth
+ * conservation purposes.  (It's not possible to have a null/undefined `value`
+ * as Storage is defined explicitly as a String store.)
+ */
 struct LSItemInfo
 {
   nsString key;
   nsString value;
 };
 
+/**
+ * Initial LSSnapshot state as produced by Datastore::GetSnapshotInitInfo.  See
+ * `LSSnapshot::LoadState` for more details about the possible states and a
+ * high level overview.
+ */
 struct LSSnapshotInitInfo
 {
+  /**
+   * As many key/value or key/void pairs as the snapshot prefill byte budget
+   * allowed.
+   */
   LSItemInfo[] itemInfos;
+  /**
+   * The total number of key/value pairs in LocalStorage for this origin at the
+   * time the snapshot was created.  (And the point of the snapshot is to
+   * conceptually freeze the state of the Datastore in time, so this value does
+   * not change despite what other LSDatabase objects get up to in other
+   * processes.)
+   */
   uint32_t totalLength;
+  /**
+   * The current amount of LocalStorage usage as measured by the summing the
+   * nsString Length() of both the key and the value over all stored pairs.
+   */
   int64_t initialUsage;
+  /**
+   * The amount of storage allowed to be used by the Snapshot without requesting
+   * more storage space via IncreasePeakUsage.  This is the `initialUsage` plus
+   * 0 or more bytes of space.  If space was available, the increase will be the
+   * `requestedSize` from the PBackgroundLSSnapshot constructor.  If the
+   * LocalStorage usage was already close to the limit, then the fallback is the
+   * `minSize` requested, or 0 if there wasn't space for that.
+   */
   int64_t peakUsage;
+  // See `LSSnapshot::LoadState` in `LSSnapshot.h`
   LoadState loadState;
 };
 
+/**
+ * This protocol is asynchronously created via constructor on PBackground but
+ * has synchronous semantics from the perspective of content on the main thread.
+ * The construction potentially involves waiting for disk I/O to load the
+ * LocalStorage data from disk as well as related QuotaManager checks, so async
+ * calls to PBackground are the only viable mechanism because blocking
+ * PBackground is not acceptable.  (Note that an attempt is made to minimize any
+ * I/O latency by triggering preloading from
+ * ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild, the central place
+ * for pre-loading.)
+ */
 sync protocol PBackgroundLSDatabase
 {
   manager PBackground;
   manages PBackgroundLSSnapshot;
 
 parent:
   // The DeleteMe message is used to avoid a race condition between the parent
   // actor and the child actor. The PBackgroundLSDatabase protocol could be
@@ -40,24 +86,60 @@ parent:
   // However, that would destroy the child actor immediatelly and the parent
   // could be sending a message to the child at the same time resulting in a
   // routing error since the child actor wouldn't exist anymore. A routing
   // error typically causes a crash. The race can be prevented by doing the
   // teardown in two steps. First, we send the DeleteMe message to the parent
   // and the parent then sends the __delete__ message to the child.
   async DeleteMe();
 
+  /**
+   * Sent in response to a `RequestAllowToClose` message once the snapshot
+   * cleanup has happened OR from LSDatabase's destructor if AllowToClose has
+   * not already been reported.
+   */
   async AllowToClose();
 
+  /**
+   * Invoked to create an LSSnapshot backed by a Snapshot in PBackground that
+   * presents an atomic and consistent view of the state of the authoritative
+   * Datastore state in the parent.
+   *
+   * This needs to be synchronous because LocalStorage's semantics are
+   * synchronous.  Note that the Datastore in the PBackground parent already
+   * has the answers to this request immediately available without needing to
+   * consult any other threads or perform any I/O.  Additionally, the response
+   * is explicitly bounded in size by the tunable snapshot prefill byte limit.
+   *
+   * @param increasePeakUsage
+   *   Whether the parent should attempt to pre-allocate some amount of quota
+   *   usage to the Snapshot so that it can
+   */
   sync PBackgroundLSSnapshot(nsString documentURI,
                              bool increasePeakUsage,
                              int64_t requestedSize,
                              int64_t minSize)
     returns (LSSnapshotInitInfo initInfo);
 
 child:
   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.
+   *
+   * Note that the QuotaManager shutdown process is more likely to happen in
+   * unit tests where we explicitly reset the QuotaManager.  At runtime, we
+   * expect windows to be closed and content processes terminated well before
+   * QuotaManager shutdown would actually occur.
+   *
+   * Also, Operations are usually aborted for an origin due to privacy API's
+   * clearing data for an origin.  Operations are aborted for a process by
+   * ContentParent::ShutDownProcess.
+   */
   async RequestAllowToClose();
 };
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/localstorage/PBackgroundLSRequest.ipdl
+++ b/dom/localstorage/PBackgroundLSRequest.ipdl
@@ -28,16 +28,28 @@ struct LSRequestPrepareObserverResponse
 
 union LSRequestResponse
 {
   nsresult;
   LSRequestPrepareDatastoreResponse;
   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.
+ * -
+ *
+ *
+ */
 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
   // to the main thread in the content process at the time we are blocking
@@ -45,20 +57,46 @@ parent:
   // We use the PBackground thread on the parent side to handle requests, but
   // sometimes we need to get information from principals and that's currently
   // only possible on the main thread. So if the main thread in the chrome
   // process is blocked by a CPOW operation, our request must wait for the CPOW
   // operation to complete. However the CPOW operation can't complete either
   // because we are blocking the main thread in the content process.
   // The dead lock is prevented by canceling our nested event loop in the
   // content process when we receive a synchronous IPC message from the parent.
+  //
+  // Note that cancellation isn't instantaneous.  It's just an asynchronous flow
+  // that definitely doesn't involve the main thread in the parent process, so
+  // we're guaranteed to unblock the main-thread in the content process and
+  // allow the sync IPC to make progress.  When Cancel() is received by the
+  // parent, it will Send__delete__.  The child will either send Cancel or
+  // Finish, but not both.
   async Cancel();
 
+  /**
+   * Sent by the child in response to Ready, requesting that __delete__ be sent
+   * with the result.  The child will either send Finish or Cancel, but not
+   * both.  No further message will be sent from the child after invoking one.
+   */
   async Finish();
 
 child:
+  /**
+   * The deletion is sent with the result of the request directly in response to
+   * either Cancel or Finish.
+   */
   async __delete__(LSRequestResponse response);
 
+  /**
+   * Sent by the parent when it has completed whatever async stuff it needs to
+   * do and is ready to send the results.  It then awaits the Finish() call to
+   * send the results.  This may seem redundant, but it's not.  If the
+   * __delete__ was sent directly, it's possible there could be a race where
+   * Cancel() would be received by the parent after it had already sent
+   * __delete__.  (Which may no longer be fatal thanks to improvements to the
+   * IPC layer, but it would still lead to warnings, etc.  And we don't
+   * expect PBackground to be highly contended nor the DOM File thread.)
+   */
   async Ready();
 };
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/localstorage/PBackgroundLSSimpleRequest.ipdl
+++ b/dom/localstorage/PBackgroundLSSimpleRequest.ipdl
@@ -2,27 +2,46 @@
  * 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 protocol PBackground;
 
 namespace mozilla {
 namespace dom {
 
+/**
+ * Response to a `LSSimpleRequestPreloadedParams` request indicating whether the
+ * origin was preloaded.
+ */
 struct LSSimpleRequestPreloadedResponse
 {
   bool preloaded;
 };
 
+/**
+ * Discriminated union where `nsresult` conveys an error code
+ */
 union LSSimpleRequestResponse
 {
   nsresult;
   LSSimpleRequestPreloadedResponse;
 };
 
+/**
+ * Simple requests are async-only from both a protocol perspective and the
+ * manner in which they're used.  In comparison, PBackgroundLSRequests are
+ * async only from a protocol perspective; they are used synchronously from the
+ * main thread via LSObject's RequestHelper mechanism.  (With the caveat that
+ * nsILocalStorageManager does expose LSRequests asynchronously.)
+ *
+ * These requests use the common idiom where the arguments to the request are
+ * sent in the constructor and the result is sent in the __delete__ response.
+ * Request types are indicated by the Params variant used and those live in
+ * `PBackgroundLSSharedTypes.ipdlh`.
+ */
 protocol PBackgroundLSSimpleRequest
 {
   manager PBackground;
 
 child:
   async __delete__(LSSimpleRequestResponse response);
 };
 
--- a/dom/localstorage/PBackgroundLSSnapshot.ipdl
+++ b/dom/localstorage/PBackgroundLSSnapshot.ipdl
@@ -20,16 +20,19 @@ struct LSRemoveItemInfo
   nsString key;
   nsString oldValue;
 };
 
 struct LSClearInfo
 {
 };
 
+/**
+ * Union of LocalStorage mutation types.
+ */
 union LSWriteInfo
 {
   LSSetItemInfo;
   LSRemoveItemInfo;
   LSClearInfo;
 };
 
 sync protocol PBackgroundLSSnapshot
@@ -55,15 +58,23 @@ parent:
     returns (int64_t size);
 
   // A synchronous ping to the parent actor to confirm that the parent actor
   // has received previous async message. This should only be used by the
   // snapshotting code to end an explicit snapshot.
   sync Ping();
 
 child:
+  /**
+   * Compels the child LSSnapshot to Checkpoint() and Finish(), effectively
+   * compelling the snapshot to flush any issued mutations and close itself.
+   *
+   * Currently this message is only expected to be sent when the last private
+   * browsing context exits.  And in that case we expect all private browsing
+   * globals to already have been destroyed.
+   */
   async MarkDirty();
 
   async __delete__();
 };
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/localstorage/nsILocalStorageManager.idl
+++ b/dom/localstorage/nsILocalStorageManager.idl
@@ -3,19 +3,33 @@
 /* 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 "nsISupports.idl"
 
 interface nsIPrincipal;
 
+/**
+ * Methods specific to LocalStorage, see nsIDOMStorageManager for methods shared
+ * with SessionStorage.  Methods may migrate there as SessionStorage is
+ * overhauled.
+ */
 [scriptable, builtinclass, uuid(d4f534da-2744-4db3-8774-8b187c64ade9)]
 interface nsILocalStorageManager : nsISupports
 {
   readonly attribute boolean nextGenLocalStorageEnabled;
 
+  /**
+   * Trigger preload of LocalStorage for the given principal.  For use by
+   * ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild to maximize the
+   * amount of time we have to load the data off disk before the page might
+   * attempt to touch LocalStorage.
+   */
   [implicit_jscontext] nsISupports
   preload(in nsIPrincipal aPrincipal);
 
+  /**
+   *
+   */
   [implicit_jscontext] nsISupports
   isPreloaded(in nsIPrincipal aPrincipal);
 };
--- a/dom/storage/Storage.h
+++ b/dom/storage/Storage.h
@@ -104,37 +104,51 @@ public:
     aFound = !aRv.ErrorCodeIs(NS_SUCCESS_DOM_NO_OPERATION);
   }
 
   virtual void
   Clear(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv) = 0;
 
   bool IsSessionOnly() const { return mIsSessionOnly; }
 
+  //////////////////////////////////////////////////////////////////////////////
+  // Testing Methods:
+  //
+  // These methods are exposed on the `Storage` WebIDL interface behind a
+  // preference for the benefit of automated-tests.  They are not exposed to
+  // content.  See `Storage.webidl` for more details.
+
   virtual void
   Open(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv)
   { }
 
   virtual void
   Close(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv)
   { }
 
   virtual void
   BeginExplicitSnapshot(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv)
   { }
 
   virtual void
   EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv)
   { }
 
+  //////////////////////////////////////////////////////////////////////////////
+
+  // Dispatch storage notification events on all impacted pages in the current
+  // process as well as for consumption by devtools.  Pages receive the
+  // notification via StorageNotifierService (not observers like in the past),
+  // while devtools does receive the notification via the observer service.
+  //
   // aStorage can be null if this method is called by LocalStorageCacheChild.
   //
   // aImmediateDispatch is for use by child IPC code (LocalStorageCacheChild)
   // so that PBackground ordering can be maintained.  Without this, the event
-  // would be/ enqueued and run in a future turn of the event loop, potentially
+  // would be enqueued and run in a future turn of the event loop, potentially
   // allowing other PBackground Recv* methods to trigger script that wants to
   // assume our localstorage changes have already been applied.  This is the
   // case for message manager messages which are used by ContentTask testing
   // logic and webextensions.
   static void
   NotifyChange(Storage* aStorage, nsIPrincipal* aPrincipal,
                const nsAString& aKey, const nsAString& aOldValue,
                const nsAString& aNewValue, const char16_t* aStorageType,
--- a/dom/webidl/Storage.webidl
+++ b/dom/webidl/Storage.webidl
@@ -50,14 +50,18 @@ partial interface Storage {
 
   /**
    * 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();
 
+  /**
+   * Ensures the database has been opened and initiates an explicit snapshot.
+   *
+   */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void beginExplicitSnapshot();
 
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void endExplicitSnapshot();
 };
--- a/ipc/glue/PBackground.ipdl
+++ b/ipc/glue/PBackground.ipdl
@@ -116,26 +116,46 @@ parent:
 
   async PBackgroundIndexedDBUtils();
 
   // Use only for testing!
   async FlushPendingFileDeletions();
 
   async PBackgroundSDBConnection(PrincipalInfo principalInfo);
 
+  /**
+   *
+   */
   async PBackgroundLSDatabase(PrincipalInfo principalInfo,
                               uint32_t privateBrowsingId,
                               uint64_t datastoreId);
 
+  /**
+   *
+   */
   async PBackgroundLSObserver(uint64_t observerId);
 
+  /**
+   * Issue an asynchronous request that will be used in a synchronous fashion
+   * through complex machinations described in `PBackgroundLSRequest.ipdl` and
+   * `LSObject.h`.
+   */
   async PBackgroundLSRequest(LSRequestParams params);
 
+  /**
+   * Issues a simple, non-cancelable asynchronous request that's used in an
+   * asynchronous fashion by callers.  (LSRequest is not simple because it used
+   * in a synchronous fashion which leads to complexities regarding cancelation,
+   * see `PBackgroundLSRequest.ipdl` for details.)
+   */
   async PBackgroundLSSimpleRequest(LSSimpleRequestParams params);
 
+  /**
+   *
+   */
   async PBackgroundLocalStorageCache(PrincipalInfo principalInfo,
                                      nsCString originKey,
                                      uint32_t privateBrowsingId);
 
   async PBackgroundStorage(nsString profilePath);
 
   async PVsync();