Bug 983187 - Test that downloads fail when an RST packet is received. r=mayhemer
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 09 Jan 2019 11:48:23 +0000
changeset 510194 30d829df41ed4a405572e4cc4634e6ac433a2468
parent 510193 785c68522094596c62e08d0f465633ef2b8f3892
child 510195 0fe293d9f49475448cf3d1a58d0fa63d0d22bc9f
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs983187
milestone66.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 983187 - Test that downloads fail when an RST packet is received. r=mayhemer This adds a way to simulate failed network connections, allowing the addition of test coverage that would otherwise not be available. This is used in the Downloads tests to ensure that failures at the network level are handled correctly. Differential Revision: https://phabricator.services.mozilla.com/D15522
media/mtransport/test/webrtcproxychannel_unittest.cpp
netwerk/base/nsISocketTransport.idl
netwerk/base/nsSocketTransport2.cpp
netwerk/base/nsSocketTransport2.h
netwerk/protocol/http/TunnelUtils.cpp
netwerk/test/httpserver/httpd.js
toolkit/components/downloads/test/unit/common_test_Download.js
toolkit/components/downloads/test/unit/head.js
toolkit/components/downloads/test/unit/test_DownloadPaths.js
--- a/media/mtransport/test/webrtcproxychannel_unittest.cpp
+++ b/media/mtransport/test/webrtcproxychannel_unittest.cpp
@@ -106,16 +106,20 @@ class FakeSocketTransportProvider : publ
   NS_IMETHOD GetTimeout(uint32_t aType, uint32_t *_retval) override {
     MOZ_ASSERT(false);
     return NS_OK;
   }
   NS_IMETHOD SetTimeout(uint32_t aType, uint32_t aValue) override {
     MOZ_ASSERT(false);
     return NS_OK;
   }
+  NS_IMETHOD SetLinger(bool aPolarity, int16_t aTimeout) override {
+    MOZ_ASSERT(false);
+    return NS_OK;
+  }
   NS_IMETHOD SetReuseAddrPort(bool reuseAddrPort) override {
     MOZ_ASSERT(false);
     return NS_OK;
   }
   NS_IMETHOD GetConnectionFlags(uint32_t *aConnectionFlags) override {
     MOZ_ASSERT(false);
     return NS_OK;
   }
--- a/netwerk/base/nsISocketTransport.idl
+++ b/netwerk/base/nsISocketTransport.idl
@@ -120,16 +120,23 @@ interface nsISocketTransport : nsITransp
      * Socket timeouts in seconds.  To specify no timeout, pass UINT32_MAX
      * as aValue to setTimeout.  The implementation may truncate timeout values
      * to a smaller range of values (e.g., 0 to 0xFFFF).
      */
     unsigned long getTimeout(in unsigned long aType);
     void          setTimeout(in unsigned long aType, in unsigned long aValue);
 
     /**
+     * Sets the SO_LINGER option with the specified values for the l_onoff and
+     * l_linger parameters. This applies PR_SockOpt_Linger before PR_Close and
+     * can be used with a timeout of zero to send an RST packet when closing.
+     */
+    void setLinger(in boolean aPolarity, in short aTimeout);
+
+    /**
      * True to set addr and port reuse socket options.
      */
     void setReuseAddrPort(in bool reuseAddrPort);
 
     /**
      * Values for the aType parameter passed to get/setTimeout.
      */
     const unsigned long TIMEOUT_CONNECT    = 0;
--- a/netwerk/base/nsSocketTransport2.cpp
+++ b/netwerk/base/nsSocketTransport2.cpp
@@ -712,16 +712,18 @@ nsSocketTransport::nsSocketTransport()
       mLock("nsSocketTransport.mLock"),
       mFD(this),
       mFDref(0),
       mFDconnected(false),
       mFDFastOpenInProgress(false),
       mSocketTransportService(gSocketTransportService),
       mInput(this),
       mOutput(this),
+      mLingerPolarity(false),
+      mLingerTimeout(0),
       mQoSBits(0x00),
       mKeepaliveEnabled(false),
       mKeepaliveIdleTimeS(-1),
       mKeepaliveRetryIntervalS(-1),
       mKeepaliveProbeCount(-1),
       mFastOpenCallback(nullptr),
       mFastOpenLayerHasBufferedData(false),
       mFastOpenStatus(TFO_NOT_SET),
@@ -1973,17 +1975,18 @@ class ThunkPRClose : public Runnable {
         mFD, gSocketTransportService->IsTelemetryEnabledAndNotSleepPhase());
     return NS_OK;
   }
 
  private:
   PRFileDesc *mFD;
 };
 
-void STS_PRCloseOnSocketTransport(PRFileDesc *fd) {
+void STS_PRCloseOnSocketTransport(PRFileDesc *fd, bool lingerPolarity,
+                                  int16_t lingerTimeout) {
   if (gSocketTransportService) {
     // Can't PR_Close() a socket off STS thread. Thunk it to STS to die
     gSocketTransportService->Dispatch(new ThunkPRClose(fd), NS_DISPATCH_NORMAL);
   } else {
     // something horrible has happened
     NS_ASSERTION(gSocketTransportService, "No STS service");
   }
 }
@@ -1994,23 +1997,32 @@ void nsSocketTransport::ReleaseFD_Locked
   NS_ASSERTION(mFD == fd, "wrong fd");
 
   if (--mFDref == 0) {
     if (gIOService->IsNetTearingDown() &&
         ((PR_IntervalNow() - gIOService->NetTearingDownStarted()) >
          gSocketTransportService->MaxTimeForPrClosePref())) {
       // If shutdown last to long, let the socket leak and do not close it.
       SOCKET_LOG(("Intentional leak"));
-    } else if (OnSocketThread()) {
-      SOCKET_LOG(("nsSocketTransport: calling PR_Close [this=%p]\n", this));
-      CloseSocket(
-          mFD, mSocketTransportService->IsTelemetryEnabledAndNotSleepPhase());
     } else {
-      // Can't PR_Close() a socket off STS thread. Thunk it to STS to die
-      STS_PRCloseOnSocketTransport(mFD);
+      if (mLingerPolarity || mLingerTimeout) {
+        PRSocketOptionData socket_linger;
+        socket_linger.option = PR_SockOpt_Linger;
+        socket_linger.value.linger.polarity = mLingerPolarity;
+        socket_linger.value.linger.linger = mLingerTimeout;
+        PR_SetSocketOption(mFD, &socket_linger);
+      }
+      if (OnSocketThread()) {
+        SOCKET_LOG(("nsSocketTransport: calling PR_Close [this=%p]\n", this));
+        CloseSocket(
+            mFD, mSocketTransportService->IsTelemetryEnabledAndNotSleepPhase());
+      } else {
+        // Can't PR_Close() a socket off STS thread. Thunk it to STS to die
+        STS_PRCloseOnSocketTransport(mFD, mLingerPolarity, mLingerTimeout);
+      }
     }
     mFD = nullptr;
   }
 }
 
 //-----------------------------------------------------------------------------
 // socket event handler impl
 
@@ -2752,16 +2764,26 @@ nsSocketTransport::SetTimeout(uint32_t t
 
 NS_IMETHODIMP
 nsSocketTransport::SetReuseAddrPort(bool reuseAddrPort) {
   mReuseAddrPort = reuseAddrPort;
   return NS_OK;
 }
 
 NS_IMETHODIMP
+nsSocketTransport::SetLinger(bool aPolarity, int16_t aTimeout) {
+  MutexAutoLock lock(mLock);
+
+  mLingerPolarity = aPolarity;
+  mLingerTimeout = aTimeout;
+
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 nsSocketTransport::SetQoSBits(uint8_t aQoSBits) {
   // Don't do any checking here of bits.  Why?  Because as of RFC-4594
   // several different Class Selector and Assured Forwarding values
   // have been defined, but that isn't to say more won't be added later.
   // In that case, any checking would be an impediment to interoperating
   // with newer QoS definitions.
 
   mQoSBits = aQoSBits;
--- a/netwerk/base/nsSocketTransport2.h
+++ b/netwerk/base/nsSocketTransport2.h
@@ -393,16 +393,20 @@ class nsSocketTransport final : public n
   nsSocketOutputStream mOutput;
 
   friend class nsSocketInputStream;
   friend class nsSocketOutputStream;
 
   // socket timeouts are not protected by any lock.
   uint16_t mTimeouts[2];
 
+  // linger options to use when closing
+  bool mLingerPolarity;
+  int16_t mLingerTimeout;
+
   // QoS setting for socket
   uint8_t mQoSBits;
 
   //
   // mFD access methods: called with mLock held.
   //
   PRFileDesc *GetFD_Locked();
   PRFileDesc *GetFD_LockedAlsoDuringFastOpen();
--- a/netwerk/protocol/http/TunnelUtils.cpp
+++ b/netwerk/protocol/http/TunnelUtils.cpp
@@ -1885,16 +1885,21 @@ SocketTransportShim::SetTimeout(uint32_t
 }
 
 NS_IMETHODIMP
 SocketTransportShim::SetReuseAddrPort(bool aReuseAddrPort) {
   return mWrapped->SetReuseAddrPort(aReuseAddrPort);
 }
 
 NS_IMETHODIMP
+SocketTransportShim::SetLinger(bool aPolarity, int16_t aTimeout) {
+  return mWrapped->SetLinger(aPolarity, aTimeout);
+}
+
+NS_IMETHODIMP
 SocketTransportShim::GetQoSBits(uint8_t *aQoSBits) {
   return mWrapped->GetQoSBits(aQoSBits);
 }
 
 NS_IMETHODIMP
 SocketTransportShim::SetQoSBits(uint8_t aQoSBits) {
   return mWrapped->SetQoSBits(aQoSBits);
 }
--- a/netwerk/test/httpserver/httpd.js
+++ b/netwerk/test/httpserver/httpd.js
@@ -393,17 +393,17 @@ nsHttpServer.prototype =
       trans.close(Cr.NS_BINDING_ABORTED);
       return;
     }
 
     var connectionNumber = ++this._connectionGen;
 
     try {
       var conn = new Connection(input, output, this, socket.port, trans.port,
-                                connectionNumber);
+                                connectionNumber, trans);
       var reader = new RequestReader(conn);
 
       // XXX add request timeout functionality here!
 
       // Note: must use main thread here, or we might get a GC that will cause
       //       threadsafety assertions.  We really need to fix XPConnect so that
       //       you can actually do things in multi-threaded JS.  :-(
       input.asyncWait(reader, 0, 0, gThreadManager.mainThread);
@@ -1062,17 +1062,18 @@ ServerIdentity.prototype =
  *   the server handling the connection
  * @param port : int
  *   the port on which the server is running
  * @param outgoingPort : int
  *   the outgoing port used by this connection
  * @param number : uint
  *   a serial number used to uniquely identify this connection
  */
-function Connection(input, output, server, port, outgoingPort, number) {
+function Connection(input, output, server, port, outgoingPort, number,
+                    transport) {
   dumpn("*** opening new connection " + number + " on port " + outgoingPort);
 
   /** Stream of incoming data. */
   this.input = input;
 
   /** Stream for outgoing data. */
   this.output = output;
 
@@ -1083,16 +1084,19 @@ function Connection(input, output, serve
   this.port = port;
 
   /** The outgoing poort used by this connection. */
   this._outgoingPort = outgoingPort;
 
   /** The serial number of this connection. */
   this.number = number;
 
+  /** Reference to the underlying transport. */
+  this.transport = transport;
+
   /**
    * The request for which a response is being generated, null if the
    * incoming request has not been fully received or if it had errors.
    */
   this.request = null;
 
   /** This allows a connection to disambiguate between a peer initiating a
    *  close and the socket being forced closed on shutdown.
@@ -3554,20 +3558,29 @@ Response.prototype =
    * response) because data may already have been sent (or because the response
    * might be expected to have been generated asynchronously or completely from
    * scratch by the handler), we stop processing this response and abruptly
    * close the connection.
    *
    * @param e : Error
    *   the exception which precipitated this abort, or null if no such exception
    *   was generated
+   * @param truncateConnection : Boolean
+   *   ensures that we truncate the connection using an RST packet, so the
+   *   client testing code is aware that an error occurred, otherwise it may
+   *   consider the response as valid.
    */
-  abort(e) {
+  abort(e, truncateConnection = false) {
     dumpn("*** abort(<" + e + ">)");
 
+    if (truncateConnection) {
+      dumpn("*** truncate connection");
+      this._connection.transport.setLinger(true, 0);
+    }
+
     // This response will be ended by the processor if one was created.
     var copier = this._asyncCopier;
     if (copier) {
       // We dispatch asynchronously here so that any pending writes of data to
       // the connection will be deterministically written.  This makes it easier
       // to specify exact behavior, and it makes observable behavior more
       // predictable for clients.  Note that the correctness of this depends on
       // callbacks in response to _waitToReadData in WriteThroughCopier
--- a/toolkit/components/downloads/test/unit/common_test_Download.js
+++ b/toolkit/components/downloads/test/unit/common_test_Download.js
@@ -1447,16 +1447,61 @@ add_task(async function test_error_sourc
   Assert.equal(download.error.result, Cr.NS_ERROR_NET_PARTIAL_TRANSFER);
 
   Assert.equal(false, await OS.File.exists(download.target.path));
   Assert.ok(!download.target.exists);
   Assert.equal(download.target.size, 0);
 });
 
 /**
+ * Ensures a download error is reported when an RST packet is received.
+ */
+add_task(async function test_error_source_netreset() {
+  if (AppConstants.platform == "win") {
+    return;
+  }
+
+  let download;
+  try {
+    if (!gUseLegacySaver) {
+      // When testing DownloadCopySaver, we want to check that the promise
+      // returned by the "start" method is rejected.
+      download = await promiseNewDownload(httpUrl("netreset.txt"));
+
+      Assert.ok(download.error === null);
+
+      await download.start();
+    } else {
+      // When testing DownloadLegacySaver, we cannot be sure whether we are
+      // testing the promise returned by the "start" method or we are testing
+      // the "error" property checked by promiseDownloadStopped.  This happens
+      // because we don't have control over when the download is started.
+      download = await promiseStartLegacyDownload(httpUrl("netreset.txt"));
+      await promiseDownloadStopped(download);
+    }
+    do_throw("The download should have failed.");
+  } catch (ex) {
+    if (!(ex instanceof Downloads.Error) || !ex.becauseSourceFailed) {
+      throw ex;
+    }
+    // A specific error object is thrown when reading from the source fails.
+  }
+
+  // Check the properties now that the download stopped.
+  Assert.ok(download.stopped);
+  Assert.ok(!download.canceled);
+  Assert.ok(download.error !== null);
+  Assert.ok(download.error.becauseSourceFailed);
+  Assert.ok(!download.error.becauseTargetFailed);
+  Assert.equal(download.error.result, Cr.NS_ERROR_NET_RESET);
+
+  Assert.equal(false, await OS.File.exists(download.target.path));
+});
+
+/**
  * Ensures download error details are reported on local writing failures.
  */
 add_task(async function test_error_target() {
   // Create a file without write access permissions before downloading.
   let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
   targetFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0);
   try {
     let download;
--- a/toolkit/components/downloads/test/unit/head.js
+++ b/toolkit/components/downloads/test/unit/head.js
@@ -6,16 +6,17 @@
 /**
  * Provides infrastructure for automated download components tests.
  */
 
 "use strict";
 
 // Globals
 
+ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
 ChromeUtils.import("resource://gre/modules/Integration.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 ChromeUtils.defineModuleGetter(this, "DownloadPaths",
                                "resource://gre/modules/DownloadPaths.jsm");
 ChromeUtils.defineModuleGetter(this, "Downloads",
                                "resource://gre/modules/Downloads.jsm");
 ChromeUtils.defineModuleGetter(this, "FileUtils",
@@ -713,16 +714,30 @@ add_task(function test_common_initialize
 
   // This URL will emulate being blocked by Windows Parental controls
   gHttpServer.registerPathHandler("/parentalblocked.zip",
     function(aRequest, aResponse) {
       aResponse.setStatusLine(aRequest.httpVersion, 450,
                               "Blocked by Windows Parental Controls");
     });
 
+  // This URL sends some data followed by an RST packet
+  gHttpServer.registerPathHandler("/netreset.txt",
+    function(aRequest, aResponse) {
+      info("Starting response that will be aborted.");
+      aResponse.processAsync();
+      aResponse.setHeader("Content-Type", "text/plain", false);
+      aResponse.write(TEST_DATA_SHORT);
+      promiseExecuteSoon().then(() => {
+        aResponse.abort(null, true);
+        aResponse.finish();
+        info("Aborting response with network reset.");
+      }).then(null, Cu.reportError);
+    });
+
   // During unit tests, most of the functions that require profile access or
   // operating system features will be disabled. Individual tests may override
   // them again to check for specific behaviors.
   Integration.downloads.register(base => ({
     __proto__: base,
     loadPublicDownloadListFromStore: () => Promise.resolve(),
     shouldKeepBlockedData: () => Promise.resolve(false),
     shouldBlockForParentalControls: () => Promise.resolve(false),
--- a/toolkit/components/downloads/test/unit/test_DownloadPaths.js
+++ b/toolkit/components/downloads/test/unit/test_DownloadPaths.js
@@ -1,17 +1,15 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /**
  * Tests for the "DownloadPaths.jsm" JavaScript module.
  */
 
-ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
-
 function testSanitize(leafName, expectedLeafName) {
   Assert.equal(DownloadPaths.sanitize(leafName), expectedLeafName);
 }
 
 function testSplitBaseNameAndExtension(aLeafName, [aBase, aExt]) {
   var [base, ext] = DownloadPaths.splitBaseNameAndExtension(aLeafName);
   Assert.equal(base, aBase);
   Assert.equal(ext, aExt);