Attachment #733786 - Flags: feedback?(benjamin)
Comment on attachment 733786 [details] [diff] [review] Block event-loop recursion (WIP) Won't this assert every time somebody calls alert() or confirm() or synchronous XHR or calls a plugin method which spins a nested event loop (which are relatively common)? There are times that we absolutely must not spin a nested event loop (especially during layout), but I'm skeptical that we ought to try to prevent this from content JS when there are so many cases where we do it on purpose.
Attachment #733786 - Flags: feedback?(benjamin) → feedback-
And in fact, for content JS there are places where the spec and web compat _requires_ the event loop to spin (c.f. showModalDialog).
Created attachment 734085 [details] [diff] [review] Block event-loop recursion (WIP #2) now suspends blocks in nsXMLHttpRequest::Send(), ShowAsModal dialogs and CreateNewContentWindow
Attachment #733786 - Attachment is obsolete: true
From IRC: [09:29] bsmedberg jesup: the problem is really plugins, because they can spin the event loop almost anywhere [09:30] bsmedberg jesup: which sucks, but I don't know if there's anything we can do about it. Instead I think it would be much more interesting to identify the critical sections where we *shouldn't* spin an event loop ... [09:31] bsmedberg it should be safe to spin the event loop any time we're running content JS [09:31] bsmedberg it might lead to weird reentrance behavior, but not security bugs/memory corruption/crashes [09:31] jesup bsmedberg: The patch adds a (debug-only) runtime abort when it believes we're spinning and shouldn't. We can set (or unset) the blocked-from-spinning state at any point we need [09:32] bsmedberg jesup: yeah, I'm saying this should 1) probably be release-mode and not just debug-mode 2) we should pick more interesting targets [09:32] bsmedberg like "whenever we're enumerating a hashtable" ... [09:43] jesup bsmedberg: so I see two related questions: 1) where is it absolutely unsafe to allow the event loop to spin (and we may want to block even in release), 2) Where is it bad/weird/causes-random-behavior to spin the loop in that it allows re-entrance? [09:44] jesup My patch was targeted at the second, but could easily be extended to cover the first as well [09:44] bsmedberg jesup: yes; I don't think we should spend time on #2 at the moment [09:44] bsmedberg the web is just crazy enough that "anytime" pretty much covers it [09:46] jesup bsmedberg: ok, though it makes me sad in that we (repeatedly) check in code that does this (and breaks things in the process). Perhaps I'll put a guard against DISPATCH_SYNC from MainThread (at least from within JS)? [09:46] bsmedberg jesup: ah yeah, I can't think of *any* reason we should DISPATCH_SYNC from the main thread [09:47] bsmedberg but of course, I've been surprised before [09:47] derf I suspect the difficulty there will be finding all the places we actually do it and fixing them before you can _leave_ that guard in.
> the problem is really plugins, because they can spin the event loop almost anywhere For fuzzing, I'd be more than happy with an assertion that ignores the plugin case. Even if it's as an exclusion as wide as MOZ_ASSERT_IF(!everLoadedAnyPlugin, ...).
You need to log in before you can comment on or make changes to this bug.