Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 27, 2020

Currently, data is not being properly transferred to Fetch.js.
So if I try to get the data in Fetch.js, I get wrong value.

var userData = HEAPU32[fetch_attr + {{{ C_STRUCTS.emscripten_fetch_attr_t.userData }}} >> 2];

console.log(getValue(userData, 'i32'));

@kripken kripken requested a review from juj July 28, 2020 21:43
@juj
Copy link
Collaborator

juj commented Jul 30, 2020

Hmm, can you instead try the following fix?

diff --git a/src/Fetch.js b/src/Fetch.js
index 758c0385b..314e3224e 100644
--- a/src/Fetch.js
+++ b/src/Fetch.js
@@ -291,7 +291,7 @@ function __emscripten_fetch_xhr(fetch, onsuccess, onerror, onprogress, onreadyst
   var fetch_attr = fetch + {{{ C_STRUCTS.emscripten_fetch_t.__attributes }}};
   var requestMethod = UTF8ToString(fetch_attr);
   if (!requestMethod) requestMethod = 'GET';
-  var userData = HEAPU32[fetch_attr + {{{ C_STRUCTS.emscripten_fetch_attr_t.userData }}} >> 2];
+  var userData = HEAPU32[fetch + {{{ C_STRUCTS.emscripten_fetch_t.userData }}} >> 2];
   var fetchAttributes = HEAPU32[fetch_attr + {{{ C_STRUCTS.emscripten_fetch_attr_t.attributes }}} >> 2];
   var timeoutMsecs = HEAPU32[fetch_attr + {{{ C_STRUCTS.emscripten_fetch_attr_t.timeoutMSecs }}} >> 2];
   var withCredentials = !!HEAPU32[fetch_attr + {{{ C_STRUCTS.emscripten_fetch_attr_t.withCredentials }}} >> 2];

The userData field is present in both the emscripten_fetch_t and emscripten_fetch_attr_t objects - once we have a fetch going on, it should be read from the fetch instance object instead.

If the above patch works, let's land that instead?

@ghost
Copy link
Author

ghost commented Aug 10, 2020

@juj
It seems this code isn't related with #11590
I reflect your review 👍

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is the change @juj suggested so I think we can land this without waiting.

Thanks @TroyTae !

@kripken kripken merged commit c8903ba into emscripten-core:master Aug 11, 2020
@kripken
Copy link
Member

kripken commented Aug 11, 2020

It would also be good to add a test for this.

@ghost ghost mentioned this pull request Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants