Last Comment Bug 52209 - JS timers can fire while a modal dialog is open
: JS timers can fire while a modal dialog is open
Status: RESOLVED FIXED
: dom0
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P3 major (vote)
: Future
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Prashant Desale
Mentors:
: 60150 (view as bug list)
Depends on: 418493
Blocks: 279518 337716
  Show dependency treegraph
 
Reported: 2000-09-11 17:49 PDT by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2008-10-31 15:32 PDT (History)
22 users (show)
csthomas: blocking1.9?
dveditz: blocking1.8.1.1-
dveditz: blocking1.8.0.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (130 bytes, text/html)
2000-09-11 17:50 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details
Prevent timeouts from running while modal dialogs are open. (4.26 KB, patch)
2006-10-03 09:30 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jonas: review+
brendan: superreview+
dveditz: approval1.8.1.1-
Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2000-09-11 17:49:43 PDT
The following HTML causes mozilla to bring up more than one modal dialog:

<html>
<head>
<script>
setTimeout("alert('timer alert');", 1000);
alert("alert!");
</script>
</head>
<body>
text
</body>
</html>

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.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2000-09-11 17:50:18 PDT
Created attachment 14442 [details]
Testcase
Comment 2 Peter Trudelle 2000-09-14 15:50:08 PDT
->future
Comment 3 Thomas 2005-02-20 03:37:26 PST
Johnny,
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
Comment 4 Jesse Ruderman 2006-05-30 08:57:41 PDT
See bug 337227 and bug 339683 for some related problems.
Comment 5 Brendan Eich [:brendan] 2006-10-02 13:21:18 PDT
Is bug 279518 a dup of this bug?

/be
Comment 6 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-10-02 16:03:05 PDT
(In reply to comment #5)
> Is bug 279518 a dup of this bug?
> 
> /be
> 

The testcases seem to be almost identical.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2006-10-03 09:30:43 PDT
Created attachment 241072 [details] [diff] [review]
Prevent timeouts from running while modal dialogs are open.

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.
Comment 8 Brendan Eich [:brendan] 2006-10-03 12:36:13 PDT
Hoping Darin can take a look too.

/be
Comment 9 Jesse Ruderman 2006-10-03 15:45:30 PDT
Is this also a problem for other asynchronous-callback things JavaScript has access to, such as XMLHttpRequest?
Comment 10 Boris Zbarsky [:bz] 2006-10-04 13:37:56 PDT
On trunk with threadmanager, it probably is.
Comment 11 Boris Zbarsky [:bz] 2006-10-04 13:38:45 PDT
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 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-10-04 16:02:50 PDT
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.
Comment 13 Brendan Eich [:brendan] 2006-10-04 16:33:52 PDT
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?

/be
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2006-10-05 10:30:59 PDT
Fix (including sicking's fix) landed on the trunk.
Comment 15 Brendan Eich [:brendan] 2006-10-05 10:39:15 PDT
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?

/be
Comment 16 Boris Zbarsky [:bz] 2006-10-05 11:17:32 PDT
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.  :(
Comment 17 Jesse Ruderman 2006-10-05 11:28:22 PDT
And mousemoves, somehow: bug 324149.
Comment 18 Igor Bukanov 2006-10-07 18:08:29 PDT
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. 
Comment 19 Daniel Veditz [:dveditz] 2006-10-07 18:21:06 PDT
If this can be landed safely in 1.8.1.1 I'd like to get it into 1.8.0.x as well.
Comment 20 Jesse Ruderman 2006-10-14 23:51:42 PDT
*** Bug 60150 has been marked as a duplicate of this bug. ***
Comment 21 Mike Schroepfer 2006-11-01 10:14:35 PST
Brendan/Bz do we want this on the 1.8.1.1 - you state in comment 15 that you are not sure then asked for approval on the patch - so we are a little confused..
Comment 22 Brendan Eich [:brendan] 2006-11-01 10:54:51 PST
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?

/be
Comment 23 Boris Zbarsky [:bz] 2006-11-01 18:44:35 PST
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?
Comment 24 Daniel Veditz [:dveditz] 2006-11-13 12:05:27 PST
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.
Comment 25 Boris Zbarsky [:bz] 2006-11-15 21:42:48 PST
Filed bug 360871 and bug 360872 on comment 16.
Comment 26 Daniel Veditz [:dveditz] 2006-11-27 17:20:10 PST
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
Comment 27 Daniel Veditz [:dveditz] 2006-12-14 15:25:40 PST
Should this be attempted on the 1.8 branches and can it be done safely? We've had more trunk baking time now.
Comment 28 Jay Patel [:jay] 2007-01-05 15:12:36 PST
Based on comment #22 and comment #23, we don't plan on taking this on the branches.  
Comment 29 Mark Smith (:mcs) 2008-03-05 10:26:09 PST
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 http://developer.mozilla.org/en/docs/Updating_extensions_for_Firefox_3#Other_changes

Note You need to log in before you can comment on or make changes to this bug.