Fix several Fetch API problems (#17541)

- Fix emscripten_fetch_close to abort active xhrs
 - use dict for active xhrs instead of list to store id generated with globalFetchIdCounter in C
 - calling xhr.abort() will callback, so we add checks to not callback on destroyed emscripten_fetch_t instances
- Don't allocate memory for zero ptrLen (somehow zero-size allocation creates a new memory address, looks like a bug)
- Save response data in xhr.onload if it was not saved in xhr.onprogress

Author: kusharami

AUTHORS

@@ -588,4 +588,5 @@ a license to everyone to use it as detailed in LICENSE.)
 * Kamaron Peterson <kamaron.peterson@gmail.com>
 * Márton Marczell <mmarczell.graphisoft@gmail.com> (copyright owned by GRAPHISOFT SE)
 * Alexander Vestin <alex.vestin@gmail.com>
-* Xiaobing Yang <yangxiaobing@jwzg.com>
+* Xiaobing Yang <yangxiaobing@jwzg.com>
+* Alexandra Cherdantseva <neluhus.vagus@gmail.com>

site/source/docs/api_reference/fetch.rst

@@ -262,8 +262,7 @@ state of the request.
 The emscripten_fetch_attr_t object has a timeoutMSecs field which allows
 specifying a timeout duration for the transfer. Additionally,
 emscripten_fetch_close() can be called at any time for asynchronous and waitable
-fetches to abort the download (this is currently broken, see `#8234
-<https://github.com/emscripten-core/emscripten/issues/8234>`_).
+fetches to abort the download.
 The following example illustrates these fields
 and the onprogress handler.
 

src/Fetch.js

@@ -5,7 +5,8 @@
  */
 
 var Fetch = {
-  xhrs: [],
+  // Map of integer fetch id to XHR request object
+  xhrs: {},
 
   // The web worker that runs proxied file I/O requests. (this field is populated on demand, start as undefined to save code size)
   // worker: undefined,
@@ -314,21 +315,22 @@ function fetchXHR(fetch, onsuccess, onerror, onprogress, onreadystatechange) {
       xhr.setRequestHeader(keyStr, valueStr);
     }
   }
-  Fetch.xhrs.push(xhr);
-  var id = Fetch.xhrs.length;
-  HEAPU32[fetch + {{{ C_STRUCTS.emscripten_fetch_t.id }}} >> 2] = id;
+  var id = HEAPU32[fetch + {{{ C_STRUCTS.emscripten_fetch_t.id }}} >> 2];
+  Fetch.xhrs[id] = xhr;
   var data = (dataPtr && dataLength) ? HEAPU8.slice(dataPtr, dataPtr + dataLength) : null;
   // TODO: Support specifying custom headers to the request.
 
   // Share the code to save the response, as we need to do so both on success
   // and on error (despite an error, there may be a response, like a 404 page).
   // This receives a condition, which determines whether to save the xhr's
   // response, or just 0.
-  function saveResponse(condition) {
+  function saveResponseAndStatus() {
     var ptr = 0;
     var ptrLen = 0;
-    if (condition) {
-      ptrLen = xhr.response ? xhr.response.byteLength : 0;
+    if (xhr.response && fetchAttrLoadToMemory && HEAPU32[fetch + {{{ C_STRUCTS.emscripten_fetch_t.data }}} >> 2] === 0) {
+      ptrLen = xhr.response.byteLength;
+    }
+    if (ptrLen > 0) {
 #if FETCH_DEBUG
       console.log('fetch: allocating ' + ptrLen + ' bytes in Emscripten heap for xhr data');
 #endif
@@ -339,12 +341,8 @@ function fetchXHR(fetch, onsuccess, onerror, onprogress, onreadystatechange) {
     }
     HEAPU32[fetch + {{{ C_STRUCTS.emscripten_fetch_t.data }}} >> 2] = ptr;
     Fetch.setu64(fetch + {{{ C_STRUCTS.emscripten_fetch_t.numBytes }}}, ptrLen);
-  }
-
-  xhr.onload = (e) => {
-    saveResponse(fetchAttrLoadToMemory && !fetchAttrStreamData);
-    var len = xhr.response ? xhr.response.byteLength : 0;
     Fetch.setu64(fetch + {{{ C_STRUCTS.emscripten_fetch_t.dataOffset }}}, 0);
+    var len = xhr.response ? xhr.response.byteLength : 0;
     if (len) {
       // If the final XHR.onload handler receives the bytedata to compute total length, report that,
       // otherwise don't write anything out here, which will retain the latest byte size reported in
@@ -354,9 +352,17 @@ function fetchXHR(fetch, onsuccess, onerror, onprogress, onreadystatechange) {
     HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.readyState }}} >> 1] = xhr.readyState;
     HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.status }}} >> 1] = xhr.status;
     if (xhr.statusText) stringToUTF8(xhr.statusText, fetch + {{{ C_STRUCTS.emscripten_fetch_t.statusText }}}, 64);
+  }
+
+  xhr.onload = (e) => {
+    // check if xhr was aborted by user and don't try to call back
+    if (!(id in Fetch.xhrs)) { 
+      return;
+    }
+    saveResponseAndStatus();
     if (xhr.status >= 200 && xhr.status < 300) {
 #if FETCH_DEBUG
-      console.log('fetch: xhr of URL "' + xhr.url_ + '" / responseURL "' + xhr.responseURL + '" succeeded with status 200');
+      console.log('fetch: xhr of URL "' + xhr.url_ + '" / responseURL "' + xhr.responseURL + '" succeeded with status ' + xhr.status);
 #endif
       if (onsuccess) onsuccess(fetch, xhr, e);
     } else {
@@ -367,27 +373,34 @@ function fetchXHR(fetch, onsuccess, onerror, onprogress, onreadystatechange) {
     }
   };
   xhr.onerror = (e) => {
-    saveResponse(fetchAttrLoadToMemory);
-    var status = xhr.status; // XXX TODO: Overwriting xhr.status doesn't work here, so don't override anywhere else either.
+    // check if xhr was aborted by user and don't try to call back
+    if (!(id in Fetch.xhrs)) { 
+      return;
+    }
 #if FETCH_DEBUG
-    console.error('fetch: xhr of URL "' + xhr.url_ + '" / responseURL "' + xhr.responseURL + '" finished with error, readyState ' + xhr.readyState + ' and status ' + status);
+    console.error('fetch: xhr of URL "' + xhr.url_ + '" / responseURL "' + xhr.responseURL + '" finished with error, readyState ' + xhr.readyState + ' and status ' + xhr.status);
 #endif
-    Fetch.setu64(fetch + {{{ C_STRUCTS.emscripten_fetch_t.dataOffset }}}, 0);
-    Fetch.setu64(fetch + {{{ C_STRUCTS.emscripten_fetch_t.totalBytes }}}, xhr.response ? xhr.response.byteLength : 0);
-    HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.readyState }}} >> 1] = xhr.readyState;
-    HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.status }}} >> 1] = status;
+    saveResponseAndStatus();
     if (onerror) onerror(fetch, xhr, e);
   };
   xhr.ontimeout = (e) => {
+    // check if xhr was aborted by user and don't try to call back
+    if (!(id in Fetch.xhrs)) { 
+      return;
+    }
 #if FETCH_DEBUG
     console.error('fetch: xhr of URL "' + xhr.url_ + '" / responseURL "' + xhr.responseURL + '" timed out, readyState ' + xhr.readyState + ' and status ' + xhr.status);
 #endif
     if (onerror) onerror(fetch, xhr, e);
   };
   xhr.onprogress = (e) => {
+    // check if xhr was aborted by user and don't try to call back
+    if (!(id in Fetch.xhrs)) { 
+      return;
+    }
     var ptrLen = (fetchAttrLoadToMemory && fetchAttrStreamData && xhr.response) ? xhr.response.byteLength : 0;
     var ptr = 0;
-    if (fetchAttrLoadToMemory && fetchAttrStreamData) {
+    if (ptrLen > 0 && fetchAttrLoadToMemory && fetchAttrStreamData) {
 #if FETCH_DEBUG
       console.log('fetch: allocating ' + ptrLen + ' bytes in Emscripten heap for xhr data');
 #endif
@@ -413,6 +426,11 @@ function fetchXHR(fetch, onsuccess, onerror, onprogress, onreadystatechange) {
     }
   };
   xhr.onreadystatechange = (e) => {
+    // check if xhr was aborted by user and don't try to call back
+    if (!(id in Fetch.xhrs)) {
+      {{{ runtimeKeepalivePop() }}}
+      return;
+    }
     HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.readyState }}} >> 1] = xhr.readyState;
     if (xhr.readyState >= 2) {
       HEAPU16[fetch + {{{ C_STRUCTS.emscripten_fetch_t.status }}} >> 1] = xhr.status;
@@ -567,21 +585,27 @@ function startFetch(fetch, successcb, errorcb, progresscb, readystatechangecb) {
 }
 
 function fetchGetResponseHeadersLength(id) {
-    return lengthBytesUTF8(Fetch.xhrs[id-1].getAllResponseHeaders()) + 1;
+    return lengthBytesUTF8(Fetch.xhrs[id].getAllResponseHeaders()) + 1;
 }
 
 function fetchGetResponseHeaders(id, dst, dstSizeBytes) {
-    var responseHeaders = Fetch.xhrs[id-1].getAllResponseHeaders();
+    var responseHeaders = Fetch.xhrs[id].getAllResponseHeaders();
     var lengthBytes = lengthBytesUTF8(responseHeaders) + 1;
     stringToUTF8(responseHeaders, dst, dstSizeBytes);
     return Math.min(lengthBytes, dstSizeBytes);
 }
 
 //Delete the xhr JS object, allowing it to be garbage collected.
 function fetchFree(id) {
-  //Note: should just be [id], but indexes off by 1 (see: #8803)
 #if FETCH_DEBUG
-  console.log("fetch: Deleting id:" + (id-1) + " of " + Fetch.xhrs);
+    console.log("fetch: Deleting id:" + id + " of " + Fetch.xhrs);
 #endif
-  delete Fetch.xhrs[id-1];
+    var xhr = Fetch.xhrs[id];
+    if (xhr) {
+        delete Fetch.xhrs[id];
+        // check if fetch is still in progress and should be aborted
+        if (xhr.readyState > 0 && xhr.readyState < 4) {
+            xhr.abort();
+        }
+    }
 }

test/fetch/xhr_abort.cpp

@@ -0,0 +1,55 @@
+// Copyright 2022 The Emscripten Authors.  All rights reserved.
+// Emscripten is available under two separate licenses, the MIT license and the
+// University of Illinois/NCSA Open Source License.  Both these licenses can be
+// found in the LICENSE file.
+
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <emscripten/fetch.h>
+
+void handleSuccess(emscripten_fetch_t *fetch) {
+  assert(false && "Should not succeed");
+  emscripten_fetch_close(fetch);
+}
+
+void handleError(emscripten_fetch_t *fetch) {
+  printf("handleError: %d %s\n", fetch->status, fetch->statusText);
+  bool isAbortedStatus = fetch->status == (unsigned short) -1;
+  assert(isAbortedStatus); // should have aborted status
+  EM_ASM({
+    const xhr = Fetch.xhrs[$0];
+    const oldReadyStateChangeHandler = xhr.onreadystatechange;
+    // Overriding xhr handlers to check if xhr.abort() was called
+    xhr.onreadystatechange = (e) => {
+      console.log("fetch: xhr.readyState: " + xhr.readyState + ', status: ' + xhr.status);
+      assert(xhr.readyState === 0 || xhr.status === 0, "readyState should be equal to UNSENT(0) or status should be equal to 0 when calling xhr.abort()");
+      oldReadyStateChangeHandler(e);
+    };
+    xhr.onload = () => {
+      assert(false, "xhr.onload should not be called after xhr.abort()");
+    };
+    xhr.onerror = () => {
+      assert(false, "xhr.onerror should not be called after xhr.abort()");
+    };
+    xhr.onprogress = () => {
+      assert(false, "xhr.onprogress should not be called after xhr.abort()");
+    };
+  }, fetch->id);
+}
+
+void handleStateChange(emscripten_fetch_t *fetch) {
+  emscripten_fetch_close(fetch); // Abort the fetch
+}
+
+int main() {
+  emscripten_fetch_attr_t attr;
+  emscripten_fetch_attr_init(&attr);
+  strcpy(attr.requestMethod, "GET");
+  attr.attributes = EMSCRIPTEN_FETCH_REPLACE;
+  attr.onsuccess = handleSuccess;
+  attr.onerror = handleError;
+  attr.onreadystatechange = handleStateChange;
+  auto fetch = emscripten_fetch(&attr, "gears.png");
+  return 0;
+}

test/test_browser.py

@@ -4597,6 +4597,11 @@ def test_fetch_stream_file(self):
   def test_fetch_headers_received(self):
     self.btest_exit('fetch/headers_received.cpp', args=['-sFETCH_DEBUG', '-sFETCH'])
 
+  @also_with_wasm64
+  def test_fetch_xhr_abort(self):
+    shutil.copyfile(test_file('gears.png'), 'gears.png')
+    self.btest_exit('fetch/xhr_abort.cpp', args=['-sFETCH_DEBUG', '-sFETCH'])
+
   # Tests emscripten_fetch() usage in synchronous mode when used from the main
   # thread proxied to a Worker with -sPROXY_TO_PTHREAD option.
   @no_firefox('https://github.com/emscripten-core/emscripten/issues/16868')