If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED WORKSFORME

Status

()

Core
DOM
RESOLVED WORKSFORME
12 years ago
9 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: WFM?)

Attachments

(2 attachments)

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.
Created attachment 191644 [details]
testcase

Updated

12 years ago
Severity: normal → critical
Keywords: crash
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.
Assignee: general → roc
Status: NEW → ASSIGNED
Attachment #192055 - Flags: superreview?(brendan)
Attachment #192055 - Flags: review?(brendan)
Blocks: 300057

Comment 9

12 years ago
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.
OS: Linux → All
Hardware: PC → All

Comment 10

12 years ago
(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 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
Attachment #192055 - Flags: superreview?(brendan)
Attachment #192055 - Flags: superreview+
Attachment #192055 - Flags: review?(brendan)
Attachment #192055 - Flags: review+
Attachment #192055 - Flags: approval1.8b4?
I think we should take roc's bulletproofing fix for 1.8.

/be
Flags: blocking1.8b4?
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.

Updated

12 years ago
Attachment #192055 - Flags: approval1.8b4? → approval1.8b4+
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 :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Keywords: fixed1.8

Updated

12 years ago
Flags: blocking1.8b4?
Noting 1.8b4+ after the fact.

/be
Flags: blocking1.8b4+

Updated

12 years ago
Keywords: fixed1.8 → verified1.8

Comment 17

12 years ago
This appears to have caused regression:

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

Comment 18

12 years ago
Confirmed. This definitely caused the regression.
Depends on: 309775
backed out on trunk and branch to dodge the regressions. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
testcase doesn't crash or break anything for me.

Updated

12 years ago
Blocks: 325306

Comment 21

11 years ago
Since this was backed out of branch, it shouldn't be verified1.8
Keywords: verified1.8
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.
Severity: critical → normal
Keywords: crash
Whiteboard: WFM?
Resolving as WORKSFORME, correct me if I'm wrong!
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.