diff --git a/ext/naett-lib/README-ppsspp.md b/ext/naett-lib/README-ppsspp.md index 4178b678a4..32bd5fa89b 100644 --- a/ext/naett-lib/README-ppsspp.md +++ b/ext/naett-lib/README-ppsspp.md @@ -88,3 +88,15 @@ Keep this list up to date - it's what makes it possible to move to a newer upstr - `src/naett_android.c`: `GetMethodID` returns NULL for a method it can't find, and calling with a NULL `jmethodID` aborts the VM; the header loop could also hand `GetStringUTFChars` a null value for a header with no entries. +- `src/naett_core.c`: `naettClose` cleared `res->request` before calling the backend's close, + which is the one thing the backend needs - the WinHTTP handles hang off the request. The + backend goes first now. +- `naett.h`: documented that a response should be complete before it's closed. Only the Android + backend really cancels and waits. +- `src/naett_win.c`: `naettPlatformCloseResponse` was empty, so the status callback kept the + freed response as its context. Unhooks the callback and closes the request handle. Not a full + cancel - a callback already running isn't waited for, which would need the + `WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING` handshake. +- `src/naett_osx.c`: `invalidateAndCancel` returns before the session lets go of its delegate, so + the delegate's back pointer to the response is cleared first, and `didReceiveData` checks it + (as `didCompleteWithError` already did). diff --git a/ext/naett-lib/naett.h b/ext/naett-lib/naett.h index b79130db3b..d49614d0b1 100644 --- a/ext/naett-lib/naett.h +++ b/ext/naett-lib/naett.h @@ -131,6 +131,12 @@ naettReq* naettGetRequest(naettRes* response); /** * @brief Closes a response object. + * + * PPSSPP: the response should be complete (see `naettComplete`) before closing it. Only the + * Android backend actually cancels an in-flight request and waits for its worker; the others + * stop what they can and return, so a transfer already inside a callback on another thread can + * still be running as this returns. Closing an incomplete response is therefore not safe in + * general - wait for it, or let it leak, which is what we do on shutdown. */ void naettClose(naettRes* response); diff --git a/ext/naett-lib/src/naett_core.c b/ext/naett-lib/src/naett_core.c index bc6e2e24e1..71f5cdaf7a 100644 --- a/ext/naett-lib/src/naett_core.c +++ b/ext/naett-lib/src/naett_core.c @@ -416,8 +416,10 @@ void naettClose(naettRes* response) { assert(response != NULL); InternalResponse* res = (InternalResponse*)response; - res->request = NULL; + // PPSSPP: the backends need the request to shut a response down - it owns the handles on + // Windows, for one - so clear it after they've had their turn, not before. naettPlatformCloseResponse(res); + res->request = NULL; KVLink* node = res->headers; freeKVList(node); free(res->body.data); diff --git a/ext/naett-lib/src/naett_osx.c b/ext/naett-lib/src/naett_osx.c index 24eb7c8ae3..1692b0c09e 100644 --- a/ext/naett-lib/src/naett_osx.c +++ b/ext/naett-lib/src/naett_osx.c @@ -98,6 +98,12 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) { id p = pool(); object_getInstanceVariable(self, "response", (void**)&res); + // PPSSPP: didComplete already checked this; this one didn't, and the delegate outlives the + // response when a session is invalidated. + if (res == NULL) { + release(p); + return; + } if (res->headers == NULL) { id response = objc_msgSend_t(id)(dataTask, sel("response")); @@ -139,10 +145,15 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) { } } + if (res->request == NULL) { + release(p); + return; + } + const void* bytes = objc_msgSend_t(const void*)(data, sel("bytes")); NSUInteger length = objc_msgSend_t(NSUInteger)(data, sel("length")); - res->request->options.bodyWriter(bytes, length, res->request->options.bodyWriterData); + res->request->options.bodyWriter(bytes, (int)length, res->request->options.bodyWriterData); res->totalBytesRead += (int)length; release(p); @@ -217,6 +228,13 @@ void naettPlatformCloseResponse(InternalResponse* res) { if (res->session == nil) { return; } + // PPSSPP: invalidateAndCancel returns before the session is done with its delegate, so clear + // the back pointer the delegate holds - a callback that lands afterwards then sees NULL + // rather than a freed response. + id delegate = objc_msgSend_t(id)(res->session, sel("delegate")); + if (delegate != nil) { + object_setInstanceVariable(delegate, "response", NULL); + } objc_msgSend_void(res->session, sel("invalidateAndCancel")); release(res->session); res->session = nil; diff --git a/ext/naett-lib/src/naett_win.c b/ext/naett-lib/src/naett_win.c index 57e1f7858a..c80b157150 100644 --- a/ext/naett-lib/src/naett_win.c +++ b/ext/naett-lib/src/naett_win.c @@ -365,6 +365,20 @@ void naettPlatformFreeRequest(InternalRequest* req) { } void naettPlatformCloseResponse(InternalResponse* res) { + // PPSSPP: this used to be empty. The status callback carries the response as its context, so + // once it's freed any further completion writes through a dangling pointer. Unhook the + // callback and close the request handle, which stops new ones being raised. + // + // This is not a full cancel: a callback already running on another thread isn't waited for, + // which would need the WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING handshake. Closing a response + // that hasn't completed still isn't supported here - see naettClose in naett.h. + InternalRequest* req = res->request; + if (req == NULL || req->request == NULL) { + return; + } + WinHttpSetStatusCallback(req->request, NULL, WINHTTP_CALLBACK_FLAG_ALL_NOTIFICATIONS, 0); + WinHttpCloseHandle(req->request); + req->request = NULL; } #endif // __WINDOWS__