Merge pull request mozilla#18673 from Snuffleupagus/use-Headers

Use `Headers` consistently in the different `IPDFStream` implementations
timvandermeij · Sep 3, 2024 · 082ad95 · 082ad95
2 parents da99f5d + d3a94f1
commit 082ad95
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 57 deletions.
diff --git a/src/display/fetch_stream.js b/src/display/fetch_stream.js
@@ -15,6 +15,7 @@
 
 import { AbortException, assert, warn } from "../shared/util.js";
 import {
+  createHeaders,
   createResponseStatusError,
   extractFilenameFromHeader,
   validateRangeRequestCapabilities,
@@ -38,18 +39,6 @@ function createFetchOptions(headers, withCredentials, abortController) {
   };
 }
 
-function createHeaders(httpHeaders) {
-  const headers = new Headers();
-  for (const property in httpHeaders) {
-    const value = httpHeaders[property];
-    if (value === undefined) {
-      continue;
-    }
-    headers.append(property, value);
-  }
-  return headers;
-}
-
 function getArrayBuffer(val) {
   if (val instanceof Uint8Array) {
     return val.buffer;
@@ -66,7 +55,7 @@ class PDFFetchStream {
   constructor(source) {
     this.source = source;
     this.isHttp = /^https?:/i.test(source.url);
-    this.httpHeaders = (this.isHttp && source.httpHeaders) || {};
+    this.headers = createHeaders(this.isHttp, source.httpHeaders);
 
     this._fullRequestReader = null;
     this._rangeRequestReaders = [];
@@ -123,17 +112,13 @@ class PDFFetchStreamReader {
     this._abortController = new AbortController();
     this._isStreamingSupported = !source.disableStream;
     this._isRangeSupported = !source.disableRange;
-
-    this._headers = createHeaders(this._stream.httpHeaders);
+    // Always create a copy of the headers.
+    const headers = new Headers(stream.headers);
 
     const url = source.url;
     fetch(
       url,
-      createFetchOptions(
-        this._headers,
-        this._withCredentials,
-        this._abortController
-      )
+      createFetchOptions(headers, this._withCredentials, this._abortController)
     )
       .then(response => {
         if (!validateResponseStatus(response.status)) {
@@ -147,7 +132,7 @@ class PDFFetchStreamReader {
         const { allowRangeRequests, suggestedLength } =
           validateRangeRequestCapabilities({
             getResponseHeader,
-            isHttp: this._stream.isHttp,
+            isHttp: stream.isHttp,
             rangeChunkSize: this._rangeChunkSize,
             disableRange: this._disableRange,
           });
@@ -222,17 +207,14 @@ class PDFFetchStreamRangeReader {
     this._isStreamingSupported = !source.disableStream;
 
     this._abortController = new AbortController();
-    this._headers = createHeaders(this._stream.httpHeaders);
-    this._headers.append("Range", `bytes=${begin}-${end - 1}`);
+    // Always create a copy of the headers.
+    const headers = new Headers(stream.headers);
+    headers.append("Range", `bytes=${begin}-${end - 1}`);
 
     const url = source.url;
     fetch(
       url,
-      createFetchOptions(
-        this._headers,
-        this._withCredentials,
-        this._abortController
-      )
+      createFetchOptions(headers, this._withCredentials, this._abortController)
     )
       .then(response => {
         if (!validateResponseStatus(response.status)) {

diff --git a/src/display/network.js b/src/display/network.js
@@ -15,6 +15,7 @@
 
 import { assert, stringToBytes } from "../shared/util.js";
 import {
+  createHeaders,
   createResponseStatusError,
   extractFilenameFromHeader,
   validateRangeRequestCapabilities,
@@ -38,11 +39,11 @@ function getArrayBuffer(xhr) {
 }
 
 class NetworkManager {
-  constructor(url, args = {}) {
+  constructor({ url, httpHeaders, withCredentials }) {
     this.url = url;
     this.isHttp = /^https?:/i.test(url);
-    this.httpHeaders = (this.isHttp && args.httpHeaders) || Object.create(null);
-    this.withCredentials = args.withCredentials || false;
+    this.headers = createHeaders(this.isHttp, httpHeaders);
+    this.withCredentials = withCredentials || false;
 
     this.currXhrId = 0;
     this.pendingRequests = Object.create(null);
@@ -70,12 +71,8 @@ class NetworkManager {
 
     xhr.open("GET", this.url);
     xhr.withCredentials = this.withCredentials;
-    for (const property in this.httpHeaders) {
-      const value = this.httpHeaders[property];
-      if (value === undefined) {
-        continue;
-      }
-      xhr.setRequestHeader(property, value);
+    for (const [key, val] of this.headers) {
+      xhr.setRequestHeader(key, val);
     }
     if (this.isHttp && "begin" in args && "end" in args) {
       xhr.setRequestHeader("Range", `bytes=${args.begin}-${args.end - 1}`);
@@ -194,10 +191,7 @@ class NetworkManager {
 class PDFNetworkStream {
   constructor(source) {
     this._source = source;
-    this._manager = new NetworkManager(source.url, {
-      httpHeaders: source.httpHeaders,
-      withCredentials: source.withCredentials,
-    });
+    this._manager = new NetworkManager(source);
     this._rangeChunkSize = source.rangeChunkSize;
     this._fullRequestReader = null;
     this._rangeRequestReaders = [];

diff --git a/src/display/network_utils.js b/src/display/network_utils.js
@@ -21,6 +21,21 @@ import {
 import { getFilenameFromContentDispositionHeader } from "./content_disposition.js";
 import { isPdfFile } from "./display_utils.js";
 
+function createHeaders(isHttp, httpHeaders) {
+  const headers = new Headers();
+
+  if (!isHttp || !httpHeaders || typeof httpHeaders !== "object") {
+    return headers;
+  }
+  for (const key in httpHeaders) {
+    const val = httpHeaders[key];
+    if (val !== undefined) {
+      headers.append(key, val);
+    }
+  }
+  return headers;
+}
+
 function validateRangeRequestCapabilities({
   getResponseHeader,
   isHttp,
@@ -98,6 +113,7 @@ function validateResponseStatus(status) {
 }
 
 export {
+  createHeaders,
   createResponseStatusError,
   extractFilenameFromHeader,
   validateRangeRequestCapabilities,

diff --git a/src/display/node_stream.js b/src/display/node_stream.js
@@ -15,6 +15,7 @@
 
 import { AbortException, assert, MissingPDFException } from "../shared/util.js";
 import {
+  createHeaders,
   extractFilenameFromHeader,
   validateRangeRequestCapabilities,
 } from "./network_utils.js";
@@ -53,7 +54,7 @@ class PDFNodeStream {
       this.url.protocol === "http:" || this.url.protocol === "https:";
     // Check if url refers to filesystem.
     this.isFsUrl = this.url.protocol === "file:";
-    this.httpHeaders = (this.isHttp && source.httpHeaders) || {};
+    this.headers = createHeaders(this.isHttp, source.httpHeaders);
 
     this._fullRequestReader = null;
     this._rangeRequestReaders = [];
@@ -291,6 +292,9 @@ class PDFNodeStreamFullReader extends BaseFullReader {
   constructor(stream) {
     super(stream);
 
+    // Node.js requires the `headers` to be a regular Object.
+    const headers = Object.fromEntries(stream.headers);
+
     const handleResponse = response => {
       if (response.statusCode === 404) {
         const error = new MissingPDFException(`Missing PDF "${this._url}".`);
@@ -321,11 +325,7 @@ class PDFNodeStreamFullReader extends BaseFullReader {
       this._filename = extractFilenameFromHeader(getResponseHeader);
     };
 
-    this._request = createRequest(
-      this._url,
-      stream.httpHeaders,
-      handleResponse
-    );
+    this._request = createRequest(this._url, headers, handleResponse);
 
     this._request.on("error", reason => {
       this._storedError = reason;
@@ -342,15 +342,9 @@ class PDFNodeStreamRangeReader extends BaseRangeReader {
   constructor(stream, start, end) {
     super(stream);
 
-    this._httpHeaders = {};
-    for (const property in stream.httpHeaders) {
-      const value = stream.httpHeaders[property];
-      if (value === undefined) {
-        continue;
-      }
-      this._httpHeaders[property] = value;
-    }
-    this._httpHeaders.Range = `bytes=${start}-${end - 1}`;
+    // Node.js requires the `headers` to be a regular Object.
+    const headers = Object.fromEntries(stream.headers);
+    headers.Range = `bytes=${start}-${end - 1}`;
 
     const handleResponse = response => {
       if (response.statusCode === 404) {
@@ -361,7 +355,7 @@ class PDFNodeStreamRangeReader extends BaseRangeReader {
       this._setReadableStream(response);
     };
 
-    this._request = createRequest(this._url, this._httpHeaders, handleResponse);
+    this._request = createRequest(this._url, headers, handleResponse);
 
     this._request.on("error", reason => {
       this._storedError = reason;

diff --git a/test/unit/network_utils_spec.js b/test/unit/network_utils_spec.js
@@ -14,6 +14,7 @@
  */
 
 import {
+  createHeaders,
   createResponseStatusError,
   extractFilenameFromHeader,
   validateRangeRequestCapabilities,
@@ -25,6 +26,44 @@ import {
 } from "../../src/shared/util.js";
 
 describe("network_utils", function () {
+  describe("createHeaders", function () {
+    it("returns empty `Headers` for invalid input", function () {
+      const headersArr = [
+        createHeaders(
+          /* isHttp = */ false,
+          /* httpHeaders = */ { "Content-Length": 100 }
+        ),
+        createHeaders(/* isHttp = */ true, /* httpHeaders = */ undefined),
+        createHeaders(/* isHttp = */ true, /* httpHeaders = */ null),
+        createHeaders(/* isHttp = */ true, /* httpHeaders = */ "abc"),
+        createHeaders(/* isHttp = */ true, /* httpHeaders = */ 123),
+      ];
+      const emptyObj = Object.create(null);
+
+      for (const headers of headersArr) {
+        expect(Object.fromEntries(headers)).toEqual(emptyObj);
+      }
+    });
+
+    it("returns populated `Headers` for valid input", function () {
+      const headers = createHeaders(
+        /* isHttp = */ true,
+        /* httpHeaders = */ {
+          "Content-Length": 100,
+          "Accept-Ranges": "bytes",
+          "Dummy-null": null,
+          "Dummy-undefined": undefined,
+        }
+      );
+
+      expect(Object.fromEntries(headers)).toEqual({
+        "content-length": "100",
+        "accept-ranges": "bytes",
+        "dummy-null": "null",
+      });
+    });
+  });
+
   describe("validateRangeRequestCapabilities", function () {
     it("rejects invalid rangeChunkSize", function () {
       expect(function () {