Substantial clarity update based on code review feedback

Thanks to @jasonmalinowski and @CyrusNajmabadi for the detailed reviews!

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs

@@ -44,17 +44,19 @@ internal sealed class MemoryMappedInfo : IDisposable
             private readonly ReferenceCountedDisposable<MemoryMappedFile> _memoryMappedFile;
 
             /// <summary>
-            /// A weak reference to the actual memory accessor that owns the VM
+            /// A weak reference to a read-only view for the memory mapped file.
             /// </summary>
             /// <remarks>
             /// <para>This holds a weak counted reference to current <see cref="MemoryMappedViewAccessor"/>, which
             /// allows additional accessors for the same address space to be obtained up until the point when no
             /// external code is using it. When the memory is no longer being used by any
-            /// <see cref="SharedReadableStream"/> objects, the view of the memory mapped file is automatically
-            /// unmapped, making the process address space it previously claimed available for other purposes. If/when
-            /// it is needed again, a new view is created.</para>
+            /// <see cref="SharedReadableStream"/> objects, the view of the memory mapped file is unmapped, making the
+            /// process address space it previously claimed available for other purposes. If/when it is needed again, a
+            /// new view is created.</para>
+            ///
+            /// <para>This view is read-only, so it is only used by <see cref="CreateReadableStream"/>.</para>
             /// </remarks>
-            private ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference _weakAccessor;
+            private ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference _weakReadAccessor;
 
             public MemoryMappedInfo(ReferenceCountedDisposable<MemoryMappedFile> memoryMappedFile, string name, long offset, long size)
             {
@@ -108,12 +110,12 @@ public Stream CreateReadableStream()
                     // Note: TryAddReference behaves according to its documentation even if the target object has been
                     // disposed. If it returns non-null, then the object will not be disposed before the returned
                     // reference is disposed (see comments on _memoryMappedFile and TryAddReference).
-                    var streamAccessor = _weakAccessor.TryAddReference();
+                    var streamAccessor = _weakReadAccessor.TryAddReference();
                     if (streamAccessor == null)
                     {
                         var rawAccessor = RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewAccessor(info.Offset, info.Size, MemoryMappedFileAccess.Read), this);
                         streamAccessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>(rawAccessor);
-                        _weakAccessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference(streamAccessor);
+                        _weakReadAccessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference(streamAccessor);
                     }
 
                     Contract.Assert(streamAccessor.Target.CanRead);
@@ -175,8 +177,7 @@ private void Dispose(bool disposing)
                 if (disposing)
                 {
                     // See remarks on field for relation between _memoryMappedFile and the views/streams. There is no
-                    // need to write _weakAccessor here since the types involved adhere to their contracts even in
-                    // concurrent code.
+                    // need to write _weakReadAccessor here since lifetime of the target is not owned by this instance.
                     _memoryMappedFile.Dispose();
                 }
             }

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs

@@ -18,12 +18,39 @@ internal partial class TemporaryStorageServiceFactory
         /// disposed.
         /// </summary>
         /// <remarks>
+        /// <para>Each instance of <see cref="ReferenceCountedDisposable{T}"/> represents a counted reference (also
+        /// referred to as a <em>reference</em> in the following documentation) to a target object. Each of these
+        /// references has a lifetime, starting when it is constructed and continuing through its release. During
+        /// this time, the reference is considered <em>alive</em>. Each reference which is alive owns exactly one
+        /// reference to the target object, ensuring that it will not be disposed while still in use. A reference is
+        /// released through either of the following actions:</para>
+        ///
+        /// <list type="bullet">
+        /// <item>The reference is explicitly released by a call to <see cref="Dispose"/>.</item>
+        /// <item>The reference is no longer in use by managed code and gets reclaimed by the garbage collector.</item>
+        /// </list>
+        ///
         /// <para>While each instance of <see cref="ReferenceCountedDisposable{T}"/> should be explicitly disposed when
         /// the object is no longer needed by the code owning the reference, this implementation will not leak resources
-        /// in the event one or more callers fail to do so. The underlying object will be deterministically released
-        /// when the last reference to it is disposed. However, the underlying object is eligible for non-deterministic
-        /// release (should it have a finalizer) when each reference to it is <em>either</em> disposed <em>or</em>
-        /// eligible for garbage collection itself.</para>
+        /// in the event one or more callers fail to do so. When all references to an object are explicitly released
+        /// (i.e. by calling <see cref="Dispose"/>), the target object will itself be deterministically released by a
+        /// call to <see cref="IDisposable.Dispose"/> when the last reference to it is released. However, in the event
+        /// one or more references is not explicitly released, the underlying object will still become eligible for
+        /// non-deterministic release (i.e. finalization) as soon as each reference to it is released by one of the
+        /// two actions described previously.</para>
+        ///
+        /// <para>When using <see cref="ReferenceCountedDisposable{T}"/>, certain steps must be taken to ensure the
+        /// target object is not disposed early.</para>
+        ///
+        /// <list type="number">
+        /// <para>Use <see cref="ReferenceCountedDisposable{T}"/> consistently. In other words, do not mix code using
+        /// reference-counted wrappers with code that references to the target directly.</para>
+        /// <para>Only use the <see cref="ReferenceCountedDisposable{T}(T)"/> constructor one time per target object.
+        /// Additional references to the same target object must only be obtained by calling
+        /// <see cref="TryAddReference"/>.</para>
+        /// <para>Do not call <see cref="IDisposable.Dispose"/> on the target object directly. It will be called
+        /// automatically at the appropriate time, as described above.</para>
+        /// </list>
         ///
         /// <para>All public methods on this type adhere to their pre- and post-conditions and will not invalidate state
         /// even in concurrent execution. The implementation of <see cref="TryAddReference"/> is lock-free; all other
@@ -226,7 +253,9 @@ public WeakReference(ReferenceCountedDisposable<T> reference)
                     var (instance, referenceCount) = reference.AtomicReadState();
                     if (referenceCount == null)
                     {
-                        // The specified reference is already not valid.
+                        // The specified reference is already not valid. This case is supported by WeakReference (not
+                        // unlike `new System.WeakReference(null)`), but we return early to avoid an unnecessary
+                        // allocation in this case.
                         return;
                     }
 

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs

@@ -39,6 +39,7 @@ internal class TemporaryStorageService : ITemporaryStorageService2
             /// <para>This value was arbitrarily chosen and appears to work well. Can be changed if data suggests
             /// something better.</para>
             /// </remarks>
+            /// <seealso cref="_storage"/>
             private const long SingleFileThreshold = 128 * 1024;
 
             /// <summary>
@@ -48,6 +49,7 @@ internal class TemporaryStorageService : ITemporaryStorageService2
             /// <para>This value was arbitrarily chosen and appears to work well. Can be changed if data suggests
             /// something better.</para>
             /// </remarks>
+            /// <seealso cref="_storage"/>
             private const long MultiFileBlockSize = SingleFileThreshold * 32;
 
             private readonly ITextFactoryService _textFactory;
@@ -99,7 +101,7 @@ private MemoryMappedInfo CreateTemporaryStorage(long size)
                     // Larger blocks are allocated separately
                     var mapName = CreateUniqueName(size);
                     var storage = MemoryMappedFile.CreateNew(mapName, size);
-                    return new MemoryMappedInfo(new ReferenceCountedDisposable<MemoryMappedFile>(storage), mapName, 0, size);
+                    return new MemoryMappedInfo(new ReferenceCountedDisposable<MemoryMappedFile>(storage), mapName, offset: 0, size: size);
                 }
 
                 while (true)