Bug 1286798 - Part 53: Review code comments; r=janv,mrbkap,mccr8
authorAndrew Sutherland <asutherland@asutherland.org>
Mon, 05 Nov 2018 14:04:39 -0500
changeset 508051 fd65b14ab7e8aba272dd49f84af229b3ecef6e0a
parent 508050 d4212b87659b68493dda32877783bbbd1bf8e250
child 508052 11c709d6c6d965459d14cbf20d52bc4fabccf391
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanv, mrbkap, mccr8
bugs1286798
milestone65.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 1286798 - Part 53: Review code comments; r=janv,mrbkap,mccr8
dom/interfaces/storage/nsIDOMStorageManager.idl
dom/localstorage/ActorsChild.h
dom/localstorage/ActorsParent.cpp
dom/localstorage/LSObject.cpp
dom/localstorage/LSObject.h
dom/localstorage/LSObserver.h
dom/localstorage/LSSnapshot.h
dom/localstorage/LocalStorageCommon.h
dom/localstorage/LocalStorageManager2.cpp
dom/localstorage/LocalStorageManager2.h
dom/localstorage/PBackgroundLSDatabase.ipdl
dom/localstorage/PBackgroundLSObserver.ipdl
dom/localstorage/PBackgroundLSRequest.ipdl
dom/localstorage/PBackgroundLSSimpleRequest.ipdl
dom/localstorage/PBackgroundLSSnapshot.ipdl
dom/localstorage/nsILocalStorageManager.idl
dom/storage/Storage.h
dom/storage/StorageNotifierService.h
dom/webidl/Storage.webidl
ipc/glue/PBackground.ipdl
ipc/ipdl/sync-messages.ini
--- 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/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/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -114,33 +114,91 @@ static_assert(kSQLitePageSizeOverride ==
 // Set to some multiple of the page size to grow the database in larger chunks.
 const uint32_t kSQLiteGrowthIncrement = kSQLitePageSizeOverride * 2;
 
 static_assert(kSQLiteGrowthIncrement >= 0 &&
               kSQLiteGrowthIncrement % kSQLitePageSizeOverride == 0 &&
               kSQLiteGrowthIncrement < uint32_t(INT32_MAX),
               "Must be 0 (disabled) or a positive multiple of the page size!");
 
+/**
+ * The database name for LocalStorage data in a per-origin directory.
+ */
 #define DATA_FILE_NAME "data.sqlite"
+/**
+ * The journal corresponding to DATA_FILE_NAME.  (We don't use WAL mode.)
+ */
 #define JOURNAL_FILE_NAME "data.sqlite-journal"
 
+/**
+ * How long between the first moment we know we have data to be written on a
+ * `Connection` and when we should actually perform the write.  This helps
+ * limit disk churn under silly usage patterns and is historically consistent
+ * with the previous, legacy implementation.
+ *
+ * Note that flushing happens downstream of Snapshot checkpointing and its
+ * batch mechanism which helps avoid wasteful IPC in the case of silly content
+ * code.
+ */
 const uint32_t kFlushTimeoutMs = 5000;
 
 const char kPrivateBrowsingObserverTopic[] = "last-pb-context-exited";
 
 const uint32_t kDefaultOriginLimitKB = 5 * 1024;
 const uint32_t kDefaultShadowWrites = true;
 const uint32_t kDefaultSnapshotPrefill = 4096;
+/**
+ * LocalStorage data limit as determined by summing up the lengths of all string
+ * keys and values.  This is consistent with the legacy implementation and other
+ * browser engines.  This value should really only ever change in unit testing
+ * where being able to lower it makes it easier for us to test certain edge
+ * cases.
+ */
 const char kDefaultQuotaPref[] = "dom.storage.default_quota";
+/**
+ * Should all mutations also be reflected in the "shadow" database, which is
+ * the legacy webappsstore.sqlite database.  When this is enabled, users can
+ * downgrade their version of Firefox and/or otherwise fall back to the legacy
+ * implementation without loss of data.  (Older versions of Firefox will
+ * recognize the presence of ls-archive.sqlite and purge it and the other
+ * LocalStorage directories so privacy is maintained.)
+ */
 const char kShadowWritesPref[] = "dom.storage.shadow_writes";
+/**
+ * Byte budget for sending data down to the LSSnapshot instance when it is first
+ * created.  If there is less data than this (measured by tallying the string
+ * length of the keys and values), all data is sent, otherwise partial data is
+ * sent.  See `Snapshot`.
+ */
 const char kSnapshotPrefillPref[] = "dom.storage.snapshot_prefill";
 
+/**
+ * The amount of time a PreparedDatastore instance should stick around after a
+ * preload is triggered in order to give time for the page to use LocalStorage
+ * without triggering worst-case synchronous jank.
+ */
 const uint32_t kPreparedDatastoreTimeoutMs = 20000;
 
+/**
+ * Cold storage for LocalStorage data extracted from webappsstore.sqlite at
+ * LSNG first-run that has not yet been migrated to its own per-origin directory
+ * by use.
+ *
+ * In other words, at first run, LSNG copies the contents of webappsstore.sqlite
+ * into this database.  As requests are made for that LocalStorage data, the
+ * contents are removed from this database and placed into per-origin QM
+ * storage.  So the contents of this database are always old, unused
+ * LocalStorage data that we can potentially get rid of at some point in the
+ * future.
+ */
 #define LS_ARCHIVE_FILE_NAME "ls-archive.sqlite"
+/**
+ * The legacy LocalStorage database.  Its contents are maintained as our
+ * "shadow" database so that LSNG can be disabled without loss of user data.
+ */
 #define WEB_APPS_STORE_FILE_NAME "webappsstore.sqlite"
 
 // Shadow database Write Ahead Log's maximum size is 512KB
 const uint32_t kShadowMaxWALSize = 512 * 1024;
 
 const uint32_t kShadowJournalSizeLimit = kShadowMaxWALSize * 3;
 
 bool
@@ -943,16 +1001,26 @@ DetachShadowDatabase(mozIStorageConnecti
 
   return NS_OK;
 }
 
 /*******************************************************************************
  * Non-actor class declarations
  ******************************************************************************/
 
+/**
+ * Coalescing manipulation queue used by `Connection` and `DataStore`.  Used by
+ * `Connection` to buffer and coalesce manipulations applied to the Datastore
+ * in batches by Snapshot Checkpointing until flushed to disk.  Used by
+ * `Datastore` to update `DataStore::mOrderedItems` efficiently/for code
+ * simplification.  (DataStore does not actually depend on the coalescing, as
+ * mutations are applied atomically when a Snapshot Checkpoints, and with
+ * `Datastore::mValues` being updated at the same time the mutations are applied
+ * to Datastore's mWriteOptimizer.)
+ */
 class WriteOptimizer final
 {
   class WriteInfo;
   class AddItemInfo;
   class UpdateItemInfo;
   class RemoveItemInfo;
   class ClearInfo;
 
@@ -996,16 +1064,22 @@ public:
 
   void
   ApplyWrites(nsTArray<LSItemInfo>& aOrderedItems);
 
   nsresult
   PerformWrites(Connection* aConnection, bool aShadowWrites);
 };
 
+/**
+ * Base class for specific mutations.  Each subclass knows how to `Perform` the
+ * manipulation against a `Connection` and the "shadow" database (legacy
+ * webappsstore.sqlite database that exists so LSNG can be disabled/safely
+ * downgraded from.)
+ */
 class WriteOptimizer::WriteInfo
 {
 public:
   enum Type {
     AddItem = 0,
     UpdateItem,
     RemoveItem,
     Clear
@@ -1015,16 +1089,19 @@ public:
   GetType() = 0;
 
   virtual nsresult
   Perform(Connection* aConnection, bool aShadowWrites) = 0;
 
   virtual ~WriteInfo() = default;
 };
 
+/**
+ * SetItem mutation where the key did not previously exist.
+ */
 class WriteOptimizer::AddItemInfo
   : public WriteInfo
 {
   nsString mKey;
   nsString mValue;
 
 public:
   AddItemInfo(const nsAString& aKey,
@@ -1051,16 +1128,19 @@ private:
   {
     return AddItem;
   }
 
   nsresult
   Perform(Connection* aConnection, bool aShadowWrites) override;
 };
 
+/**
+ * SetItem mutation where the key already existed.
+ */
 class WriteOptimizer::UpdateItemInfo final
   : public AddItemInfo
 {
 public:
   UpdateItemInfo(const nsAString& aKey,
                  const nsAString& aValue)
     : AddItemInfo(aKey, aValue)
   { }
@@ -1095,16 +1175,19 @@ private:
   {
     return RemoveItem;
   }
 
   nsresult
   Perform(Connection* aConnection, bool aShadowWrites) override;
 };
 
+/**
+ * Clear mutation.
+ */
 class WriteOptimizer::ClearInfo final
   : public WriteInfo
 {
 public:
   ClearInfo()
   { }
 
 private:
@@ -1295,16 +1378,17 @@ public:
   }
 
   ArchivedOriginScope*
   GetArchivedOriginScope() const
   {
     return mArchivedOriginScope;
   }
 
+  //////////////////////////////////////////////////////////////////////////////
   // Methods which can only be called on the owning thread.
 
   // This method is used to asynchronously execute a connection datastore
   // operation on the connection thread.
   void
   Dispatch(ConnectionDatastoreOperationBase* aOp);
 
   // This method is used to asynchronously close the storage connection on the
@@ -1327,16 +1411,17 @@ public:
   Clear();
 
   void
   BeginUpdateBatch();
 
   void
   EndUpdateBatch();
 
+  //////////////////////////////////////////////////////////////////////////////
   // Methods which can only be called on the connection thread.
 
   nsresult
   EnsureStorageConnection();
 
   mozIStorageConnection*
   StorageConnection() const
   {
@@ -1466,27 +1551,64 @@ public:
   Shutdown();
 
   NS_INLINE_DECL_REFCOUNTING(ConnectionThread)
 
 private:
   ~ConnectionThread();
 };
 
+/**
+ * Canonical state of Storage for an origin, containing all keys and their
+ * values in the parent process.  Specifically, this is the state that will
+ * be handed out to freshly created Snapshots and that will be persisted to disk
+ * when the Connection's flush completes.  State is mutated in batches as
+ * Snapshot instances Checkpoint their mutations locally accumulated in the
+ * child LSSnapshots.
+ */
 class Datastore final
 {
   RefPtr<DirectoryLock> mDirectoryLock;
   RefPtr<Connection> mConnection;
   RefPtr<QuotaObject> mQuotaObject;
   nsCOMPtr<nsIRunnable> mCompleteCallback;
+  /**
+   * PrepareDatastoreOps register themselves with the Datastore at
+   * and unregister in PrepareDatastoreOp::Cleanup.
+   */
   nsTHashtable<nsPtrHashKey<PrepareDatastoreOp>> mPrepareDatastoreOps;
+  /**
+   * PreparedDatastore instances register themselves with their associated
+   * Datastore at construction time and unregister at destruction time.  They
+   * hang around for kPreparedDatastoreTimeoutMs in order to keep the Datastore
+   * from closing itself via MaybeClose(), thereby giving the document enough
+   * time to load and access LocalStorage.
+   */
   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
+   * Database 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;
@@ -1519,16 +1641,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
@@ -1584,16 +1709,25 @@ 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
@@ -1873,34 +2007,113 @@ private:
                                        const int64_t& aMinSize,
                                        LSSnapshotInitInfo* aInitInfo) override;
 
   bool
   DeallocPBackgroundLSSnapshotParent(PBackgroundLSSnapshotParent* aActor)
                                      override;
 };
 
+/**
+ * Attempts to capture the state of 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 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 snapshot 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);
 
@@ -1928,16 +2141,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();
 
@@ -4979,16 +5197,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.
   PBackgroundParent* databaseBackgroundActor = aDatabase->Manager();
 
   for (Observer* observer : *array) {
     if (observer->Manager() != databaseBackgroundActor) {
       observer->Observe(aDatabase, aDocumentURI, aKey, aOldValue, aNewValue);
     }
   }
 }
--- 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())
@@ -345,16 +404,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,38 @@ 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.
+ */
 class LSObject final
   : public Storage
 {
   typedef mozilla::ipc::PrincipalInfo PrincipalInfo;
 
   friend nsGlobalWindowInner;
 
   nsAutoPtr<PrincipalInfo> mPrincipalInfo;
@@ -47,35 +69,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,32 +173,36 @@ 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;
 
   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();
@@ -165,22 +212,42 @@ 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 prepare a new observer in the parent process and also construction of
+   * corresponding actors, which will result in the observer being fully
+   * registered in the parent process.
+   */
   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/LSSnapshot.h
+++ b/dom/localstorage/LSSnapshot.h
@@ -15,22 +15,60 @@ 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 are unknown.
+     */
     AllOrderedKeys,
+    /**
+     * All keys and their values are known, but in an arbitrary order.
+     */
     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
@@ -181,16 +181,23 @@
  * interface.
  */
 
 namespace mozilla {
 namespace dom {
 
 extern const char16_t* kLocalStorageType;
 
+/**
+ * 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,57 @@
 
 namespace mozilla {
 namespace dom {
 
 class LSRequestParams;
 class LSSimpleRequestParams;
 class Promise;
 
+/**
+ * Under LSNG this exposes nsILocalStorageManager::Preload to ContentParent to
+ * trigger preloading.  Otherwise, 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,63 @@ 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.
+   */
   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.
+   *
+   * 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/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
@@ -21,23 +21,36 @@ struct LSRequestPrepareDatastoreResponse
   NullableDatastoreId datastoreId;
 };
 
 struct LSRequestPrepareObserverResponse
 {
   uint64_t observerId;
 };
 
+/**
+ * Discriminated union which can contain an error code (`nsresult`) or
+ * particular request response.
+ */
 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.
+ *
+ * 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
   // to the main thread in the content process at the time we are blocking
@@ -45,20 +58,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,47 @@
  * 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 which can contain an error code (`nsresult`) or
+ * particular simple request response.
+ */
 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
@@ -40,30 +43,72 @@ parent:
   async DeleteMe();
 
   async Checkpoint(LSWriteInfo[] writeInfos);
 
   async Finish();
 
   async Loaded();
 
+  /**
+   * Invoked on demand to load an item that didn't fit into the initial
+   * snapshot prefill.
+   *
+   * This needs to be synchronous because LocalStorage's semantics are
+   * synchronous.  Note that the Snapshot 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.
+   */
   sync LoadItem(nsString key)
     returns (nsString value);
 
+  /**
+   * Invoked on demand to load all keys in in their canonical order if they
+   * didn't fit into the initial snapshot prefill.
+   *
+   * This needs to be synchronous because LocalStorage's semantics are
+   * synchronous.  Note that the Snapshot 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.
+   */
   sync LoadKeys()
     returns (nsString[] keys);
 
+  /**
+   * This needs to be synchronous because LocalStorage's semantics are
+   * synchronous.  Note that the Snapshot in the PBackground parent typically
+   * doesn't need to consult any other threads or perform any I/O to handle
+   * this request.  However, it has to call a quota manager method that can
+   * potentially do I/O directly on the PBackground thread.  It can only happen
+   * rarely in a storage pressure (low storage space) situation.  Specifically,
+   * after we get a list of origin directories for eviction, we will delete
+   * them directly on the PBackground thread.  This doesn't cause any
+   * performance problems, but avoiding I/O completely might need to be done as
+   * a futher optimization.
+   */
   sync IncreasePeakUsage(int64_t requestedSize, int64_t minSize)
     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.
+   * The child LSSnapshot does that either immediately if it's just waiting
+   * to be reused or when it gets into a stable state.
+   *
+   * This message is expected to be sent in the following two cases only:
+   *   1. The state of the underlying Datastore starts to differ from the state
+   *      captured at the time of snapshot creation.
+   *   2. 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.
+   *
+   * This method will not create a QuotaManager-managed directory on disk if
+   * one does not already exist for the principal.
+   */
   [implicit_jscontext] nsISupports
   preload(in nsIPrincipal aPrincipal);
 
   [implicit_jscontext] nsISupports
   isPreloaded(in nsIPrincipal aPrincipal);
 };
--- a/dom/storage/Storage.h
+++ b/dom/storage/Storage.h
@@ -105,37 +105,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/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,22 +29,45 @@ 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();
 
+  /**
+   * Ensures the database has been opened and initiates an explicit snapshot.
+   * Snapshots are normally automatically ended and checkpointed back to the
+   * parent, but explicitly opened snapshots must be explicitly ended via
+   * `endExplicitSnapshot` or `close`.
+   */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void beginExplicitSnapshot();
 
+  /**
+   * Ends the explicitly begun snapshot and retains the underlying database.
+   * Compare with `close` which also drops the reference to the database.
+   */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
   void endExplicitSnapshot();
 };
--- a/ipc/glue/PBackground.ipdl
+++ b/ipc/glue/PBackground.ipdl
@@ -129,18 +129,29 @@ parent:
   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);
 
--- a/ipc/ipdl/sync-messages.ini
+++ b/ipc/ipdl/sync-messages.ini
@@ -918,23 +918,23 @@ description =
 description = See Bug 1505976 - investigate changing to async instead of matching GPU pattern
 [PVideoDecoderManager::PVideoDecoder]
 description =
 [PVideoDecoderManager::Readback]
 description =
 [PBackgroundStorage::Preload]
 description =
 [PBackgroundLSDatabase::PBackgroundLSSnapshot]
-description =
+description = See corresponding comment in PBackgroundLSDatabase.ipdl
 [PBackgroundLSSnapshot::LoadItem]
-description =
+description = See corresponding comment in PBackgroundLSSnapshot.ipdl
 [PBackgroundLSSnapshot::LoadKeys]
-description =
+description = See corresponding comment in PBackgroundLSSnapshot.ipdl
 [PBackgroundLSSnapshot::IncreasePeakUsage]
-description =
+description = See corresponding comment in PBackgroundLSSnapshot.ipdl
 [PBackgroundLSSnapshot::Ping]
 description = See corresponding comment in PBackgroundLSSnapshot.ipdl
 [PRemoteSpellcheckEngine::Check]
 description =
 [PRemoteSpellcheckEngine::CheckAndSuggest]
 description =
 [PRemoteSpellcheckEngine::SetDictionary]
 description =