Bug 1416879 - Part 4: FetchDriver needs to propagate write failures. r=baku
☠☠ backed out by ed0571d18d1f ☠ ☠
authorAndrew Sutherland <asutherland@asutherland.org>
Thu, 04 Jan 2018 18:04:55 -0500
changeset 449854 8e4fd74e7f5e69df7363bdb560f79dde347ce082
parent 449853 5453b8a58f0c2c28dc7c407c531c266972bff423
child 449855 994dc643a2ab62f03fef780a58971b476a4b6f4a
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1416879
milestone59.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 1416879 - Part 4: FetchDriver needs to propagate write failures. r=baku In the scenario where a ServiceWorker returns a pass-through fetch via `evt.respondWith(fetch("underlying"))`, in order for the "underlying" HTTP channel to be canceled when the outer HTTP channel is canceled, FetchDriver's OnDataAvailable method needs to return an error when the output pipe experiences an error. Unfortunately, the contract for ReadSegments is effectively that it returns NS_OK regardless of what the rv of the write handler returned, so relying on the returned rv is insufficient. And because various Write*() methods will all fast-path to returning NS_OK if a count of 0 is passed, it's necessary to infer a closed/broken pipe by noticing that we tried to write more than 0 bytes of data but 0 bytes were written. (This is safe because the pipe we write into was created by FetchDriver::OnStartRequest which explicitly creates an infinite pipe, so it's not possible for the write to fail due to lack of space in the pipe.)
dom/fetch/FetchDriver.cpp
--- a/dom/fetch/FetchDriver.cpp
+++ b/dom/fetch/FetchDriver.cpp
@@ -1082,35 +1082,47 @@ FetchDriver::OnDataAvailable(nsIRequest*
           mMainThreadEventTarget->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);
         if (NS_WARN_IF(NS_FAILED(rv))) {
           return rv;
         }
       }
     }
   }
 
-  uint32_t aRead;
+  // Needs to be initialized to 0 because in some cases nsStringInputStream may
+  // not write to aRead.
+  uint32_t aRead = 0;
   MOZ_ASSERT(mResponse);
   MOZ_ASSERT(mPipeOutputStream);
 
   // From "Main Fetch" step 19: SRI-part2.
   // Note: Avoid checking the hidden opaque body.
+  nsresult rv;
   if (mResponse->Type() != ResponseType::Opaque &&
       ShouldCheckSRI(mRequest, mResponse)) {
     MOZ_ASSERT(mSRIDataVerifier);
 
     SRIVerifierAndOutputHolder holder(mSRIDataVerifier, mPipeOutputStream);
-    nsresult rv = aInputStream->ReadSegments(CopySegmentToStreamAndSRI,
-                                             &holder, aCount, &aRead);
-    return rv;
+    rv = aInputStream->ReadSegments(CopySegmentToStreamAndSRI,
+                                    &holder, aCount, &aRead);
+  } else {
+    rv = aInputStream->ReadSegments(NS_CopySegmentToStream,
+                                    mPipeOutputStream,
+                                    aCount, &aRead);
   }
 
-  nsresult rv = aInputStream->ReadSegments(NS_CopySegmentToStream,
-                                           mPipeOutputStream,
-                                           aCount, &aRead);
+  // If no data was read, it's possible the output stream is closed but the
+  // ReadSegments call followed its contract of returning NS_OK despite write
+  // errors.  Unfortunately, nsIOutputStream has an ill-conceived contract when
+  // taken together with ReadSegments' contract, because the pipe will just
+  // NS_OK if we try and invoke its Write* functions ourselves with a 0 count.
+  // So we must just assume the pipe is broken.
+  if (aRead == 0 && aCount != 0) {
+    return NS_BASE_STREAM_CLOSED;
+  }
   return rv;
 }
 
 NS_IMETHODIMP
 FetchDriver::OnStopRequest(nsIRequest* aRequest,
                            nsISupports* aContext,
                            nsresult aStatusCode)
 {