Closed Bug 52209 Opened 24 years ago Closed 18 years ago

JS timers can fire while a modal dialog is open


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






(Reporter: jst, Assigned: jst)



(Keywords: dom0)


(2 files)

The following HTML causes mozilla to bring up more than one modal dialog:

setTimeout("alert('timer alert');", 1000);

This is bad, and even seems to cause a crash in todays build. Dan, we talked
about this, one possible solution could be to give timers a context and make the
timers not fire (postpone them in stead) if the context (window) has a modal
dialog open.
Attached file Testcase
Target Milestone: --- → Future
Keywords: dom0
If I run the attachement I get an "alert" screen, followed by a "timer alert".
Are these those modal dialogs you're talking about?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20050111
Assignee: danm.moz → nobody
See bug 337227 and bug 339683 for some related problems.
Is bug 279518 a dup of this bug?

Blocks: 279518
Blocks: 337716
(In reply to comment #5)
> Is bug 279518 a dup of this bug?
> /be

The testcases seem to be almost identical.
This prevents timeouts from running in a window (and any of its its top-level window's children) while there's a modal dialog open for that window (or any window inside the same top-level window). When the modal dialog is closed we dispatch an event off of which we run any pending timeouts in all windows that were affected by the modal dialog.
Attachment #241072 - Flags: superreview?(brendan)
Attachment #241072 - Flags: review?(bzbarsky)
Hoping Darin can take a look too.

Assignee: nobody → jst
Is this also a problem for other asynchronous-callback things JavaScript has access to, such as XMLHttpRequest?
On trunk with threadmanager, it probably is.
I'll try to get to this ASAP, but given the state of time I have and my review queue that's going to be probably at least a week.  :(
Comment on attachment 241072 [details] [diff] [review]
Prevent timeouts from running while modal dialogs are open.

I take it LeaveModalState is only called on the toplevel window? If not you need to grab the toplevel window in there.

>+  for (i = 0; i < length && aTopWindow->mModalStateDepth != 0; i++) {

Shouldn't this be |aTopWindow->mModalStateDepth == 0|?

r=me with that.
Attachment #241072 - Flags: review?(bzbarsky) → review+
Comment on attachment 241072 [details] [diff] [review]
Prevent timeouts from running while modal dialogs are open.

r=me with that sicking good catch fixed.

Will this back-port to 1.8 branch easily?

Attachment #241072 - Flags: superreview?(brendan)
Attachment #241072 - Flags: superreview+
Attachment #241072 - Flags: approval1.8.1.1?
Fix (including sicking's fix) landed on the trunk.
Closed: 18 years ago
Resolution: --- → FIXED
Not sure we can back-port for 1.8.1.x -- some extension or Mozilla-only content might be counting on the ability to run timeouts "in the background" of a modal.  Comments?

I don't think we want to mess with this on the 1.8 branch...

I do think we need a followup for trunk for XMLHttpRequest and anything else that does async notifications.  Like event handlers?  At least on GTK key events get dispatched to the main window while a modal dialog is up.  :(
And mousemoves, somehow: bug 324149.
Self-reminder: check if generator close hooks still allow to execute scripts when modal dialog is showing even with this bug fixed. For example, when GC is triggered in another window, the script can still show more alerts AFAICS. 
If this can be landed safely in I'd like to get it into 1.8.0.x as well.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
*** Bug 60150 has been marked as a duplicate of this bug. ***
Brendan/Bz do we want this on the - you state in comment 15 that you are not sure then asked for approval on the patch - so we are a little confused..
I had to review the chronology here to recall my thinking, but clearly I first shared dveditz's wish to fix this in 1.8.x, then thought better of it due to the unfrozen but significant API incompatibility.  I'm still thinking we should hang tough on 1.8.x and fix this only for 1.9.  Boris?

I stand by what I said in comment 16 -- I think this is fragile code and would rather not touch it on the branch.

Did those followups ever happen, by the way?
I think we're going to want this in FF2 at some point, but I'll defer to the fragility judgment for the next ones.
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Comment on attachment 241072 [details] [diff] [review]
Prevent timeouts from running while modal dialogs are open.

Not this time, possibly not ever on the 1.8 branch
Attachment #241072 - Flags: approval1.8.1.1? → approval1.8.1.1-
Should this be attempted on the 1.8 branches and can it be done safely? We've had more trunk baking time now.
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Based on comment #22 and comment #23, we don't plan on taking this on the branches.  
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Depends on: 418493
Because of this change, I had to switch to using nsITimer in one of the extensions I work on.  To help other extension developers, I added this issue to
You need to log in before you can comment on or make changes to this bug.