On Tue Sep 3 16:33:40 2024 +0000, Jacek Caban wrote:
Why do you block only specific bits of `hidden_proc`? Shouldn't we do the same when processing event tasks and timers? What's so special about other functions ran via script runner, shouldn't we do the blocking more generic there by putting it in `nsRunnable_Run`? FWIW, it's fine to skip some parts and it's fine if not every check has its test, but when you add, I'd like to understand the logic behind it. If the logic is that tasks should not be processed recursively (which seems right to me), then I guess we'd want to refactor `hidden_proc` to always block them. Similarly, if `run_end_load` is somehow special, then why? Also `SuperNavigate` looks suspicious to me, are you sure it's the right condition?
Oh, for the other functions via script runner, I forgot. I wasn't looking at script runner specifically, I was instead tracking parts where Gecko callbacks ends up calling some external code/notification, and I placed them there. I can move it if you prefer, though it won't really do anything since the others don't have any notifications.
BTW, it's the same with `handle_load`. Do you also want me to move this to HandleEvent there, instead?
On second thought, you're right that I should also block during events or timers. But the problem is, that breaks sync XHR tests, since Gecko will enter a (nested) message loop from an event, and then it needs tasks dispatched to do the actual connection. Although honestly that's already a problem if we do that from a non-event notification… so it's already an issue I just unveiled and hadn't considered.
It doesn't seem too trivial to fix. We could track `tasks_locked` at the point of a sync XHR Send(), but then it would also process other tasks that were previously blocked, and ideally we'd only process those related to the sync XHR itself. Any ideas how I should proceed? I don't want to waste too much time on trying things that won't be accepted.
And lastly for `SuperNavigate`, I just found it out during testing various things, else the new test would fail. I can remove it. Do you have other way to interpret that test that's added in the commit?