JS timers can fire while a modal dialog is open

RESOLVED FIXED in Future

Status

()

Core
DOM: Core & HTML
P3
major
RESOLVED FIXED
17 years ago
9 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

({dom0})

Trunk
Future
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?
blocking1.8.1.1 -
blocking1.8.0.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

17 years ago
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

17 years ago
Created attachment 14442 [details]
Testcase

Comment 2

17 years ago
->future
Target Milestone: --- → Future
Keywords: dom0

Comment 3

12 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

Updated

12 years ago
Assignee: danm.moz → nobody

Comment 4

11 years ago
See bug 337227 and bug 339683 for some related problems.
Flags: blocking1.9?
Is bug 279518 a dup of this bug?

/be
Blocks: 279518

Updated

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

The testcases seem to be almost identical.
(Assignee)

Comment 7

11 years ago
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.
Attachment #241072 - Flags: superreview?(brendan)
Attachment #241072 - Flags: review?(bzbarsky)
Hoping Darin can take a look too.

/be
Assignee: nobody → jst

Comment 9

11 years ago
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?

/be
Attachment #241072 - Flags: superreview?(brendan)
Attachment #241072 - Flags: superreview+
Attachment #241072 - Flags: approval1.8.1.1?
(Assignee)

Comment 14

11 years ago
Fix (including sicking's fix) landed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 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?

/be
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

11 years ago
And mousemoves, somehow: bug 324149.

Comment 18

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

11 years ago
*** Bug 60150 has been marked as a duplicate of this bug. ***

Comment 21

11 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..
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
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-
Filed bug 360871 and bug 360872 on comment 16.
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?

Updated

11 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+

Comment 28

11 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+

Updated

9 years ago
Depends on: 418493

Comment 29

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