On Tue Feb 21 15:08:17 2023 +0000, Gabriel Ivăncescu wrote:
Here's the new version. Unfortunately it's more complicated than I would have liked, but it's necessary to handle all the cases (at least from what I could think of). First issue handled now is that aborted XHRs can be also restarted (according to Gecko code, which I followed closely). To handle this properly, I store a magic cookie (actually two) in the XHR object. Since both abort() and open() are sync operations, the cookie is incremented on them (and reverted on failure), and then checked for delayed events. We store two magic cookies, because one of them is the one matching the XHR object's magic cookie when the pending events task was added. If they mismatch, the task gets removed, and a new one with a new pending events cookie is created. So all the older task events are discarded, as they should be. Note that we have to keep the pending events cookie on the XHR object, rather than the task, because we need to know if we should create a new task or if it's queued already and we just handle events there. The other main problem is that `responseText` still had to be handled, although for different reasons (it was still broken before). We still need to track it, albeit only on progress events dispatched this time, or when `responseText` itself is called. Consider the following scenario that encompasses all 3 possible corner cases, assuming the async_xhr's full response is "1234567890":
- async_xhr.send() is called, but has a slow response from server.
- sync_xhr.send() is called, so it starts blocking.
- async_xhr receives progress from server, with progress event, now it
has "1234" partial response.
- sync_xhr calls one of its handlers, which then does
`async_xhr.responseText`. At this point, this must be NULL/empty, because we've delayed its progress event in the first place.
- sync_xhr.send() finishes blocking.
- jscript code calls `async_xhr.responseText`, which must return NULL
again, as we haven't entered the msg loop.
- code returns and we process msg loop. We dispatch the progress event,
its handler checks `async_xhr.responseText`, which must now be "1234". Then it blocks using another sync_xhr.send().
- async_xhr receives progress from server, with progress event, now it
has "123456" partial response.
- sync_xhr calls one of its handlers, `async_xhr.responseText` must
return "1234", not "123456", i.e. we have to keep track of its state at least before dispatching any progress events.
- sync_xhr.send() finishes blocking, and we go into msg loop, we
dispatch progress event, update async_xhr's state to "123456".
- later **during some other event handler**, it checks
`async_xhr.responseText`, which now returns "1234567" (we're unblocked, so this is fine), with **no progress event sent yet**.
- sync_xhr.send() is called, so it starts blocking again.
- async_xhr gets more progress now, finishes loading, but we must delay
it because we're blocked.
- in a sync_xhr handler, it checks `async_xhr.responseText`, which must
be "1234567" because we've had that returned before, even with no progress event dispatched yet. So, we must keep track of its state when calling responseText without being blocked, as well. Both "123456" (old state at progress event) and "1234567890" (current, fully completed state) would be wrong here! Also another thing to keep in mind is that normal progress events are sent before XHR switches to DONE/COMPLETE readystate, while the others (abort, error, timeout) are sent *after* it switches to DONE. This had to be handled when synthesizing them.
It's not clear to me that we need to worry about `responseText`'s length in such cases or at least I don't think that spec gives such guarantees (https://xhr.spec.whatwg.org/#the-send()-method, 'If not roughly 50ms have passed since these steps were last invoked, then return.' for example). Sync XHRs aside, the assumption that `responseText`'s length matches progress event is only valid inside the handler, not between events. An impact of sync XHR inside event handler is less clear, but it still seems fine to me to continue aggregating async XHR data in that case.