[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor-browser] 282/311: Bug 1752270 - Retry a request that failed with 408 if HTTP/2 is used. r=necko-reviewers, kershaw a=dmeehan



This is an automated email from the git hooks/post-receive script.

pierov pushed a commit to branch geckoview-99.0.1-11.0-1
in repository tor-browser.

commit dc9b23fa08e053087c1ddaf75254d86775568696
Author: Dragana Damjanovic <dd.mozilla@xxxxxxxxx>
AuthorDate: Tue Mar 22 21:12:21 2022 +0000

    Bug 1752270 - Retry a request that failed with 408 if HTTP/2 is used. r=necko-reviewers,kershaw a=dmeehan
    
    Differential Revision: https://phabricator.services.mozilla.com/D140827
---
 netwerk/protocol/http/ConnectionHandle.cpp  |  4 +++
 netwerk/protocol/http/Http2Session.cpp      |  7 ++--
 netwerk/protocol/http/Http3Session.cpp      |  2 ++
 netwerk/protocol/http/Http3Session.h        |  1 -
 netwerk/protocol/http/HttpConnectionBase.h  |  4 ++-
 netwerk/protocol/http/HttpConnectionUDP.cpp |  4 +++
 netwerk/protocol/http/nsAHttpConnection.h   |  4 ++-
 netwerk/protocol/http/nsHttpConnection.cpp  | 13 ++------
 netwerk/protocol/http/nsHttpTransaction.cpp | 21 ++++++++++++
 netwerk/test/unit/test_http_408_retry.js    | 51 ++++++++++++++++++++++-------
 10 files changed, 83 insertions(+), 28 deletions(-)

diff --git a/netwerk/protocol/http/ConnectionHandle.cpp b/netwerk/protocol/http/ConnectionHandle.cpp
index 20986e0d3cc8a..180d479492020 100644
--- a/netwerk/protocol/http/ConnectionHandle.cpp
+++ b/netwerk/protocol/http/ConnectionHandle.cpp
@@ -84,5 +84,9 @@ void ConnectionHandle::TopBrowsingContextIdChanged(uint64_t id) {
   // Do nothing.
 }
 
+PRIntervalTime ConnectionHandle::LastWriteTime() {
+  return mConn->LastWriteTime();
+}
+
 }  // namespace net
 }  // namespace mozilla
diff --git a/netwerk/protocol/http/Http2Session.cpp b/netwerk/protocol/http/Http2Session.cpp
index 78297e36385aa..fd748d21a0d16 100644
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -4430,8 +4430,7 @@ nsresult Http2Session::OnHeadersAvailable(nsAHttpTransaction* transaction,
                                           nsHttpRequestHead* requestHead,
                                           nsHttpResponseHead* responseHead,
                                           bool* reset) {
-  return mConnection->OnHeadersAvailable(transaction, requestHead, responseHead,
-                                         reset);
+  return NS_OK;
 }
 
 bool Http2Session::IsReused() {
@@ -4671,5 +4670,9 @@ bool Http2Session::CanAcceptWebsocket() {
          (mPeerAllowsWebsockets || !mProcessedWaitingWebsockets);
 }
 
+PRIntervalTime Http2Session::LastWriteTime() {
+  return mConnection->LastWriteTime();
+}
+
 }  // namespace net
 }  // namespace mozilla
diff --git a/netwerk/protocol/http/Http3Session.cpp b/netwerk/protocol/http/Http3Session.cpp
index 40f5c885930f0..c2174c8ebee3a 100644
--- a/netwerk/protocol/http/Http3Session.cpp
+++ b/netwerk/protocol/http/Http3Session.cpp
@@ -1896,5 +1896,7 @@ nsresult Http3Session::GetTransactionSecurityInfo(nsISupports** secinfo) {
   return NS_OK;
 }
 
+PRIntervalTime Http3Session::LastWriteTime() { return mLastWriteTime; }
+
 }  // namespace net
 }  // namespace mozilla
diff --git a/netwerk/protocol/http/Http3Session.h b/netwerk/protocol/http/Http3Session.h
index 9d98c515e9b17..b2265c3e1facf 100644
--- a/netwerk/protocol/http/Http3Session.h
+++ b/netwerk/protocol/http/Http3Session.h
@@ -95,7 +95,6 @@ class Http3Session final : public nsAHttpTransaction, public nsAHttpConnection {
 
   int64_t GetBytesWritten() { return mTotalBytesWritten; }
   int64_t BytesRead() { return mTotalBytesRead; }
-  PRIntervalTime LastWriteTime() { return mLastWriteTime; }
 
   nsresult SendData(nsIUDPSocket* socket);
   nsresult RecvData(nsIUDPSocket* socket);
diff --git a/netwerk/protocol/http/HttpConnectionBase.h b/netwerk/protocol/http/HttpConnectionBase.h
index 6235b7af5e92c..34dd394f5d8ad 100644
--- a/netwerk/protocol/http/HttpConnectionBase.h
+++ b/netwerk/protocol/http/HttpConnectionBase.h
@@ -136,6 +136,7 @@ class HttpConnectionBase : public nsSupportsWeakReference {
   virtual nsresult GetPeerAddr(NetAddr* addr) = 0;
   virtual bool ResolvedByTRR() = 0;
   virtual bool GetEchConfigUsed() = 0;
+  virtual PRIntervalTime LastWriteTime() = 0;
 
  protected:
   // The capabailities associated with the most recent transaction
@@ -189,7 +190,8 @@ NS_DEFINE_STATIC_IID_ACCESSOR(HttpConnectionBase, HTTPCONNECTIONBASE_IID)
   bool IsReused() override;                                                    \
   [[nodiscard]] nsresult PushBack(const char* data, uint32_t length) override; \
   void SetEvent(nsresult aStatus) override;                                    \
-  virtual nsAHttpTransaction* Transaction() override;
+  virtual nsAHttpTransaction* Transaction() override;                          \
+  PRIntervalTime LastWriteTime() override;
 
 }  // namespace net
 }  // namespace mozilla
diff --git a/netwerk/protocol/http/HttpConnectionUDP.cpp b/netwerk/protocol/http/HttpConnectionUDP.cpp
index 8fe5f7e9b0c3f..2a2155f9a8722 100644
--- a/netwerk/protocol/http/HttpConnectionUDP.cpp
+++ b/netwerk/protocol/http/HttpConnectionUDP.cpp
@@ -448,6 +448,10 @@ nsresult HttpConnectionUDP::ForceSend() {
 
 HttpVersion HttpConnectionUDP::Version() { return HttpVersion::v3_0; }
 
+PRIntervalTime HttpConnectionUDP::LastWriteTime() {
+  return mHttp3Session->LastWriteTime();
+}
+
 //-----------------------------------------------------------------------------
 // HttpConnectionUDP <private>
 //-----------------------------------------------------------------------------
diff --git a/netwerk/protocol/http/nsAHttpConnection.h b/netwerk/protocol/http/nsAHttpConnection.h
index f1e47c8baa64c..ed595105a4ac3 100644
--- a/netwerk/protocol/http/nsAHttpConnection.h
+++ b/netwerk/protocol/http/nsAHttpConnection.h
@@ -164,6 +164,7 @@ class nsAHttpConnection : public nsISupports {
   virtual nsresult GetPeerAddr(NetAddr* addr) = 0;
   virtual bool ResolvedByTRR() = 0;
   virtual bool GetEchConfigUsed() = 0;
+  virtual PRIntervalTime LastWriteTime() = 0;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsAHttpConnection, NS_AHTTPCONNECTION_IID)
@@ -257,7 +258,8 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsAHttpConnection, NS_AHTTPCONNECTION_IID)
   }                                                                          \
   bool GetEchConfigUsed() override {                                         \
     return (!(fwdObject)) ? false : (fwdObject)->GetEchConfigUsed();         \
-  }
+  }                                                                          \
+  PRIntervalTime LastWriteTime() override;
 
 // ThrottleResponse deliberately ommited since we want different implementation
 // for h1 and h2 connections.
diff --git a/netwerk/protocol/http/nsHttpConnection.cpp b/netwerk/protocol/http/nsHttpConnection.cpp
index 8b01fad1a12bf..cb879aefe87ae 100644
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -1134,18 +1134,7 @@ nsresult nsHttpConnection::OnHeadersAvailable(nsAHttpTransaction* trans,
 
   // deal with 408 Server Timeouts
   uint16_t responseStatus = responseHead->Status();
-  static const PRIntervalTime k1000ms = PR_MillisecondsToInterval(1000);
   if (responseStatus == 408) {
-    // If this error could be due to a persistent connection reuse then
-    // we pass an error code of NS_ERROR_NET_RESET to
-    // trigger the transaction 'restart' mechanism.  We tell it to reset its
-    // response headers so that it will be ready to receive the new response.
-    if (mIsReused && ((PR_IntervalNow() - mLastWriteTime) < k1000ms)) {
-      Close(NS_ERROR_NET_RESET);
-      *reset = true;
-      return NS_OK;
-    }
-
     // timeouts that are not caused by persistent connection reuse should
     // not be retried for browser compatibility reasons. bug 907800. The
     // server driven close is implicit in the 408.
@@ -1621,6 +1610,8 @@ HttpVersion nsHttpConnection::Version() {
   return mLastHttpResponseVersion;
 }
 
+PRIntervalTime nsHttpConnection::LastWriteTime() { return mLastWriteTime; }
+
 //-----------------------------------------------------------------------------
 // nsHttpConnection <private>
 //-----------------------------------------------------------------------------
diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp
index 1fc862a704ec2..3fc80715143c5 100644
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -2169,6 +2169,27 @@ nsresult nsHttpTransaction::HandleContentStart() {
         mNoContent = true;
         LOG(("this response should not contain a body.\n"));
         break;
+      case 408:
+        LOG(("408 Server Timeouts"));
+
+        if (mConnection->Version() >= HttpVersion::v2_0) {
+          mForceRestart = true;
+          return NS_ERROR_NET_RESET;
+        }
+
+        // If this error could be due to a persistent connection
+        // reuse then we pass an error code of NS_ERROR_NET_RESET
+        // to trigger the transaction 'restart' mechanism.  We
+        // tell it to reset its response headers so that it will
+        // be ready to receive the new response.
+        LOG(("408 Server Timeouts now=%d lastWrite=%d", PR_IntervalNow(),
+             mConnection->LastWriteTime()));
+        if ((PR_IntervalNow() - mConnection->LastWriteTime()) >=
+            PR_MillisecondsToInterval(1000)) {
+          mForceRestart = true;
+          return NS_ERROR_NET_RESET;
+        }
+        break;
       case 421:
         LOG(("Misdirected Request.\n"));
         gHttpHandler->ClearHostMapping(mConnInfo);
diff --git a/netwerk/test/unit/test_http_408_retry.js b/netwerk/test/unit/test_http_408_retry.js
index 421d3a52665e9..872107ed85391 100644
--- a/netwerk/test/unit/test_http_408_retry.js
+++ b/netwerk/test/unit/test_http_408_retry.js
@@ -27,17 +27,41 @@ add_task(async function test() {
   async function check408retry(server) {
     info(`Testing ${server.constructor.name}`);
     await server.execute(`global.server_name = "${server.constructor.name}";`);
-    await server.registerPathHandler("/test", (req, resp) => {
-      let oldSock = global.socket;
-      global.socket = resp.socket;
-      if (global.socket == oldSock) {
-        resp.writeHead(408);
-        resp.end("stuff");
-        return;
-      }
-      resp.writeHead(200);
-      resp.end(global.server_name);
-    });
+    if (
+      server.constructor.name == "NodeHTTPServer" ||
+      server.constructor.name == "NodeHTTPSServer"
+    ) {
+      await server.registerPathHandler("/test", (req, resp) => {
+        let oldSock = global.socket;
+        global.socket = resp.socket;
+        if (global.socket == oldSock) {
+          setTimeout(
+            arg => {
+              arg.writeHead(408);
+              arg.end("stuff");
+            },
+            1100,
+            resp
+          );
+          return;
+        }
+        resp.writeHead(200);
+        resp.end(global.server_name);
+      });
+    } else {
+      await server.registerPathHandler("/test", (req, resp) => {
+        let oldSock = global.socket;
+        global.socket = resp.socket;
+        if (!global.sent408) {
+          global.sent408 = true;
+          resp.writeHead(408);
+          resp.end("stuff");
+          return;
+        }
+        resp.writeHead(200);
+        resp.end(global.server_name);
+      });
+    }
 
     async function load() {
       let { req, buff } = await loadURL(
@@ -59,5 +83,8 @@ add_task(async function test() {
     await load();
   }
 
-  await with_node_servers([NodeHTTPServer, NodeHTTPSServer], check408retry);
+  await with_node_servers(
+    [NodeHTTPServer, NodeHTTPSServer, NodeHTTP2Server],
+    check408retry
+  );
 });

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits