okhttp: optimize HPACK to index :path and fix dynamic table eviction bugs (grpc#12819)

Allow the :path pseudo-header to be added to the HPACK dynamic table in
the gRPC-Java OkHttp transport, resolving the original goal of grpc#12819
and redoing reverted commit 397d3e7 (grpc#12799, grpc#12820).

Background & Root Cause:
In gRPC over HTTP/2, :path headers represent static service/method names
(e.g., /package.Service/Method), which are constant across RPC calls.
Previously, indexing :path was reverted (grpc#12820) because high dynamic
table churn uncovered latent bugs in legacy OkHttp Hpack.java:

1. Off-by-one NPE loop bound in evictToRecoverBytes:
   When bytesToRecover exceeded total dynamic table size, the loop condition
   (j >= nextDynamicTableIndex) inspected the unallocated nextDynamicTableIndex
   slot containing null, throwing a NullPointerException. Fixed by changing
   the loop bound to (j > nextDynamicTableIndex).

2. Reference leak post-eviction:
   System.arraycopy shifted active entries rightward, but left stale
   Header/ByteString references in vacated array slots. Fixed by setting
   vacated array slots to null via Arrays.fill().

3. Case sensitivity normalization:
   Header entries inserted into dynamicTable are now guaranteed to store
   lowercased name keys, preventing failed dynamic table header lookups.

4. Table Size Setting Handling:
   Works in conjunction with grpc#12818 (SETTINGS_HEADER_TABLE_SIZE handling).

Fixes grpc#12819
Related: grpc#12799, grpc#12818, grpc#12820, b/514688016

Author: AgraVator

okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Hpack.java

@@ -189,14 +189,15 @@ private int evictToRecoverBytes(int bytesToRecover) {
       int entriesToEvict = 0;
       if (bytesToRecover > 0) {
         // determine how many headers need to be evicted.
-        for (int j = dynamicTable.length - 1; j >= nextDynamicTableIndex && bytesToRecover > 0; j--) {
+        for (int j = dynamicTable.length - 1; j > nextDynamicTableIndex && bytesToRecover > 0; j--) {
           bytesToRecover -= dynamicTable[j].hpackSize;
           dynamicTableByteCount -= dynamicTable[j].hpackSize;
           dynamicTableHeaderCount--;
           entriesToEvict++;
         }
         System.arraycopy(dynamicTable, nextDynamicTableIndex + 1, dynamicTable,
             nextDynamicTableIndex + 1 + entriesToEvict, dynamicTableHeaderCount);
+        Arrays.fill(dynamicTable, nextDynamicTableIndex + 1, nextDynamicTableIndex + 1 + entriesToEvict, null);
         nextDynamicTableIndex += entriesToEvict;
       }
       return entriesToEvict;
@@ -489,17 +490,21 @@ void writeHeaders(List<io.grpc.okhttp.internal.framed.Header> headerBlock) throw
           out.writeByte(0x40);
           writeByteString(name);
           writeByteString(value);
-          insertIntoDynamicTable(header);
-        } else if (name.startsWith(PSEUDO_PREFIX) && !io.grpc.okhttp.internal.framed.Header.TARGET_AUTHORITY.equals(name)) {
-          // Follow Chromes lead - only include the :authority pseudo header, but exclude all other
-          // pseudo headers. Literal Header Field without Indexing - Indexed Name.
+          insertIntoDynamicTable(new io.grpc.okhttp.internal.framed.Header(name, value));
+        } else if (name.startsWith(PSEUDO_PREFIX)
+            && !io.grpc.okhttp.internal.framed.Header.TARGET_AUTHORITY.equals(name)
+            && !io.grpc.okhttp.internal.framed.Header.TARGET_PATH.equals(name)) {
+          // Allow :authority and :path pseudo headers to be indexed. Other pseudo headers are not
+          // indexed.
+          // This is a departure from original Chrome behavior, as gRPC paths (ServiceName/MethodName)
+          // are stable and benefit from indexing.
           writeInt(headerNameIndex, PREFIX_4_BITS, 0);
           writeByteString(value);
         } else {
           // Literal Header Field with Incremental Indexing - Indexed Name.
           writeInt(headerNameIndex, PREFIX_6_BITS, 0x40);
           writeByteString(value);
-          insertIntoDynamicTable(header);
+          insertIntoDynamicTable(new io.grpc.okhttp.internal.framed.Header(name, value));
         }
       }
     }
@@ -557,14 +562,15 @@ private int evictToRecoverBytes(int bytesToRecover) {
       int entriesToEvict = 0;
       if (bytesToRecover > 0) {
         // determine how many headers need to be evicted.
-        for (int j = dynamicTable.length - 1; j >= nextDynamicTableIndex && bytesToRecover > 0; j--) {
+        for (int j = dynamicTable.length - 1; j > nextDynamicTableIndex && bytesToRecover > 0; j--) {
           bytesToRecover -= dynamicTable[j].hpackSize;
           dynamicTableByteCount -= dynamicTable[j].hpackSize;
           dynamicTableHeaderCount--;
           entriesToEvict++;
         }
         System.arraycopy(dynamicTable, nextDynamicTableIndex + 1, dynamicTable,
             nextDynamicTableIndex + 1 + entriesToEvict, dynamicTableHeaderCount);
+        Arrays.fill(dynamicTable, nextDynamicTableIndex + 1, nextDynamicTableIndex + 1 + entriesToEvict, null);
         nextDynamicTableIndex += entriesToEvict;
       }
       return entriesToEvict;

okhttp/third_party/okhttp/test/java/io/grpc/okhttp/internal/framed/HpackTest.java

@@ -1108,22 +1108,41 @@ public void doNotIndexPseudoHeaders() throws IOException {
     hpackWriter.writeHeaders(headerEntries(":method", "PUT"));
     assertBytes(0x02, 3, 'P', 'U', 'T');
     assertEquals(0, hpackWriter.dynamicTableHeaderCount);
-
-    hpackWriter.writeHeaders(headerEntries(":path", "/okhttp"));
-    assertBytes(0x04, 7, '/', 'o', 'k', 'h', 't', 't', 'p');
-    assertEquals(0, hpackWriter.dynamicTableHeaderCount);
   }
 
   @Test
-  public void incrementalIndexingWithAuthorityPseudoHeader() throws IOException {
-    hpackWriter.writeHeaders(headerEntries(":authority", "foo.com"));
-    assertBytes(0x41, 7, 'f', 'o', 'o', '.', 'c', 'o', 'm');
+  public void pseudoHeaderIndexingForPathAndAuthority() throws IOException {
+    // :path should now be indexed
+    hpackWriter.writeHeaders(headerEntries(":path", "/okhttp"));
+    assertBytes(0x44, 7, '/', 'o', 'k', 'h', 't', 't', 'p');
+    assertEquals(1, hpackWriter.dynamicTableHeaderCount);
+    // Second call to same :path should be an index reference byte (0xbe)
+    hpackWriter.writeHeaders(headerEntries(":path", "/okhttp"));
+    assertBytes(0xbe);
     assertEquals(1, hpackWriter.dynamicTableHeaderCount);
 
+    // :authority should be indexed
+    hpackWriter.writeHeaders(headerEntries(":authority", "foo.com"));
+    assertBytes(0x41, 7, 'f', 'o', 'o', '.', 'c', 'o', 'm');
+    assertEquals(2, hpackWriter.dynamicTableHeaderCount);
+    // Second call to same :authority should be an index reference byte (0xbe)
     hpackWriter.writeHeaders(headerEntries(":authority", "foo.com"));
     assertBytes(0xbe);
+    assertEquals(2, hpackWriter.dynamicTableHeaderCount);
+  }
+
+  @Test
+  public void evictToRecoverBytesDoesNotNpeWhenBytesToRecoverExceedsTable() throws IOException {
+    hpackWriter.writeHeaders(headerEntries("custom-key", "custom-value"));
     assertEquals(1, hpackWriter.dynamicTableHeaderCount);
 
+    // Force resize to table size 0, requiring total eviction exceeding table capacity
+    hpackWriter.resizeHeaderTable(0);
+
+    assertEquals(0, hpackWriter.dynamicTableHeaderCount);
+    assertEquals(0, hpackWriter.dynamicTableByteCount);
+  }
+
     // If the :authority header somehow changes, it should be re-added to the dynamic table.
     hpackWriter.writeHeaders(headerEntries(":authority", "bar.com"));
     assertBytes(0x41, 7, 'b', 'a', 'r', '.', 'c', 'o', 'm');