303484 - Recursive spawning of alerts inside alerts causes crash/stack overflow/JS errors

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems wrong that JS timeouts are firing at all while the alert is showing, right?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 9

[Simon Fraser [no longer active]]

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.

Comment 10

[Simon Fraser [no longer active]]

(In reply to comment #9)
> Attachment 190205 [details] [edit] 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 11

[Brendan Eich [:brendan]]

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

Comment 12

[Brendan Eich [:brendan]]

I think we should take roc's bulletproofing fix for 1.8.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

Maybe we should use CallQI instead of ->QI to avoid the casting issue roc ran into?

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 16

[Brendan Eich [:brendan]]

Noting 1.8b4+ after the fact.

/be

Comment 17

[Mike Kaply [:mkaply]]

This appears to have caused regression:

https://bugzilla.mozilla.org/show_bug.cgi?id=309775

Comment 18

[Mike Kaply [:mkaply]]

Confirmed. This definitely caused the regression.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

backed out on trunk and branch to dodge the regressions. Reopening.

Comment 20

[Wayne Mery (:wsmwk)]

testcase doesn't crash or break anything for me.

Comment 21

[Nickolay_Ponomarev]

Since this was backed out of branch, it shouldn't be verified1.8

Comment 22

[Mats Palmgren (inactive)]

Testcase does not crash for me.
Firefox trunk on Linux: only one alert opens, closing it opens the next...
Firefox 2.0.0.11 on Linux: all alerts opens at once, but doesn't crash.

Comment 23

[Nochum Sossonko [:Natch]]

Resolving as WORKSFORME, correct me if I'm wrong!