Closed
Bug 52209
Opened 24 years ago
Closed 18 years ago
JS timers can fire while a modal dialog is open
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: jst, Assigned: jst)
References
Details
(Keywords: dom0)
Attachments
(2 files)
|
130 bytes,
text/html
|
Details | |
|
4.26 KB,
patch
|
sicking
:
review+
brendan
:
superreview+
dveditz
:
approval1.8.1.1-
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Comment 1•24 years ago
|
||
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•19 years ago
|
||
See bug 337227 and bug 339683 for some related problems.
Flags: blocking1.9?
(In reply to comment #5)
> Is bug 279518 a dup of this bug?
>
> /be
>
The testcases seem to be almost identical.
|
|
Assignee | |
Comment 7•18 years ago
|
||
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)
|
|
||
Comment 9•18 years ago
|
||
Is this also a problem for other asynchronous-callback things JavaScript has access to, such as XMLHttpRequest?
|
||
Comment 10•18 years ago
|
||
On trunk with threadmanager, it probably is.
|
|
||
Comment 11•18 years ago
|
||
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 13•18 years ago
|
||
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
Attachment #241072 -
Flags: superreview?(brendan)
Attachment #241072 -
Flags: superreview+
Attachment #241072 -
Flags: approval1.8.1.1?
|
|
Assignee | |
Comment 14•18 years ago
|
||
Fix (including sicking's fix) landed on the trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
|
|
||
Comment 15•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
And mousemoves, somehow: bug 324149.
|
||
Comment 18•18 years ago
|
||
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•18 years ago
|
||
If this can be landed safely in 1.8.1.1 I'd like to get it into 1.8.0.x as well.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
|
|
||
Comment 20•18 years ago
|
||
*** Bug 60150 has been marked as a duplicate of this bug. ***
|
||
Comment 21•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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 25•18 years ago
|
||
|
|
||
Comment 26•18 years ago
|
||
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-
|
|
||
Comment 27•18 years ago
|
||
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?
|
||
Updated•18 years ago
|
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
|
|
||
Comment 28•18 years ago
|
||
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+
|
||
Comment 29•17 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•