Bug 1393063 - Part 1: Fix IPCStreamSource's handling of async streams returning 0 from Available. r=bkelly, a=lizzard
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 13 Jun 2017 16:16:55 -0400
changeset 655635 6a38cf9003c60bc5158627e2048b6a4f2dab8683
parent 655634 b185b492e4eedf396ce4ca68bbfce04523025046
child 655636 02852b5a5b3c6b13029da12b9da5060f88ee2222
push id76942
push userbmo:scwwu@mozilla.com
push dateWed, 30 Aug 2017 08:27:15 +0000
reviewersbkelly, lizzard
bugs1393063, 1371699
milestone56.0
Bug 1393063 - Part 1: Fix IPCStreamSource's handling of async streams returning 0 from Available. r=bkelly, a=lizzard This aligns the code more closely with how the input stream pump works: 0 available bytes when the stream itself told us it's ready means the stream is at the end. Note from asuth: This fix was originally part of bug 1371699, it was part 5. However, its initial landing was backed out, but it is now very much needed for bug 1393063, so I'm landing it.
ipc/glue/IPCStreamSource.cpp
ipc/glue/IPCStreamSource.h
--- a/ipc/glue/IPCStreamSource.cpp
+++ b/ipc/glue/IPCStreamSource.cpp
@@ -185,41 +185,50 @@ IPCStreamSource::ActorDestroyed()
     mWorkerPrivate = nullptr;
   }
 }
 
 void
 IPCStreamSource::Start()
 {
   NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
-  DoRead();
+  DoRead(ReadReason::Starting);
 }
 
 void
 IPCStreamSource::StartDestroy()
 {
   NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
   OnEnd(NS_ERROR_ABORT);
 }
 
 void
-IPCStreamSource::DoRead()
+IPCStreamSource::DoRead(ReadReason aReadReason)
 {
   NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
   MOZ_ASSERT(mState == eActorConstructed);
   MOZ_ASSERT(!mCallback);
 
   // The input stream (likely a pipe) probably uses a segment size of
   // 4kb.  If there is data already buffered it would be nice to aggregate
   // multiple segments into a single IPC call.  Conversely, don't send too
   // too large of a buffer in a single call to avoid spiking memory.
   static const uint64_t kMaxBytesPerMessage = 32 * 1024;
   static_assert(kMaxBytesPerMessage <= static_cast<uint64_t>(UINT32_MAX),
                 "kMaxBytesPerMessage must cleanly cast to uint32_t");
 
+  // Per the nsIInputStream API, having 0 bytes available in a stream is
+  // ambiguous.  It could mean "no data yet, but some coming later" or it could
+  // mean "EOF" (end of stream).
+  //
+  // If we're just starting up, we can't distinguish between those two options.
+  // But if we're being notified that an async stream is ready, then the
+  // first Available() call returning 0 should really mean end of stream.
+  // Subsequent Available() calls, after we read some data, could mean anything.
+  bool noDataMeansEOF = (aReadReason == ReadReason::Notified);
   while (true) {
     // It should not be possible to transition to closed state without
     // this loop terminating via a return.
     MOZ_ASSERT(mState == eActorConstructed);
 
     // Use non-auto here as we're unlikely to hit stack storage with the
     // sizes we are sending.  Also, it would be nice to avoid another copy
     // to the IPC layer which we avoid if we use COW strings.  Unfortunately
@@ -229,30 +238,37 @@ IPCStreamSource::DoRead()
     uint64_t available = 0;
     nsresult rv = mStream->Available(&available);
     if (NS_FAILED(rv)) {
       OnEnd(rv);
       return;
     }
 
     if (available == 0) {
-      Wait();
+      if (noDataMeansEOF) {
+        OnEnd(rv);
+      } else {
+        Wait();
+      }
       return;
     }
 
     uint32_t expectedBytes =
       static_cast<uint32_t>(std::min(available, kMaxBytesPerMessage));
 
     buffer.SetLength(expectedBytes);
 
     uint32_t bytesRead = 0;
     rv = mStream->Read(buffer.BeginWriting(), buffer.Length(), &bytesRead);
     MOZ_ASSERT_IF(NS_FAILED(rv), bytesRead == 0);
     buffer.SetLength(bytesRead);
 
+    // After this point, having no data tells us nothing about EOF.
+    noDataMeansEOF = false;
+
     // If we read any data from the stream, send it across.
     if (!buffer.IsEmpty()) {
       SendData(buffer);
     }
 
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
       Wait();
       return;
@@ -286,17 +302,17 @@ IPCStreamSource::Wait()
 void
 IPCStreamSource::OnStreamReady(Callback* aCallback)
 {
   NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
   MOZ_ASSERT(mCallback);
   MOZ_ASSERT(aCallback == mCallback);
   mCallback->ClearSource();
   mCallback = nullptr;
-  DoRead();
+  DoRead(ReadReason::Notified);
 }
 
 void
 IPCStreamSource::OnEnd(nsresult aRv)
 {
   NS_ASSERT_OWNINGTHREAD(IPCStreamSource);
   MOZ_ASSERT(aRv != NS_BASE_STREAM_WOULD_BLOCK);
 
--- a/ipc/glue/IPCStreamSource.h
+++ b/ipc/glue/IPCStreamSource.h
@@ -116,17 +116,21 @@ protected:
 
 private:
   class Callback;
 
   // WorkerHolder methods
   virtual bool
   Notify(dom::workers::Status aStatus) override;
 
-  void DoRead();
+  enum class ReadReason {
+    Starting, // We're trying to read because we just started off.
+    Notified  // We're trying to read because the streams said it's ready.
+  };
+  void DoRead(ReadReason aReadReason);
 
   void Wait();
 
   void OnStreamReady(Callback* aCallback);
 
   nsCOMPtr<nsIAsyncInputStream> mStream;
   RefPtr<Callback> mCallback;