See upcoming testcase. By firing an alert on a timeout while another alert is showing, we can nest event loops to arbitrary depths and cause all kinds of trouble.
It seems wrong that JS timeouts are firing at all while the alert is showing, right?
I thought we had a bug about the issues around which exact event queue nsITimer events end up firing in.... (as in, not the event queue that posted the timer, but the current event queue for the thread, or something to that effect).
(In reply to comment #3) > I thought we had a bug about the issues around which exact event queue nsITimer > events end up firing in.... (as in, not the event queue that posted the timer, > but the current event queue for the thread, or something to that effect). I don't think that would work very well.
I mean, doesn't that mean that all timer-related events would not fire until the alert closed? That probably isn't always what we want.
I think we should have a event queue nesting limit, refuse to create event queues nested beyond the limit, and ensure that that error propagates back to window opening, which in this case would cause alert() to throw a JS exception. Any objections?
I think there are bigger problems here then just the crash. Content JS should not 'reenter' while an alert() is being displayed. Something like: ... setTimeout("doB()", 10); startA(); alert('hi'); endA(); ... Should never cause doB() to be called between startA and endA. But I agree that we probably don't want to stop *all* timers on an alert(). The throbber etc would be nice to have running. And there are probably parts of loading that would be blocked too. Maybe we should have some way to "bind" a timer to a queue and then use that for content-setTimeout calls.
Created attachment 192055 [details] [diff] [review] fix This is the bulletproofing fix. With this, the testcase spawns a lot of activity but most of the alerts fail to show and the browser remains stable. If we want to change timeout policy w.r.t. modal alerts, better file another bug. Note that this fixes crippling bugs in GetYounger/GetElder, which I can only assume were never previously called.
Attachment 190205 [details] in bug 300057 shows a slightly different problem (which, for reasons that I don't understand, is manifest only on Mac). It has a <div> with an "onmouseover=alert()", and also some DHTML. Each time the content changes, the view manager is posting a synthetic mouse moved event via *youngest* (i.e. the popup's) event queue, and this mouse moved event spawns an "onmouseover", and thus another alert(). What seems wrong about this situation is that the view manager is posting to the youngest event queue (which may belong to a modal window). It seems that the relationship between event queues and window is not well defined.
(In reply to comment #9) > Attachment 190205 [details]  in bug 300057 shows a slightly different problem (which, for reasons that I don't understand, is manifest only on Mac) Roc says that it is apparent on Linux. I stand corrected.
Comment on attachment 192055 [details] [diff] [review] fix Nits only, r+sr=me. >+ ++depth; >+ if (depth > MAX_EVENTQUEUE_NESTING_DEPTH) Combine into one line? >+ chromeFlags |= nsIWebBrowserChrome::CHROME_MODAL | >+ nsIWebBrowserChrome::CHROME_DEPENDENT; You just moved/unindented this, but how about indenting the right operand of | to line up with the left? Thanks for getting this one! Nominating for 1.8b4, a clear fix we should include, but I'll let another driver + it. /be
I think we should take roc's bulletproofing fix for 1.8. /be
Maybe we should use CallQI instead of ->QI to avoid the casting issue roc ran into?
And fwiw, GetElder is called from nsEventQueueServiceImpl::~nsEventQueueServiceImpl (or rather from hash_enum_remove_queues) and has been for a while. Wonder why that never bit us; or rather why we never noticed it biting.
checked in on trunk and branch, with CallQueryInterface as bz suggested. > Combine into one line? I didn't do that. I hate side effecting boolean expressions :-)
Noting 1.8b4+ after the fact. /be
This appears to have caused regression: https://bugzilla.mozilla.org/show_bug.cgi?id=309775
Confirmed. This definitely caused the regression.
backed out on trunk and branch to dodge the regressions. Reopening.
testcase doesn't crash or break anything for me.
Since this was backed out of branch, it shouldn't be verified1.8
Testcase does not crash for me. Firefox trunk on Linux: only one alert opens, closing it opens the next... Firefox 220.127.116.11 on Linux: all alerts opens at once, but doesn't crash.
Resolving as WORKSFORME, correct me if I'm wrong!