Closed Bug 61098 (alertloops) Opened 24 years ago Closed 14 years ago

Exit all currently active scripts (allow aborting modal window.alert() loops in javascript (js))

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta7+
status2.0 --- ?
status1.9.2 --- ?
status1.9.1 --- wanted

People

(Reporter: ruairif, Assigned: jst)

References

(Depends on 2 open bugs, Blocks 5 open bugs)

Details

(Keywords: hang, Whiteboard: [sg:want P2][has review][wanted1.9.3])

Attachments

(9 files, 17 obsolete files)

Don't click here unless you want your browser to crash. Proof of concept. Especially annoying if you have lots of tabs.
(deleted), text/html
Details
Opera alert dialog box
(deleted), image/png
Details
stop_runaway_script.js
(deleted), application/x-javascript
Details
Description of how to close a tab with a running endless alert loop.
(deleted), text/html
Details
Fixed on Chrome: Checkbox appears on the second loop of while(1)alert(1)
(deleted), image/png
Details
log of irc comments from bz
(deleted), text/plain
Details
Updated fix.
(deleted), patch
sicking
: review+
Details | Diff | Splinter Review
Followup fixes.
(deleted), patch
sicking
: review+
Details | Diff | Splinter Review
For checkin.
(deleted), patch
jst
: review+
Details | Diff | Splinter Review 
Often when you're testing scripts, javascript or otherwise, you end up getting 
lost in a load of alert boxes, causing you to spend ages clicking the [OK]
button just to end the script.
It'd  be quite nice if there was an option to "end all currently active script" 
or "turn off javascript for this page" selectable from the menubar.
I realise that giving focus to an alert box means that you can't return to the 
browser until all the boxes have closed, so I suppose this would have to be an 
option selectable from an alert box also (perhaps alongside the standard [x] 
button in the top-right corner, there could be a similar icon indicating "close 
all boxes" in some fashion??).Would this conflict with ECMAscript standards 
though?
Just a thought...
Browser, not engine. Reassigning to Browser-General for consideration -
Assignee: rogerl → asa
Status: UNCONFIRMED → NEW
Component: Javascript Engine → Browser-General
Ever confirmed: true
QA Contact: pschwartau → doronr
Summary: [enhancement] Exit all currently active scripts → [enhancement] Exit all currently active scripts
making rfe.
Assignee: asa → nobody
Keywords: helpwanted
Summary: [enhancement] Exit all currently active scripts → [RHE] Exit all currently active scripts
Summary: [RHE] Exit all currently active scripts → [RFE] Exit all currently active scripts
dup of some part of bug 22049...
DOM 0 then it is.
Assignee: nobody → jst
Component: Browser-General → DOM Level 0
QA Contact: doronr → desale
Nice to have but hard to do with the threading model used in mozilla. Not high
priority --> Future
Status: NEW → ASSIGNED
Target Milestone: --- → Future
See bug 59314 for another suggestion as to how to prevent DoS attacks using 
modal dialogs.  In that bug, window-modal dialogs would be replaced with 
content-modal dialogs, so that the user could still leave the page or at least 
close the window.

Note that many dialogs besides page javascript alerts are currently modal -- 
the "connection refused" alert is a good example.  Bug 28586 would eliminate at 
least some of the networking-error alerts.
Hmm, and don't forget the Basic Auth dialog.  That one isn't going away anytime 
soon.
Keywords: dom0
Keywords: dom0
*** Bug 128196 has been marked as a duplicate of this bug. ***
Summary: [RFE] Exit all currently active scripts → Exit all currently active scripts
*** Bug 156346 has been marked as a duplicate of this bug. ***
I think this is really a common problem for web-developers. I've fallen into
this trap several times myself (accidentally created an infite loop around my
debugging alert()). But instead of aborting all scripts, I'd like to have
something similar to the "A script on this page is causing Mozilla to run slow"
message with the option to abort the script. As a trigger for this event I would
suggest something like this:

When an alert is closed, note the time of the close. When a new alert pops up,
compute the difference between the current time and the close time of the
previous alert. If it is below a sensible threshold (let's say 5 seconds),
increase a counter. If it is above the threshold, reset the counter. If the
counter reaches 10, fire the dialog "A script on this page is creating alert()
boxes at an alarming rate. This could make your browser unusable. Abort the script?"

In case the description doesn't make this clear. The box should fire if several
times in a row the time between alerts is too short to click
Stop/Close/Back/Whatever.

Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
Status: ASSIGNED → NEW
Example:
http://megajocke.y2.org/post/oops.html

Don't open it if you have something important open in the browser.
*** Bug 213586 has been marked as a duplicate of this bug. ***
*** Bug 66097 has been marked as a duplicate of this bug. ***
*** Bug 215453 has been marked as a duplicate of this bug. ***
*** Bug 218608 has been marked as a duplicate of this bug. ***
IMHO, a javascript blocker that popups under restrictive conditions is OK but
not enough. An accelerator such as Ctrl+Pause on PCs, and whatever fits on Macs,
must be provided.

http://www.stremler.net/evil_js.html
Acting on Duplicate resolution from bug 66097 comment 16:

*Moving 'Blocks: bug 140346' from bug 66097 to bug 61098.
 (Take further action as needed.)
Blocks: 140346
*** Bug 219345 has been marked as a duplicate of this bug. ***
*** Bug 241605 has been marked as a duplicate of this bug. ***
*** Bug 240311 has been marked as a duplicate of this bug. ***
*** Bug 251227 has been marked as a duplicate of this bug. ***
*** Bug 249360 has been marked as a duplicate of this bug. ***
*** Bug 258585 has been marked as a duplicate of this bug. ***
*** Bug 261950 has been marked as a duplicate of this bug. ***
Summary: Exit all currently active scripts → Exit all currently active scripts (allow aborting modal window.alert() loops in javascript (js))
*** Bug 270391 has been marked as a duplicate of this bug. ***
*** Bug 276080 has been marked as a duplicate of this bug. ***
*** Bug 123308 has been marked as a duplicate of this bug. ***
*** Bug 261390 has been marked as a duplicate of this bug. ***
*** Bug 136324 has been marked as a duplicate of this bug. ***
*** Bug 281145 has been marked as a duplicate of this bug. ***
*** Bug 283838 has been marked as a duplicate of this bug. ***
*** Bug 286400 has been marked as a duplicate of this bug. ***
Given the amount of dupes, this is a highly requested feature.  Actually, it
would be a lot easier to implement a Javascript function that stops this,
accessable through this JavaScript console.  In this respect, a short-term
solution wouldn't require any UI changes.  I still have this blasted Javascript
alert loop in one of my windows (unfix.org/~jeroen/archive/javascript_loop.html
if you're interested in having an non-functioning browser window), as I was
searching Google for such a JS command.

BTW, I do like haferfrost@web.de's solution from comment #10 as a longer term
solution.
*** Bug 304496 has been marked as a duplicate of this bug. ***
(In reply to comment #5)
> Nice to have but hard to do with the threading model used in mozilla. Not high
> priority --> Future

That's interesting, because I thought that it would be easy to implement: just
use the same feature that lets Mozilla abort scripts that are taking too long to
execute...

By the way, sorry about the dupe.
Basically pops up alert boxes that say 'This alert box will never go away.' and
'The only way you can get rid of this box is by terminating the browser
process.'
How do you determine that the alert will never go away?
(In reply to comment #38)
> How do you determine that the alert will never go away?

In theory, it shouldn't end (the loop is on a true, i.e. while(true), which, of
course, will never evaluate to false. In practice, these loops can be used, but
they're generally stopped via a break statement).

If you don't trust me, you can try it and then keep on pressing OK. For
practical reasons, let's make 20 equal infinity.
No, I mean how does Mozilla know that the loop will never end? It's easy enough
if it's just

while (true) {
  alert("");
}

but if it's in the form of

while (true) {
  if (someFunction()) {
    alert("");
  } else {
    break;
  }
}

how would you determine that someFunction() will ever return false? See
http://en.wikipedia.org/wiki/Halting_problem

The "taking too long to execute" feature I'm guessing just figures that the
script has been running too long rather than determining that it's an infinite
loop. When you loop an alert(), the script isn't executing most of the time,
it's just waiting for the user to press OK. The idea in comment 10 is similar,
but definitely not the same as the "taking too long" feature.
I think the idea is to let the user decide when the alert is an "infinite" loop,
like some sort of second option in a normal alert box that says "OK, and stop
the script."
I think the feature can be expressed as:

    * The Stop button should always work. *

At the moment, if you have a script that pops up endless dialogue boxes, you
cannot hit the Stop button in the browser.  It's disabled because of the modal
dialogue box.  One answer to this is to include a mini-Stop-button in the
dialogue box itself.  Another would be to make Javascript dialogues non-modal,
or at least allow Stop and other browser UI elements to keep working when a
Javascript dialogue box is displayed.
(In reply to comment #40)
> No, I mean how does Mozilla know that the loop will never end? It's easy enough

It doesn't have to. All that is required is a CTRL+Break shortcut or simmilar,
to abort the currently running script.
fwiw anyone w/ venkman open and an understanding of how it works can stop such 
loops today. (so can anyone w/ ie's script debugger and ie properly configured).

can people please stop writing these comments in the bug? unless you're 
actually posting code, you're just wasting everyone's time.
Blocks: 304634
Suggestion for people using alert() for debugging purposes:

Use HTML/DOM for showing debugging information instead of alert(). I've rigged
up a small library that allows just this. If you're looping 1,000 times through,
it'll just update the debug div 1,000 times; eventually, Mozilla will prompt you
to abort the script.

Suggestions for improvement here welcome.

---------------------
//USE: debugWindow.show('key','value');
//
//SHOULDN'T NEED THIS, BUT: debugWindow.hide()

var DebugWindow = function(styleAttributes) {
	this.pane = document.createElement('div');
	this.pane.style.position = 'absolute';
	this.pane.style.top = 0;
	this.pane.style.right = 0;
	this.pane.style.display = 'none';
	this.pane.style.fontSize = 'small';
	this.pane.style.width = '200px';
	this.pane.style.backgroundColor = 'darkred';
	this.pane.style.color = 'white';

	for (var attribute in styleAttributes) {
		this.pane.style[attribute] = styleAttributes[attribute];
	}

	var header = document.createElement('h4');
	header.appendChild(document.createTextNode('Debug Window'));

	this.messageDiv = document.createElement('div');

	var hideButton = document.createElement('button');
	hideButton.appendChild(document.createTextNode('Hide'));
	hideButton.onclick = function() {
		this.parentNode.style.display = 'none';
	}

	this.data = {};

	this.pane.appendChild(header);
	this.pane.appendChild(document.createElement('hr'));
	this.pane.appendChild(this.messageDiv);
	this.pane.appendChild(document.createElement('hr'));
	this.pane.appendChild(hideButton);
	document.body.appendChild(this.pane);

	return this;
}

//KEYS MUST EVALUATE TO BOOLEAN TRUE
DebugWindow.prototype.show = function(key, newValue) {
	this.pane.style.display = 'block';

	if (key) {
		var keyExists = this.data[key];
		var newTextNode = document.createTextNode(newValue);

		if (keyExists) {
			this.data[key].value = newValue;
			this.data[key].elem.replaceChild(
			   newTextNode, this.data[key].elem.lastChild
			);
		} else {
			this.data[key] = {
			   value: newValue, elem: document.createElement('div')
			};
			this.data[key].elem.appendChild(
			   document.createTextNode(key+': ')
			);
			this.data[key].elem.appendChild(newTextNode);
		}

		this.messageDiv.appendChild(this.data[key].elem);
	}
}
DebugWindow.prototype.hide = function() {
	this.pane.style.display = 'none';
}

window.debugWindow = new DebugWindow();
*** Bug 309394 has been marked as a duplicate of this bug. ***
*** Bug 294093 has been marked as a duplicate of this bug. ***
1. best solution
* modal windows should only modal for content area of browser window, not for
browser menus or toolbars

2. or a CTRL+Break (Comment #43)

3. simple and quick solution (temporary)

javascript engine should check user pref javascript.enabled 
after statments executing ... 
* alert(), confirm(), prompt(), print(), window.open()
* file browse window <input type=file>
* download due to location="data:application/x-y-z,test"

and user can re-launch another browser session and disable javascript
(In reply to comment #48)
> 1. best solution
> * modal windows should only modal for content area of browser window, not for
> browser menus or toolbars
> 
> 2. or a CTRL+Break (Comment #43)

I think both of these should be eventually implimented.
May I suggest that the dialogs created by a page be implemented as children of
the tab containing the page? That is, if my page creates a dialog, instead of it
being its own toplevel window, it be displayed within the content area of the
tab which contains the code requesting it?

That way, not only could the browswer's own chrome still be accessible, but the
issues of one tab spoofing an alert for another tab would also go away, as the
dialog so created would be on the spoofing page's content area, not the spoofed
page's content area.

This would also be more in line with the idea behind tabs - that multiple web
sites are represented by one user interface level window - currently alerts
break that model.

This would ALSO prevent the annoying behavior of a page being loaded in a
background tab usurping the focus when a "Set cookie", "Run Javascript", or "Run
Plugin" dialog is thrown - such dialogs would be on the tab, and thus would not
interrupt me on the tab I am working with.
I have a possible solution to the problem (I think/hope). 
pseudo code;

$delta_lines = span lines of code
$nbr = number of popups
$delta_time = timespan, excluding the time for user input to occur (pressing the
OK button)

if ($delta_line tries to open $nbr alerts or windows in $delta_time seconds) {
halt javascript;
} 

Sorry for adding to the spam, but I have a suggestion I didn't see in any of the
comments above.

The code could be changed so that, if instead of dismissing a modal Javascript
dialog by clicking on a button, you dismissed it by holding some modifier key
(for instance, Control or Shift) and clicking on a button, either the script
were stopped or all new attempts to open a modal Javascript dialog were ignored.

There is a precedent for this sort of solution on the Windows
shutdown/restart/logoff dialogs.
*** Bug 312786 has been marked as a duplicate of this bug. ***
Should this not be a critical bug, since it causes data loss by rendering
existing tabs completely inaccessible?
*** Bug 317891 has been marked as a duplicate of this bug. ***
(In reply to comment #52)
> The code could be changed so that, if instead of dismissing a modal Javascript
> dialog by clicking on a button, you dismissed it by holding some modifier key
> (for instance, Control or Shift) and clicking on a button, 

A modifier-key is not quite usable, and users who are an bad websites with alert-loop probably don't want to search for modifier-Keys. maybe we want in alert,confirm an promt-windows some new "cancel script"-Button or something like this. This button may be hidden in the first alert but may be shown after (for example) 3 alerts are shown in 60 seconds.

> either the script
> were stopped or all new attempts to open a modal Javascript dialog were
> ignored.

I don't think that we want wo continue with the script. If a script is so evil that is showns many alerts, then the script will not do anything useful after it. Look at this example-script:
while(true) { alert("haha"); }
If we ignore alerts now, then the script will be in a infinite loop and causes probably additional "the browser is running slowly, cancel script?"-warnings and/or cause the browser to be very slow.

Wouldn't it be possible to raise a simple javascript-error when the user want's to cancel the script?

Lupin, I agree. This seems to be critical because often there is no easy way to exit this script and the user has to kill the browser with the taskmanager which can cause dataloss.
Just so you know: there usually is an easy way to get out, all you have to do is press Ctrl + W and keep it pressed while you click the OK button.
Ctrl+W closes the current tab which may cause dataloss if there where textareas or something similar in it.

You probably agree that this is not a suitable method for the normal stupid user.
(In reply to comment #58)
> Ctrl+W closes the current tab which may cause dataloss if there where textareas
> or something similar in it.
> 
> You probably agree that this is not a suitable method for the normal stupid
> user.
> 

A small point, but "the normal stupid user" probably doesn't encounter many sites where the need to exit all scripts is evident.
ive thought of that but the thing you need to know is that it would close the site.  what i want is just to stop the script not close the whole site itself cause sometimes the problem is the script not the site.  there may be useful info on the site.  and also by holding down ctrl-W you could accidentally close the other tabs you have open.  why cant firefox just remove the freezing of the site in case a script comes out?  do you think its possible to just have an extension on it?  not NoScript but just something that will not freeze up the whole browser in case a javascript pops up
(In reply to comment #59)
> A small point, but "the normal stupid user" probably doesn't encounter many
> sites where the need to exit all scripts is evident.

There are many "joke-sites" in the web, that create lots of alerts.

The problem is that websites _can_ annoy the user. And that is bad. We have many technologies in Firefox that prevent some of these annoyances. We have a popup blocker, a GUI for disabling plugins and an option to prevent sites from opening new windows. It would be very nice to have the option to cancel annoying scripts, too.

IMO, the best solution would be to create a new button in alert(), confirm() and similar windows.

Maybe we should redesign the entire alert()-Window. There should be an information that the text comes from the webpage an this is not an message from Firefox or the OS. For example, webpages could say "Your computer has been infected by viruses, please delete [some important file] and reboot" or something like that. "Normal stupid users" may believe that.
Blocks: 327310
*** Bug 253239 has been marked as a duplicate of this bug. ***
> site in case a script comes out?  do you think its possible to just have an
> extension on it?  not NoScript but just something that will not freeze up the
> whole browser in case a javascript pops up

None of this would be a problem for me if the alerts were content-modal instead of window-modal.

Then you could just click the X of a rogue site''s tab and get rid of it. 

I really don't get why they need to be window-modal. It makes no sense to me. It's also very distracting when you're working with mutiple tabs, and one in the background pops up an alert, causing your tab to change.


 
There should be a "Kill script" button in every alert() dialog to be able to kill the script.

Someone should change the severity to critical, because this bug can cause data loss.

The bug was opened more than five years ago. Why is it so difficult to fix it?
(In reply to comment #64)
> There should be a "Kill script" button in every alert() dialog to be able to
> kill the script.
> 
> Someone should change the severity to critical, because this bug can cause data
> loss.
> 
> The bug was opened more than five years ago. Why is it so difficult to fix it?
> 

Opera 9 has such a feature, it has a small checkbox in the alert that says: "stop executing scripts on this page"

Opera also has a more "designed" alert box, it has a content area (which contains the message), and a border around that which holds, the okay button, and this new stop script checkbox.

Why they use a checkbox, and not a simple button eludes me, possibly because a large button (because of the length of the text) would be attracting focus from the user, and would be clicked by not reading users to quick.
Attached image Opera alert dialog box (deleted) — 
Screen-shot of the Opera alert dialog box.

When selecting the "Stop executing scripts on this page", every page loaded on that tab has Javascript turned off.
*** Bug 355122 has been marked as a duplicate of this bug. ***
My bug has been marked as duplicate of this one, so I want just post additional thoughts attached to the deleted one:

Even alerts from properly working scripts shouldn't make whole main window unresponsive. They should only block tab that created them.
Reason: Sometimes you need to copy text from such box to the mail you are writing in other tab.
Besides, blocking only one tab's content resolves the problem with a loop - you can just close the tab.

Also, from my point of view it's more than an enhancement. Just try to start filling a long form in the e-banking web page and enter such loop in the second page (looking for data for the form or anything)... and then try to save your data from the form or even only logout... If it may lead to data or money loose, for me it's just a bug.
I agree - anything that allows a web page to hijack a user’s browser ought to be considered a deficiency.
Just a note: even if this does end up being patched, it won't do much against malicious web pages. For instance, the exploit listed here <http://sla.ckers.org/forum/read.php?14,1294> maxes out CPU and RAM.

Here's my advice: if you're a developer, stop using alert dialogues to debug scripts, instead, dump the data in a dynamically generated textarea. If you're a concerned end user, install NoScript.
> Here's my advice: if you're a developer, stop using alert dialogues to debug
> scripts, instead, dump the data in a dynamically generated textarea. If you're
> a concerned end user, install NoScript.

People keep missing the main issue in this bug.

I could have 8 tabs open, be on the 8th tab, and the page in the first tab causes an alert. What happens?

- An alert box appears totally inturrupting whatever I am doing, even though I am not on the first tab

- The alert box causes the tabs to switch to the first one, without asking me at all.

It's a huge issue. It's a security issue as well, because the alert box steals focus! If I was entering a password that contained a space in it in the page I was on at the time, and my eyes were not on the screen, then large portions of that password could be captured by the offending page, since the space will close the alert. I would think it would be simple to do ap roff of concept of this hijack - site has a link that opens secure site in a new window, starts a timer, timer fires and steals back focus, hopefully grabbing part of the password.
(In reply to comment #71)
> People keep missing the main issue in this bug.

The main issue in this bug, per the summary and every comment before yours, is being able to abort JavaScript from the alert dialog. You want bug 123913.
How about a (more general) solution of making the "scripts are taking too long, kill?" dialog come up if a user holds the Esc key down for say 2 sec.
(In reply to comment #73)
> How about a (more general) solution of making the "scripts are taking too long,
> kill?" dialog come up if a user holds the Esc key down for say 2 sec.
> 

What about a simple solution: if i hit "Esc", kill everything that moves (images, plugins, scripts,...) *now* (only after the page has fully loaded, before that just stop loading)
Still no way to stop the while(1) alert('blocked') that blocks the navigator and requiers to end task or kill navigator, not really clean...
Matthias Wallner > should also block loading of pages (that can block the navigateor with too larg pages, like 10000 cell table in a pages)
And don't forget to write that clealy in the help file

> How about a (more general) solution of making the "scripts are taking too long,
> kill?" dialog come up if a user holds the Esc key down for say 2 sec.
and if we click no ant we want 1min later to kill that? should be better to have CTRL+SCROLLLOCK 
IMO the major issue of this problem is still the window-modal alert dialogs. I really don't get why we still have to have those. 

It is endlessly annoying for one webpage in a background tab to be able to interrupt browsing in another with a stupid dialog.

Dialogs should be document-modal only.

Also see my comment #71 as to why IMO this is a security issue.
(In reply to comment #75)
> > How about a (more general) solution of making the "scripts are taking too long,
> > kill?" dialog come up if a user holds the Esc key down for say 2 sec.
> and if we click no ant we want 1min later to kill that? should be better to
> have CTRL+SCROLLLOCK 

Then you would hold Esc pressed for say 2 sec again, get the dialog and answer yes.


Agreed with all the above posters who said the alert boxes should be content-modal.  I'd expand that definition and say 'tab-modal'.

I would do that, as well as adding a button to alert boxes saying something like 'OK, and stop script execution".  I'd also alias Ctrl-Break to pressing this button.  It might still be useful to be able to view the page's content, but get rid of the alert box, rather than having to close the whole tab.
Attached file stop_runaway_script.js (deleted) — 
One solution for this is JavaScript Engine should check "javascript.enabled" preference before and after 
alert(), confirm(), prompt() etc. This will allow user to start another Firefox session and disable JavaScript.

I found currently when we check navigator.userAgent we get the latest value from preference.
So I made a temp solution for testers see attachment stop_runaway_script.js I have also put how to use instructions in it.

I think we should be able to convert this to Greasemonkey script to run automatically for all pages https://addons.mozilla.org/en-US/firefox/addon/748
Wouldn't the simplest and quickest solution be - 
if you press the button in the alert, it behaves like now
else  
if you press the "close window"/Alt+F4/ctrl+w then disable the script  using the same technique that is used in unresponsive scripts.   

It makes sense conceptually, as the close window is normally associated with a shutdown/Stop program, whereas using the button is a continue the program kinda operation.   Therefore the operation would be what one expect from a program.

I have always considered that the close window acts like pressing the button in the alert window, is highly counter-intuitive.

Of course I do not have any experience with the underlying code, but it seems to me that should be a quick and easy fix?

This problem really should be upgraded from an enhancement to something more, because it can cause data loss, and perhaps in some odd cases even loss of real money, because of having to abort a net bank transaction - I generally make sure that I no tabs open when I use my netbank, but it may not be everone who does this, and it is soo easy to forget an open window.
If you're referring to the parent window's close button, making it clickable would require making the alert non-modal, which would be not be a quick and easy fix. If you're referring to the alert's close button, alerts don't have a close button on all platforms.
Removing CC.
(In reply to comment #84)
> If you're referring to the parent window's close button, making it clickable
> would require making the alert non-modal, which would be not be a quick and
> easy fix. 

Actually, that may be the quickest fix of all these proposals. While an alert box has to block JS script from executing to keep compatibility with all existing scripts, I have never seen anything that says that a JS alert box has to be modal.

If the alert was not modal then the user could close the tab or window causing the issue.
This needs to be put in. Ive encountered spyware ads that are infinite confirm() and alert() loops. This is a security concern.
Actually I was referring to the close window button on the modal alert, that button does the exact same as if you pressed the OK button in current implementation, it releases the java script block.

My suggestion in it's simplicity is to make that close window button, abort the script instead of releasing the block.

OR

To make it even simpler, closing the modal alert window, release the UI from the modal lock, but still keeps the javascript blocked.

Either solution would allow the user to close the offending window, or to continue with what they were doing.

To my mind it seems that these two solutions I mention here, should have least impact on the system as a whole, while still making sense for the average user.

Comment #84 states that alerts don't have a close button on all platforms.  This would be an elegant solution from a UI standpoint, though, I think, if they did.
In fact, the bug is that :

Status:  	NEW
Severity: 	enhancement
Priority: 	P3
Target Milestone: 	Future

since 2000, there's this bug, and even Firefox 3 beta have this bug.

Will contact admin...
Depends on: 410247
Depends on: 391834
No longer depends on: 410247
I have been hijacked several times by pages that catch me in infinite loops, not allowing me to close the tab, or save any data. I have been forced to kill Firefox.

Now, when I submit it as a bug, it gets marked as a duplicate of a request for "enhancement", which will be resolved in the "future".

What gives? This is a *serious vulnerability* in the application since it allows *any page* to hijack the application and force the user to kill it, lose opened tabs and form data, cancel downloads, etc.

This is not just an "enhancement", it is something that needs to be fixed ASAP, and apparently, this bug has existed for 7 years! Am I wrong?
Assignee: general → nobody
QA Contact: desale → general
Target Milestone: Future → ---
Flags: blocking1.9?
Severity: enhancement → critical
Keywords: hang
I have had the same thing happen to me. Luckily this specific bug has recently gotten some recognition by online security consultants:
http://www.internetisseriousbusiness.com
also:
http://traceurl.com/go/144/Futurism

(In reply to comment #94)
> http://www.internetisseriousbusiness.com

both goes to same site.
May be who all want this bug to be fixed should popularize 
http://www.internetisseriousbusiness.com
to bring attention to all security consultants...

Note that the loop isn't started unless you click the "start loop" button.
Yes, yo can kill the tab using Ctrl-W if you know that. But most users won't figure that out. There should be an obvious way to kill the script, like there is 
with Opera.

Pressing OK and Control-W repeatedly (as described in Attachment 295358 [details], at least when tested using that attachment) is not really a usable workaround.  I could not get it to work if I tried to do it slowly (so as to avoid closing extra tabs), and then when I tried to do it fast, it still took a LONG time, and then when it finally did work I ended up accidentally closing Firefox entirely.

As it was not considered a good solution to just let the close window close the modal popup, but still block the javascript, because some platforms do not support the close window option, perhaps just enabling Ctrl+w to close the modal window, but still block the javascript would be better, and perhaps even simpler to implement.  If the blocking of the javascript is the reason why not, then just pass the ctrl+w through and close the tab causing the problem, this would solve the issue.

Though arguing that some platforms do not have a close windows button, seems to be a weak argument, since afaik 95+% of platforms do support this option, as afaik all windows, most Linux, and freeBSD platforms all do support it, so the problem would be solved on the commonly used platforms.  (NB I do not know about Mac, as it is years since I've used those systems), then there is the smalle problem of solving it on those platforms that do not have a close window on the modal dialog - Perhaps just add a button?

The ok and Ctrl+w option seems to be a kludgy way of doing it which is not obvious to non-technical users.  It however does seem to work, clicking ok rapidly and pressing Ctrl+w does indeed close the tab, however you may loose more than one tab.

However, considering this bug has been left for many years and called a request for enhancement, makes me think this bug is more a victim of politics rather than technical issues!  

I cannot envision why this particular problem should be so difficult to solve or implement a workaround for - in the worst case adding a button to the modal javascript alert box, which says stop the javascript (release the gui), or in the best case using the close window button and/or ctrl+w to pass through to the parent, closing the parent.
All I hear is talk of 'modal this' and 'CTRL+W' that (which doesn't work anyways, try it on my link in a new window). Perhaps some sort of 'Rick Roll detection algorithm', now were talking! 

Does any of this help the fact that I have had that bloody song from Rick Roll stuck in my head for 3 days?

Somebody please fix this bug! Think of the children that are being exposed!
(In reply to comment #94)
> I have had the same thing happen to me. Luckily this specific bug has recently
> gotten some recognition by online security consultants:
> http://www.internetisseriousbusiness.com

That site is SFW, but what if the loop is hawking spyware, or shooting porn pop ups? Isn't the point of FF to stop those things?
(In reply to comment #101)
> That site is SFW, but what if the loop is hawking spyware, or shooting porn pop
> ups? Isn't the point of FF to stop those things?

Or worse, please see my comment #71 for IMO how someone could easily, with the right timing, exploit this problem to grab website passwords or other sensitive information.

This should be tagged as security.
As Jason said in comment 72, that's not this bug.
(In reply to comment #103)
> As Jason said in comment 72, that's not this bug.

It is all related. If the dialog was not window-modal both problems would be solved.


This would be a great feature to add - but will not block release -> moving to wanted.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Keyboard shortcuts (CTRL+W or CTRL+BREAK or holding ESC for 2 seconds) are not "discoverable".

Adding an Opera-style "stop executing scripts on this page" checkbox is a very intuitive mechanism for dealing with infinite alert loops.

What are the technical difficulties with implementation?
Here is a great example of this problem (do not click on it unless you are ready to force-quit your browser)

http://www.rickrolling.com
(In reply to comment #106)
> Keyboard shortcuts (CTRL+W or CTRL+BREAK or holding ESC for 2 seconds) are not
> "discoverable".
> 
> Adding an Opera-style "stop executing scripts on this page" checkbox is a very
> intuitive mechanism for dealing with infinite alert loops.
> 
> What are the technical difficulties with implementation?
> 

make "disable js" menu item in right button clicking menu. 
There really needs to be some solution to this problem. There's absolutely no excuse for Firefox to still have this sort of 8-year-old design flaw.

http://www.choose.yudia.net/
(In reply to comment #112)
> There really needs to be some solution to this problem. There's absolutely no
> excuse for Firefox to still have this sort of 8-year-old design flaw.
> 
> http://www.choose.yudia.net/
> 

What do other browsers do?
See comment #65 for a description of what Opera does.
Blocks: eviltraps
hello safari!
I ran into a malicious advertisement today that locked the browser into an infinite loop of alerts while it tried to access an Active X control.  It failed hard at the latter but the former is still an issue.  I had to open task manager and end the process in order to break out of the loop.  Mind you this was an ad for "porn tube" so I had the joy of staring at some guy's flaccid cock the whole time.

The big problem with the alerts is that they prevent a user from doing anything other than confirming the alert.  The inability to close the browser or tab is perhaps the biggest reason why this is being used maliciously and successfully. 
I believe the window modal solution is the way to go, not the "kill the javascript" way.  Alerts should be window modal (like the "should I save the password?" notification).

This is also a serious security problem as shown by commonly used code like the following.  The user must start the task manager (yuck) to kill the Firefox process, there is no other escape (Ctrl-W, Ctrl-F4, window close, etc do not work!)

function alert_vc() {
     if (confirm('Click \'OK\' to download and install media codec.'))
     {
     		setTimeout("alert_vc();",7000);
          location.href='http://codecsystem.c*m/someplace/<Trojan>.exe';
     }
     else {
          if (alert('Please download new version of media codec software.')) {
               alert_vc();
          }
          else {
               alert_vc();
          }               
     }
}
Alias: alertloops
Flags: blocking1.9.1?
I concur with Steven and Duncan that it is a serious security hole. Today, I ran into a site with an infinitely looping alert that was trying to get me to download and install an Active-X control. Unable to close the tab, I had to kill Firefox with Task Manager. Not wanting to lose my 40 open tabs, I had it restore the previous session only to have the problem reoccur. I got my cake and ate it too by editing sessionstore.js, changing the offending URL to something nonexistent, but that is not a solution a normal user will ever figure out. Many users will eventually just give up and in frustration and click Okay.
In a month, this bug will hit its 8-year anniversary. Perhaps we should have a fail-cake to celebrate?

Seriously, this has gotta be *stupid-easy* to fix, right?

1. Add a checkbox
2. Make it call the "stop scripts in this page" function

Worst-case scenario: alert() uses shared code that is called from non-page processes. In that case, we duplicate it and modify one version.

That's ALL we have to do. I don't know how to write XUL, but I know that someone who is reading this *does*.
moldymen.com is a new one I've run into.

Also, Opera has this fixed. Every alert box allows you the option to block future alert boxes from that domain.
I really like what Google Chrome does here.

Instead of showing an ugly checkbox on every dialog, it shows it only after the first dialog.  This makes a lot of sense, since most of the time, two dialogs in a row is already wrong.

Think for a moment of a use case that includes showing two dialogs in a row without any delay.  They exist, but they are ruefully rare and mostly consist of poor man's debugging.

Of course, that's not a bullet-proof fix, because a setTimeout/setInterval loop could be used instead of a regular loop (have not tested this in Google Chrome.)  Best solution to that: a timer.  If it's been less than 500ms since the last dialog, show a checkbox to prevent future dialogs.

This would of course apply to alert(), confirm(), and prompt().  I don't think any others are necessary.

Of course, content-modal vs. window-modal, etc. is a separate issue.  Even if alerts were content-modal, there's still gain to having this feature.  A simple time check doesn't seem that expensive either.

-[Unknown]
Some hostile site could get around the timer by setting its own timer to delay putting up the next dialog box only after the proposed timer has run out.  The above idea is not bad, but better have a backup, such as a hotkey that terminates scripts no matter what (maybe Control-Period on most computers and Command-Period on Mac?).
(In reply to comment #133)
> Some hostile site could get around the timer by setting its own timer to delay
> putting up the next dialog box only after the proposed timer has run out.  The
> above idea is not bad, but better have a backup, such as a hotkey that
> terminates scripts no matter what (maybe Control-Period on most computers and
> Command-Period on Mac?).

That is not a bad idea.  However, I don't think this really represents a flaw in the overall solution; a delay of 500ms gives a user sufficient time to close the tab or window without any dataloss.

Essentially, it fixes the core problem of being able to "DoS" the user.  If the site is going to spam the user, that's arguably the site's discretion.

Thanks,
Todd
We should fix this, but it's not going to happen for 1.9.1.
Flags: wanted-next+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Happy birthday!
Soon this bug will be able to buy alcohol.

There is another issue. You cannot use "restore last session" after killing fx, because it will auto open insecure page.
That problem is solved in the upcoming Firefox 3.1 where you can selectively restore pages on startup.
@ Johnny Stenback:
This is something that I'd rather call a semi-solution. One can selectively restore pages, but they're not able to avoid closing a browser because of malicious Javascript code. What if sb has open a partly filled form in one tab and is forced to close the whole browser?

I vote for:
1) Opera-like option showing in all js popup-windows making people able to stop executing scripts on the page
2) tab-modal rather than window-modal js windows

And even though I consider 1+2 the best choice, I'd also be very happy with the first one - it must be easy, REALLY EASY to implement. And also would solve most of the problems. Why is it so big problem just to agree with that and implement this?
(In reply to comment #139)
> @ Johnny Stenback:
> This is something that I'd rather call a semi-solution.

Yeah, agreed. Just sayin' that it's possible to avoid restoring pages that DOS you.

> I vote for:
> 1) Opera-like option showing in all js popup-windows making people able to stop
> executing scripts on the page
> 2) tab-modal rather than window-modal js windows
> 
> And even though I consider 1+2 the best choice, I'd also be very happy with the
> first one - it must be easy, REALLY EASY to implement. And also would solve
> most of the problems. Why is it so big problem just to agree with that and
> implement this?

I fully agree with you. The Opera solution is easy (relatively), and I'd have no problems accepting such a solution. It's a problem of resources, I don't have them, if you do, or can find them, we'd gladly accept contributed patches for this.
Easy solution, make it so that the alert() dialog doesn't over-power the main window (meaning you could close the tab causing the issue or close Firefox all together).
Riddle did UserJS that was making alert()s a kind of layer instead of a popup window.

This made Firefox stop changing tabs when alert() appears.

Probably it does work no more.

http://perfectionorvanity.com/2006/12/09/mniej-ssace-window-alert/
That is what I was alluding to in comment #50 - if all alerts that are pertinent to a tab appeared as entities within that tab, then this whole bug becomes a non-issue (as do the issues of phishing sites attempting to exploit race conditions to make their alert()s appear over another tab, etc.)
This would be good. Would be even better if tab would be closable even when alert() shown (especially making nervous when alert() prevents you from closing a page, in a joke for example).
That's what bug 59314 is about ("Alerts should be content-modal, not window-modal"), btw.
After encountering a lot of “joke” web pages that make you go through tens or hundreds of Alert boxes (with text saying “This is frustrating, right?” or something along those lines), and others that make you go through an infinite loop, I agree that this needs to be addressed.

A duck/Rick roll is moderately fun, but such “joke” pages are definitely a pain and may require you to force-close Firefox and lose important pages or content.

Pick up a solution and fix this already! This bug shouldn't have been around for eight years.

“I vote for:
1) Opera-like option showing in all js popup-windows making people able to stop
executing scripts on the page
2) tab-modal rather than window-modal js windows”

I would vote for solution #2. This is what logic dictates: JS alerts come from a specific document (displayed in a tab), and should not take precedence over general browser controls and other tabs. You should be able to close the tab, to go to other tabs, to go to the URL field and change it, to perform a search using the top-right search box, etc. The only thing those alert windows should be allowed to do is lock the tab's content.
In Google Chrome which I'm currently running the issue is solved in exactly the same way as in Opera - there is an option "prevent displaying additional modal windows on this page". JavaScript is not tab-modal, but window-modal, but it doesn't hurt as we have this option.

This is what we really need in Fx - it's simple but powerful. And should be easy to implement. From major browsers only IE and Firefox are affected...
Actually, in my testing it's more like application-modal than window-modal. A modal pop-up in one window blocks modal pop-ups and synchronous XHR calls in all windows. See bug 460366. Each window/tab should be independent with respect to the others - modal alerts should only block that portion of the UI. The ideal solution would be a separate UI "thread" for each window or tab which operates independently of the others and can't block them or the main menu bar of Firefox. 

We encountered this problem in developing an XULrunner app and ended up solving it by having each instance of the main window of our app run in its own process...but this approach is probably wasteful of memory and not desirable for Firefox.

I agree that Chrome has this particular problem solved in the right way.
Whiteboard: [sg:want P2]
Flags: blocking1.9.2?
i think opera takes are of this well with the check box to stop scripts for current page. can do the same here. all the javascript alerts/ confirm boxes are modal but have a checkbox to stop scripts after that ... a screen shot of the same is here http://picasaweb.google.com/Tushar.Kapila/F1#5310120878045178258 and a test file
I just came across a really good example of why the feck we need this.

http://sourmath.com/

Yeah, so there are a few ways to do it.  Can we just PICK ONE and implement it?  Ding, ding!  Firefox paid devs, this one over here has been waiting nearly TEN YEARS, where's the service???
My bug 486480 is the same, I guess there needs to be an option in every Javascript-generated web page dialog (including confirm, alert, prompt, etc.) to "stop all further scripts on this same page"
Yet another example:
http://wykp.pl/
It's a site where some Polish internauts that have written incorrectly "wykop.pl" get. This site's address explicity the Firefox browser behaviour which makes user unable to close the tab.

As Firefox is Polish most popular browser as for now, such sites address thousands of Firefox users in just one of plenty of countries.
Blocks: 460477
It is -totally- ridiculous that this is still an issue.
Can we set this fix as a target milestone for Firefox 3.1?  Or at least Firefox 4.....
i will pay $20 for this if there are others like that who would maybe we can work together - make a few paypal accounts to fund this. can use http://www.rentacoder.com/ escrow and find someone who can make a patch for us?

firefox is open source so they can start from the current source code and code this so its an easy patch to add back. can email me at tgkprog gm'l
Tushar, that is a good idea, but I'd recommend http://www.fossfactory.org/ rather than rentacoder.
done hope to see some of you sponsor it. i adeed $8, more in a week, low on funds right now :-)  Andrés are you a dev or user?
http://www.fossfactory.org/project/p157 done hope to see some of you sponsor it. i adeed $8, more in a week, low on funds right now :-)  Andrés are you a dev or user?
(In reply to comment #164)
> http://www.fossfactory.org/project/p157 done hope to see some of you sponsor
> it. i adeed $8, more in a week, low on funds right now :-)  Andrés are you a
> dev or user?


Dev, not mozilla-dev though.

Nice, so I've checked and we're 106 people on the CC list. If everybody puts $8 we reach $848, which is not much but at least we can start to call it a "bounty". As for me, I'll add $16 as soon as I rescue my PayPal account.
@Tushar Kapila
I added $5.15 (but I'm only a student yet, I don't earn for myself ;)).
great  Michał hope more donate too.
Attached patch a start? (obsolete) (deleted) — — Splinter Review 
I'd like to know if this would be a direction I could take for this bug, the core pieces are pretty much in place, the issue is really just using them. I'm fairly new to Mozilla's backend, so comments would be very welcome.

/me peruses through blame to get some background on nsGlobalWindow...
So, a few questions/thoughts:

1) Should I use prefs and lockout the site from further prompts entirely?
2) If prefs are the way to go, there should probably be a counter that checks if a certain amount of time (say 3 seconds) has passed since the last alert and resets mShowDisableDialog.
2a) I would then have to handle prompts onunload/onbeforeunload without the timer.
3) The prefs should probably be manageable through the Page Info dialog.

I'm thinking of just overhauling these methods entirely with a much simpler impl. that just calls a js component which makes the prompt dialog and handles all the disabling/counting, etc. This would presumably be much cleaner and much safer, is this a better direction?
I don't know how difficult this would be to implement in firefox/gecko's architecture, but would it be possible to render prompts as an overlay within the tab itself? Then there's no need to actually abort the script or have prefs to lock it out, as the site is only breaking itself, not the entire browser. This approach is, I believe, used in google chrome.
bdonlan(In reply to comment #174)
> I don't know how difficult this would be to implement in firefox/gecko's
> architecture, but would it be possible to render prompts as an overlay within
> the tab itself? Then there's no need to actually abort the script or have prefs
> to lock it out, as the site is only breaking itself, not the entire browser.
> This approach is, I believe, used in google chrome.

You are describing content modal dialogs, which is already filed as bug 59314
@Natch: I wouldn't bother with prefs, for two reasons: 1) We need *something*
in place, first of all, and people can always complicate it later, and 2)
People can just avoid the site in the future. (I've only ever seen the
alert-loop occur on malicious sites and my own poorly-written test code.)
@Tim McCormack
I don't agree with the "people can avoid this site in the future" attitude. We could comment in this way any vulnerability. People will always encounter 'malicious' sites and browser's role is to prevent them from doing any reasonable harm do its users.
@Michał: Within the scope of this bug, a "malicious site" is one that alert-loops. With Natch's patch, one of these sites causes no harm and is easy to escape, so there is no danger in just having them avoid the site in the future.

Of course, prefs could be *useful*, but not having them now is no reason to block progress.
This problem is not only a problem with script generated modal windows, it can also be triggered by http servers who request authentication, but never repond with a authentication failed but keep looping the login request.

This locks the browser into requesting a new username/password for ever untill the correct combination is supplied.

So simply stopping the javascript from running doesn't help.

Some modem/routers do this (medion hardware) but its easy to create a website which does this.
@jan klopper: This could be solved by either the content-modal approach *or* by modification of Natch's approach. As you say, though, it's not a javascript issue, so I think it should be treated separately (user needs a way to back out of an authentication dialog loop.)
(In reply to comment #179)
> This problem is not only a problem with script generated modal windows, it can
> also be triggered by http servers who request authentication, but never repond
> with a authentication failed but keep looping the login request.
This would be another bug, but the auth dialog has a "Cancel" button that will stop the loop and show the server's error message.
(In reply to comment #181)
> (In reply to comment #179)
> > This problem is not only a problem with script generated modal windows, it can
> > also be triggered by http servers who request authentication, but never repond
> > with a authentication failed but keep looping the login request.
> This would be another bug, but the auth dialog has a "Cancel" button that will
> stop the loop and show the server's error message.

Yes, but what if the server never shows you an error message but simply requests the login credentials again? (I stumbled upon a router that does this)
Attached patch patch v.2 (obsolete) (deleted) — — Splinter Review 
This includes a fix for iframes that make alert/confirm/prompt dialogs by keeping the counter on the top document. The interval between dialogs is set to 3 seconds, any two dialogs within that timeframe will trigger the checkbox.

Something to think about: should the methods return NS_ERROR_* when they're disabled (potentially stopping scripts on the page) or, as the patch here does, continue returning NS_OK? The stop script dialog will actually stop the scripts on the page in the case of while(1)alert('hi') and returning an error can anyhow be circumvented by just doing while(1){try{alert(1)}catch(e){continue}} or some such...

Either way, taking and requesting review.
Assignee: nobody → highmind63
Attachment #378235 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #379408 - Flags: superreview?(jst)
Attachment #379408 - Flags: review?(jst)
This is really only important for 1.9.0.x or 1.9.1.x being that content and chrome processes will be separated for 1.9.2, requesting wanted (maybe for 1.9.1.x at least?). This fix is pretty trivial in nature and only touches the alert/prompt/confirm methods of the global window...
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
There are so many simple solutions to this problem, that I really do not get why this is still an issue.

Solutions in the simple class, no counters needed, no settings needed.
#1 add a disable scripts to all modal js popups.
#2 add a disable modal to all modal window  - the button only close thes window and block the caller, with a sleep.

However the action of disabling should not return an error code to the caller, because that allows the caller to circumvent the disable action.   It should simply place the caller in sleep, and place a notification on the page that modals or javascript is disabled temporarily on this page.
Attached file log of irc comments from bz (deleted) — 
Going to revise the patch a bit, hopefully tonight.
Attachment #379408 - Attachment is obsolete: true
Attachment #379408 - Flags: superreview?(jst)
Attachment #379408 - Flags: review?(jst)
Attached patch patch ver. 3 (obsolete) (deleted) — — Splinter Review 
This does not fix the showModalDialog issue, that issue can't be fixed like alert/prompt/confirm (where would the checkbox go?). I'll split off another bug for that. I'm not requesting review yet, too tired to even check what I did, I'll look at this tomorrow and proceed.
If this patch allows alerts after reloading the page, does it also cover the case where a page shows an alert, then automatically reloads to show the alert again?
Comment on attachment 379843 [details] [diff] [review]
patch ver. 3

This still isn't good, specifically the new GetTop method doesn't work out well, have to use two checks instead of just one, and passing in the assertion strings. I'll change this into a macro instead. That should also prevent crashes in case it is indeed null (I've never yet managed to be in such a state) everytime it is accessed.

Comment 188 is also problematic, I'll try to see what can be done in that case. I think the only solution is to pref the choice...
Attachment #379843 - Attachment is obsolete: true
Comment on attachment 379843 [details] [diff] [review]
patch ver. 3

here's a free r-

 NS_IMETHODIMP
 nsGlobalWindow::Confirm(const nsAString& aString, PRBool* aReturn)
 {
   FORWARD_TO_OUTER(Confirm, (aString, aReturn), NS_ERROR_NOT_INITIALIZED);
+  if (myWindow->mDisallowDialog)
+    return NS_OK;

You're violating XPCOM by returning an NS_SUCCEEDED value and not setting your out value.

Personally, I'd much rather you return an NS_FAILED value which will cause some form of exception to go into the js engine which would give a better way of handling things.


>@@ -4167,23 +4206,12 @@ nsGlobalWindow::Prompt(const nsAString& 
>                        const nsAString& aTitle, PRUint32 aSavePassword,
>                        nsAString& aReturn)
> {
>-  // We don't use "aTitle" because we ignore the 3rd (title) argument to
>-  // prompt(). IE and Opera ignore it too. See Mozilla bug 334893.
>-  SetDOMStringToNull(aReturn);

I find it very strange that you seem to have removed this block.

>-  // This code depends on aSavePassword being defaulted to
>-  // nsIAuthPrompt::SAVE_PASSWORD_NEVER, which happens to have the
>-  // value 0. If that ever changes, this code needs to deal!
>-
>-  PR_STATIC_ASSERT(nsIAuthPrompt::SAVE_PASSWORD_NEVER == 0);

Well, rather *these* blocks.

>@@ -5501,7 +5549,6 @@ nsGlobalWindow::Close()
>   // (since the tab UI code will close the tab in stead). Sure, this
>   // could be abused by content code, but do we care? I don't think
>   // so...
>-
>   PRBool wasInClose = mInClose;
>   mInClose = PR_TRUE;

I'm not sure why you've removed this blank line.

>+#ifdef DEBUG
>+  nsGlobalWindow* topWin = GetTop(this, "Uh, EnterModalState() called w/o a reachable top window?");
>+#else
>+  nsGlobalWindow* topWin = GetTop(this);
>+#endif

The idea of having different calling conventions ifdef DEBUG makes me sick, please don't.

>+ScriptDialogLabel=Don't show this again for this site

You haven't defined "site" here at all.

Which is bad, do you mean "window", "domain", "page", or something else?
Attachment #379843 - Flags: review-
Attached patch Different approach. (obsolete) (deleted) — — Splinter Review 
Ok, this patch proposes a different approach, this isn't entirely ready for review yet, but I would welcome any comments. I covered all of timeless' concerns, besides the assert because the method doesn't use nsIAuthPrompt anymore and doesn't use aSavePassword at all.

I'll need to file a followup to get this into page info and the crh dialog. This uses a similar pref hack as the geolocation service.

This also fixes showModalDialog by raising a Confirm dialog if there were too many dialogs (in too short a time) on the page asking if the user wishes to disable dialogs for the site. The text was taken verbatim from Chrome, I'll get ui-review once I'm happy with the patch.
Er, just realized i should probably be using the principal uri to catch odd cases of javascript: and data: (and who knows what else) uris. I'll fix that next...
great to see a fix on the way. maybe someone can come up with a test plan for beta testers?
also there is a modest but heart felt payment in escrow for this. hope others contribute now that a fix is in the making. please go to http://www.fossfactory.org/project/p157 to contribute
Attached patch use the permissionManager (obsolete) (deleted) — — Splinter Review 
Much better, same idea as the second approach. Comments, as always are welcome. I think this is ready for review. Note, this patch will not disable dialogs for chrome, another added benefit.
Attachment #380996 - Attachment is obsolete: true
Attachment #381186 - Flags: superreview?(jst)
Attachment #381186 - Flags: review?(jst)
I don't see the reason for all the 3 second time duration checks. Does it really hurt, to show the checkbox for all content alerts starting with the first checkbox?

Opera does the same. And it would allow you to stop annoying scripts like

setInterval(function(){alert("foo")},4000);

IMO alerts are ugly by itself. Thet means if I see a page (mis)using an alert, I wouldn't want to give it a chance to show another.
(In reply to comment #196)
There are very legitimate reasons for sites to use alert/prompt/confirm. If every dialog would have a checkbox allowing the user to prevent future dialogs there would be too many "good" sites that get blacklisted (either by mistake or simply because the user didn't realize what the checkbox was for). This at least leaves the legitimate use case intact, while allowing the user to blacklist the evil ones. The checkbox ensures that the site will no longer be able to show the user *any* dialogs, so I think treading with care, instead of carpet-bombing, makes more sense.
(In reply to comment #197)
> If every dialog would have a checkbox allowing the user to prevent future
> dialogs there would be too many "good" sites that get blacklisted.

Suggestion: Two checkboxes. The first says "Stop all scripts on this page". The second checkbox is below, indented, and normally disabled, saying "Always disable javascript on this domain". While the first box is checked, the second is enabled. I also recommend having a help link next to the second, so the user can make an informed decision.

How will prefs be handled? I suppose you could create a new blacklist/whitelist similar to the cookie and software installation lists. Come to think of it, the add-on installation mechanism offers a nifty solution, where a button in the "installation blocked" banner gives the user a pre-filled UI to allow future installation prompts from the site. Similarly, you could replace the second checkbox with a button saying "Always stop JS on this site" or somesuch, with a similar action.
I would place user preference above webmaster preference in every case. alert/prompt/confirm are pretty old-school, anyway, being replaced with more appropriate AJAX notification methods.

In any case, putting the checkbox on every alert box is by far the simplest method, and it would be (practically) impossible for someone to find a way around it. If the checkbox only appears on the second and subsequent boxes, someone could (perhaps) bounce the user between two different domains to keep them trapped, resetting the box-counter every time.
I think 59314 currently should get the higher priority, as alert loops are more common and you really just want to get away from this **** page afap without having to set any preferences (Honestly, when I hit the "three gays in the bath tub"-page I don't want to read the alert dialog and the checkbox labels - I want to close it right away).
Attachment #381186 - Flags: superreview?(jst)
Attachment #381186 - Flags: review?(jst)
Attachment #381186 - Flags: review-
Comment on attachment 381186 [details] [diff] [review]
use the permissionManager

>+  static inline nsGlobalWindow *MaybeGetTop(nsGlobalWindow *aWin)
>+  {
>+    nsCOMPtr<nsIDOMWindow> top;
>+    aWin->GetTop(getter_AddRefs(top));
>+    if (top)
>+      return static_cast<nsGlobalWindow *>(static_cast<nsIDOMWindow *>(top.get()));
>   }

free r-:
MaybeGetTop has a return type, but not all control paths return a value.

please try compiling with stricter compiler / warning combinations so that you catch such things.

>+ScriptDialogLabel=Prevent this page from creating additional dialogs

minor warning: a page could try to trigger the addSidebar, addSearch, and other dialogs (if they still exist).

I wonder if instead of simply discarding the messages they should be sent to the consoleservice.
window.sidebar.addPanel("Google", "http://www.google.com/", "")

Dammit, there are just too many ways a site can abuse Firefox users. See https://developer.mozilla.org/en/DOM/window.sidebar for all the possibilities. IMHO it should not be possible for sites to do the stuff in that page. Worst part about this is that in 3.0 most of those issues are very minor:

1) Adding a search engine in a loop throws up the Stop Script dialog.
2) Adding a sidebar throws up a non-modal dialog.

In 3.5 and 3.next the dialog is modal for sidebar additions.

My solution for all of these (which I think would be better handled in a followup) is to use the popup blocker code to control them. That would enforce no more than 20, and only allow them when the user requested them. Thanks for bringing this to my attention.

Oh, and let's not forget about window.external.AddSearchProvider which is also content-accessible. Anyone got an oversized hacksaw?
Attached patch patch with warning fix (obsolete) (deleted) — — Splinter Review 
This just fixes the compiler warning, the rest have separate bugs filed.

The issue with sending it to the console can also be handled in a separate bug if that's indeed wanted.
Attachment #381186 - Attachment is obsolete: true
Attachment #382063 - Flags: superreview?(jst)
Attachment #382063 - Flags: review?(timeless)
Comment on attachment 382063 [details] [diff] [review]
patch with warning fix

so... my concern about the text is that the text will confuse the user. If i see a button which says "prevent this page from creating additional dialogs", then I don't expect to get additional dialogs if i trigger that feature :).

at this point, I'm out of comments. I wasn't going to be your r+ (and mconnor or an equiv won't be either).

But I'm passing the review flag. And yes, thanks for asking me and thanks for fixing the warning (personally i'd have done if (!top) return nsnull; return ...; -- it's closer to the gecko way.)
Attachment #382063 - Flags: review?(timeless) → review?(mconnor)
Comment on attachment 382063 [details] [diff] [review]
patch with warning fix

Need to initialize mLastDialogQuitTime. New patch shortly.
Attachment #382063 - Attachment is obsolete: true
Attachment #382063 - Flags: superreview?(jst)
Attachment #382063 - Flags: review?(mconnor)
Attached patch assertion fix (obsolete) (deleted) — — Splinter Review 
Assertion was because the TimeStamp was 0, added a check for that, should be good to go.
Attachment #382618 - Flags: superreview?(jst)
Attachment #382618 - Flags: review?(mconnor)
Flags: wanted1.9.1? → wanted1.9.1.x?
Attachment #382618 - Flags: superreview?(jst)
Attachment #382618 - Flags: review?(mconnor)
Attachment #382618 - Flags: review?(jst)
Comment on attachment 382618 [details] [diff] [review]
assertion fix

I'm defintely not a good reviewer for this patch.  bumping that to jst.  Not sure on the strings, flagging the bug as uiwanted.
Keywords: uiwanted
Attached patch minor cleanup and update to tip (obsolete) (deleted) — — Splinter Review 
This has bit-rotted a bit. Plus, took out some stuff that wasn't being used and reversed the logic in ShowModalDialog to show the prompt before the second window is raised.

I guess I'll leave the review/superreview to jst.
Attachment #382618 - Attachment is obsolete: true
Attachment #387019 - Flags: superreview?(jst)
Attachment #387019 - Flags: review?(jst)
Attachment #382618 - Flags: review?(jst)
Flags: wanted1.9.2?
Depends on: js-unwind
Attachment #387019 - Flags: superreview?(jst)
Attachment #387019 - Flags: superreview-
Attachment #387019 - Flags: review?(jst)
Attachment #387019 - Flags: review-
Comment on attachment 387019 [details] [diff] [review]
minor cleanup and update to tip

First off, sorry for the delay in getting to this, vacationing and catching up after my vacation kept me from looking at this before now.

I'd be happy to accept a change to deal with dialog spamming sites, but I don't think using the permission manager is the right thing to do here. I don't think disabling dialogs per URI is what we want. That would mean if a site that uses dialogs (sanely), and relies on them, ever gets dialogs disabled (due to a temporary problem with the site or what not), then there's no clear way for users that disabled dialogs on that site (to save their browser from being held hostage by the broken site) to make dialogs on the site work again. It would seem to me that we simply want this to be a per window state that we can set, meaning that if you visit site.com and get spammed by dialogs and disable them, next time you go back you'd need to disable them again. And chances are you won't go back. And I think we want that, to avoid problems like the one described above. I don't think we want to deal with the case where a site shows an alert on load, and shows it again if reloading etc, those situations are annoying, sure, but you can recover from them and simply not go back to those sites. If we want to worry about that type of problem, I think that's material for either a different bug, or a Firefox extension.

Here's some general notes about the patch as well:

>+// Amount of time between alert/prompt/confirm that will trigger the stop dialog
>+// checkbox
>+#define CONCURRENT_DIALOG_TIME_LIMIT 3 // 3 sec

How about "Amount of time allowed between alert/prompt/confirm before enabling the stop dialog checkbox."?

+nsGlobalWindow::MakeScriptDialogText(const char *aPropName, nsXPIDLString &aOutLabel) {
+  nsContentUtils::GetLocalizedString(nsContentUtils::eCOMMON_DIALOG_PROPERTIES,
+                                     aPropName, aOutLabel);
+}

Is this really necessary? How about just calling nsContentUtils::GetLocalizedString() directly? If not, stick this in the header and make it inline...

+PRBool
+nsGlobalWindow::ShowDisableDialog(nsGlobalWindow *aWin)

Name this ShouldEnableDisableDialog() or something so it doesn't sound like this method shows a disable dialog or something :)

- In nsGlobalWindow::IsDialogDisabled():

+  nsCOMPtr<nsIURI> uri;
+  aWin->GetPrincipal()->GetURI(getter_AddRefs(uri));

This should return PR_FALSE if uri is null, as it will be if this is ever called from chrome code.

+nsresult
+nsGlobalWindow::SetDialogDisabled(nsGlobalWindow *aWin)

Make this not be static so you don't need to pass in the window all the time.

- In nsGlobalWindow::Alert():

+  nsGlobalWindow *myWindow = MaybeGetTop(this);

Make MaybeGetTop() be a non-static method in nsGlobalWindow that returns an nsGlobalWindow* so you don't need to pass in the window. And name all the "myWindow" variables "topWindow", since that's what it really is.

+  if (IsDialogDisabled(myWindow))
+    return NS_OK;

Should we not throw when we encounter a case like this? AFAICT only broken code, or code that shouldn't run in the first place will ever trigger this, and not throwing here could hide such code rather than making it clear to developers that something's not right.

I want to have a closer look at more details in this once this gets closer, but I'd like to see the bigger issues ironed out before I do that.
As a user I agree that just the once/ per session memory is enough as a fix. If I go back to the site then dialogs should be enabled again - and I can disable them again (like its with the Opera UI).

An extension could later tell me (when I click a link or enter a URL in the address bad) that this site is on my  "annoying dialog spamming" list and prompt me to stop loading the page or stop scripts from runing even before the page downloads
Attached patch patch ver. 4 (obsolete) (deleted) — — Splinter Review 
(In reply to comment #209)
> I'd be happy to accept a change to deal with dialog spamming sites, but I don't
> think using the permission manager is the right thing to do here. I don't think
> disabling dialogs per URI is what we want. That would mean if a site that uses
> dialogs (sanely), and relies on them, ever gets dialogs disabled (due to a
> temporary problem with the site or what not), then there's no clear way for
> users that disabled dialogs on that site (to save their browser from being held
> hostage by the broken site) to make dialogs on the site work again. It would
> seem to me that we simply want this to be a per window state that we can set,
> meaning that if you visit site.com and get spammed by dialogs and disable them,
> next time you go back you'd need to disable them again. And chances are you
> won't go back. And I think we want that, to avoid problems like the one
> described above. I don't think we want to deal with the case where a site shows
> an alert on load, and shows it again if reloading etc, those situations are
> annoying, sure, but you can recover from them and simply not go back to those
> sites. If we want to worry about that type of problem, I think that's material
> for either a different bug, or a Firefox extension.

Ok, removed the permission manager part. Not even sure why it would help, as in the case of a reloading top-level window the counter would anyhow be reset.
> - In nsGlobalWindow::IsDialogDisabled():
> 
> +  nsCOMPtr<nsIURI> uri;
> +  aWin->GetPrincipal()->GetURI(getter_AddRefs(uri));
> 
> This should return PR_FALSE if uri is null, as it will be if this is ever
> called from chrome code.

Since I'm not using the permission manager I have no need of getting the uri. This code is anyhow not intended to be called from chrome so it shouldn't really matter. I don't think it's worth the effort to check for a chrome call here, so I didn't.
> +  if (IsDialogDisabled(myWindow))
> +    return NS_OK;
> 
> Should we not throw when we encounter a case like this? AFAICT only broken
> code, or code that shouldn't run in the first place will ever trigger this, and
> not throwing here could hide such code rather than making it clear to
> developers that something's not right.

I changed this to throw, but I would really like to be able to give a more constructive message than "Component returned failure code (NS_ERROR_FAILURE)...". No other dom code that i know of throws meaningful error messages, only js and security stuff. I'm pretty sure it can be done with JS_ReportError, just wanted to get your opinion first if this is necessary...

Everything else was addressed. I made all the helper functions inline, and non-static.
Attachment #387019 - Attachment is obsolete: true
Attachment #388115 - Flags: superreview?(jst)
Attachment #388115 - Flags: review?(jst)
Attachment #388115 - Attachment is patch: true
Attachment #388115 - Attachment mime type: application/octet-stream → text/plain
Attached patch patch ver. 5 (obsolete) (deleted) — — Splinter Review 
Ok, I came up with a better overall heuristic for disabling alerts/etc., along these lines:

1) Any time a popup is allowed, we won't show the disable dialog until the spam count is reached.
2) The spam count is incremented for every alert that satisfies all of these conditions:
  a) It's only incremented when we're going through the popup allowed control flow.
  b) It's incremented if the alert/etc. is within CONCURRENT_DIALOG_TIME_LIMIT of the previous dialog, otherwise it's reset.
3) Otherwise (i.e. when not in an event or in trusted code) the count is incremented, and when MAX_DIALOG_LIMIT is reached the user will be able to disable them.
4) Even if alert/etc. is disabled, an alert can still show if the popup state is openAllowed and the dialog spam count is below MAX_DIALOG_LIMIT. This can happen if there's a page that does something like |<body onload="while(1)alert(1)"> <a href="javascript:alert(1)"> Alert </a> </body>|, and the user disables it during the onload loop, then clicks on the link.

I also fixed one case (in showModalDialog) where I forgot to change NS_OK to NS_ERROR_FAILURE.
Attachment #388115 - Attachment is obsolete: true
Attachment #388158 - Flags: superreview?(jst)
Attachment #388158 - Flags: review?(jst)
Attachment #388115 - Flags: superreview?(jst)
Attachment #388115 - Flags: review?(jst)
Not blocking 1.9.2 on this, but this is likely to get fixed anyways as I'm close to having time to finish my review of the patch for this bug.
Flags: blocking1.9.2? → blocking1.9.2-
Comment on attachment 388158 [details] [diff] [review]
patch ver. 5

- In nsGlobalWindow::Alert():

+  if (IsDialogDisabled())
+    return NS_ERROR_FAILURE;

Maybe return NS_ERROR_NOT_AVAILABLE to be a bit more descriptive about this error? I don't think it's worth coding up something to explain what's going on in this case beyond the exception. Same thing in all the other methods that do this check.

- In nsGlobalWindow::Alert():

+  PRBool disallowDialog = PR_FALSE;

This variable is only used inside the if (shouldEnableDisableDialog) block below, so move the declaration inside that block please. Same thing in a few more similar places.

+  SetLastDialogQuitTime();

Could we do this in LeaveModalState() instead of spreading out calls to this method?

- In nsGlobalWindow::Prompt():

So this change changes us from using nsIAuthPrompt to using nsIPrompt, but I think that's fine. nsIAuthPrompt used to be the only prompting API we had and AFAIK that's the only reason we use it here and not nsIPrompt.

-  SetDOMStringToNull(aReturn);

You still need this.

+  inline nsGlobalWindow *MaybeGetTop()

I'm think this should be inline as you have it, but it should be named GetTop(), no need for "Maybe". It won't conflict with the existing GetTop(), different signature...

+  inline PRBool ShouldEnableDisableDialog()

But this method, and all the other inline methods below, really don't need to be inline. Move them to the .cpp file please.

+    topWindow = topWindow->GetCurrentInnerWindowInternal();
+    if (topWindow->mLastDialogQuitTime.IsNull())

It's technically possible for GetCurrentInnerWindowInternal() to return null IIRC, so null check here and other places where you call GetCurrentInnerWindowInternal().

r- until that's fixed, but this is looking good!
Attachment #388158 - Flags: superreview?(jst)
Attachment #388158 - Flags: superreview-
Attachment #388158 - Flags: review?(jst)
Attachment #388158 - Flags: review-
Attached patch patch ver. 5.1 (comments addressed) (obsolete) (deleted) — — Splinter Review 
Ok, all the comments are addressed in this patch.

I also fixed a case where the disable dialog could show with the result being entirely meaningless, as follows:

with a script such as | var a = 5; while(--a)alert(a); | if the user hits the disable checkbox then we will only allow dialogs if they're are openAllow'ed and remain _below_ MAX_DIALOG_LIMIT. Previously it was allowed up until MAX_DIALOG_LIMIT, at which time ShouldEnableDisableDialog would return true and the disable checkbox would show, but it wouldn't do anything useful.

The change is small, in IsDialogDisabled I changed the last check:
| topWindow->mDialogAbuseCount > MAX_DIALOG_LIMIT | to | >= |.
Attachment #388158 - Attachment is obsolete: true
Attachment #390693 - Flags: superreview?(jst)
Attachment #390693 - Flags: review?(jst)
If this doesn't make alpha, can it still make 1.9.2?

Also note: the MAX_DIALOG_LIMIT and CONCURRENT_DIALOG_TIME_LIMIT defines might need some tweaking. Maybe 2 seconds for the latter and 4 or 5 for the former?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Attached patch patch ver. 5.2 (obsolete) (deleted) — — Splinter Review 
Fixed two more things, the spam count shouldn't be reset when popups are > openControlled and the time stamp is under the allotted duration, it should be incremented. Also, removed the nsIPermissionManager include as it was from a different patch *sigh*.

Same questions re:changing the define values from the previous comment.
Attachment #390693 - Attachment is obsolete: true
Attachment #394384 - Flags: superreview?(jst)
Attachment #394384 - Flags: review?(jst)
Attachment #390693 - Flags: superreview?(jst)
Attachment #390693 - Flags: review?(jst)
Hi guys,


Please don't look for a really complicated / bloated solution for such a simple problem. IMHO the browser should stay slim.
Any fancier solution coud be implemented as a plug in.


The browser should just NOT hang due to badly written web sites or due to a programming error during development.

It should ALWAYS be possible to STOP a javascript of a given tab (even if an alert popped up) (with or without loops. If I decide I'm no more interested in a certain tab I should be able to stop or close  it)


I think the simplest fix from a user point of view would be:
- alerts are only modal for a tab and not for the entire window.
Thus one could easily click on the stop button in the main menu.

- another probably quite simpe fix (not messing around with the modality) would be to pop up a 'do you want to stop the current javascript' dialog as soon as the user closes an alert() instead of clicking on 'OK' (or alternatively always display a small 'abort' button in alert() windows.

- If my suggested second fix is not possible, then why not 'pause' ALL javascripts if desired, close the tab and resume all java scripts.


It would just be nice if the issue could be fixed in the next release.
It is open since 2000 and looking at the amount of feedback web developers and users would like to hav a fix.

P.S. As long as the issue is not fixed. Perhaps it could be worked around with a plugin? (allowing to abort any alert() /  confirm() )
And still, the problem isn't just javascript dialogs not being modal.

The problem also exists with http authentication dialogs.
If the users gets to a malicious server which will always request for new credentials, the browser will lock up like with a while(){alert();}

The normal way of handling http authentication is to send a forbidden header after a number of tries, but there's a whole range of cheap routers who will just keep sending requests for authentication instead. All the tricks to solve just the javascript problem will all be unneeded as soon as this authentication problem is solved.
Isn't my suggested solution for alert() not the same as for authentification???

Adding an abort option on modal windows (e.g. popping up a dialog when 'closing' an alert / auth window)

The only difference to alerts would be, that 'aborting' an authentification might affect multiple tabs (e.g. when restoring a previous session)

It should always be possible to make the browser stop doing something repeatedly if the user desires so.
Johnny, is there anything else I need to address in the new patch? It seemed to be fine modulo some comments you made, but then this fell off...
Whiteboard: [sg:want P2] → [sg:want P2][needs r/sr jst]
Comment on attachment 394384 [details] [diff] [review]
patch ver. 5.2

Not much left here, this is looking good. The one thing that we need to straighten out still is to get our automated tests (mochitest) to pass with this patch, currently they do not since they do things like open modal dialogs one after another (showModalDialog() in particular) and test various things, and with this patch we end up throwing the modal dialog asking if we should allow the modal dialog to open. We need a way to let those tests keep running. You can run the tests that trigger this problem yourself with the following command:

  python <obj-dir>/_tests/testing/mochitest/runtests.py --test-path=dom/tests/mochitest/bugs

(and to run the whole set of tests, remove the --test-path argument, and prepare to wait for an hour or so, and you won't be able to use that computer while it's running, the tests are extremely sensitive to focus etc while running, so any UI interactions while the tests run are likely to cause failed tests).

The few things that still don't look quite right with the patch are:

- In nsGlobalWindow::IsDialogDisabled():

+  return topWindow &&
+         topWindow->mDialogDisabled &&
+         (topWindow->GetPopupControlState() > openAllowed ||
+          topWindow->mDialogAbuseCount >= MAX_DIALOG_LIMIT);

Isn't the first condition sort of backwards there? I'd expect dialogs to be disabled if we have no top window.

- In nsGlobalWindow::ShouldEnableDisableDialog():

This method should probably return false if nsContentUtils::IsCallerTrustedForCapability("UniversalXPConnect") returns true, that fixes some of the problems with mochitest, but not all.

- In nsGlobalWindow::Prompt():

-  nsXPIDLString uniResult;
+  nsAutoString fixed;
+  StripNullChars(aInitial, fixed);
+  PRUnichar* uniResult = ToNewUnicode(fixed);

Don't use raw PRUinchar pointers when avoidable, use nsXPIDLString here.

+  NS_ENSURE_SUCCESS(rv, rv);

This leaks uniResult if rv is an error code. This would not be a problem with nsXPIDLString...

With those minor changes made and with a solution to the modal dialogs that now block mochitest figured out, I'll r+sr this. I can help look more into the mochitest thing as well if you're unable to. Thanks for your patience here, and sorry once again for not finding the time to get back to this sooner!
Attachment #394384 - Flags: superreview?(jst)
Attachment #394384 - Flags: superreview-
Attachment #394384 - Flags: review?(jst)
Attachment #394384 - Flags: review-
About the mochitests, the code already detects chrome privileges because popups are automatically escalated to popupAllowed for trusted documents. The problem can be one of two things, a) they're not using chrome privileges or b) they open more than 10 dialogs, I'll look into it.

About the uniResult, the signature on nsIPrompt::Prompt (the one that uses a checkbox) only allows PRUnichar and it needs to read and write to that string (it uses an inout param). I'll fix the leak though.
Attached patch patch ver. 5.3 (obsolete) (deleted) — — Splinter Review 
Fixed the leak, still using PRUnichar, the inout PRUnichar is used to first set the string in the prompt textbox (optional argument to window.prompt) and then used to return the string entered by the user.

The check for topWindow in |IsDialogDisabled| is necessary, I think, because the call to it is made at the start of the function (i.e. in alert/prompt/confirm) at which time it seems that it's possible not to have a top window (or at least not an inner window on the top window). The checking fit the pattern of all other similar calls to get the inner window of the top window.

Sent this patch to try, results are fine (modulo a completely unrelated error on Linux and Windows):

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252266088.1252272608.11345.gz&fulltext=1 (WINNT)

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252266089.1252274473.30953.gz&fulltext=1 (Linux)

Mac hasn't completed yet, but I have to run, if any errors/warnings come up on the mac box I'll be sure to report that here.
Attachment #394384 - Attachment is obsolete: true
Attachment #398983 - Flags: superreview?(jst)
Attachment #398983 - Flags: review?(jst)
Attached patch patch ver. 5.3 (obsolete) (deleted) — — Splinter Review 
Just realized what you meant about the top window, _reverse_ the logic, not _remove_. Makes a lot more sense now, updated the patch.
Attachment #398983 - Attachment is obsolete: true
Attachment #399080 - Flags: superreview?(jst)
Attachment #399080 - Flags: review?(jst)
Attachment #398983 - Flags: superreview?(jst)
Attachment #398983 - Flags: review?(jst)
Attached patch patch ver. 5.4 (obsolete) (deleted) — — Splinter Review 
Missed one PR_FALSE in IsDialogDisabled, changed it to PR_TRUE as well. Also fixed showModalDialog, I wasn't using the title label, must've gotten messed up somewhere along the way...
Attachment #399080 - Attachment is obsolete: true
Attachment #399166 - Flags: superreview?(jst)
Attachment #399166 - Flags: review?(jst)
Attachment #399080 - Flags: superreview?(jst)
Attachment #399080 - Flags: review?(jst)
Nochum, so far as I can tell your patch doesn't actually stop the script right?  It just prevents further alerts?  Just making sure I'm clear on that.  It's a good patch and I hope it gets applied.
(In reply to comment #230)
Yes, your analysis is correct, this patch does not stop the execution of the script. The reason for that (and for the heuristic of when to disable alerts) is that they are valid DOM calls that are legitimately used heavily around the web. IMHO, this calls for at least some sort of abuse threshold before killing the dialog, and even then, it could be a developer mistake, a bug in the website or a myriad of other reasons that don't necessarily mean "evil site".

In most cases this will stop the currently running script anyhow, unless it's in a try-catch block, since it returns an error code. If the website does abuse the try-catch thing, the stop script dialog will pop up after a few seconds of looping over the script.
guys,

 is this really the right way to fix it?
I think no heuristics are needed. 
The main problem is, that the dialog box is modal and not, that users 
are flooded by dialog boxes.

As long as the user has the choice to stop a script even while a dialog box is open everything would be fine
Same for authentification windows etc. 

When a user clicks on the top right corner of a dialog box/ authentification box to close it, then the user could be asked if he wants to continue or stop the script.

whether he wants to retry authentification or just stop requesting a page.

Perhaps I overlooked the explanation what would be less elegant or wrong with that approach, but to me my suggestion seems a nice way to solve this problem.
In support of what klausfpga said, this is the direction Firefox will eventually have to go, eventually. Any fancy mechanism that attempts to track how many times an alert open within a period of time will be worked around by attackers. The best solution would be to make the dialog box non-modal, and second best (but still worthwhile) would be to make it so users can exit all currently active scripts on a page.

There's no reason that the site would have to be permanently blacklisted. If the user decides they're tired of having to stop script execution on a particular site all the time, they'll stop visiting it.

I think something critical is being forgotten here: The user should be left in charge, ultimately. Even if the site has a "legitimate" reason to send a series of alerts and dialogs, the user should be able to abort it so they can go do something else, if they choose. If this makes webmasters unhappy, they'll just have to figure out a better way to interact with the user.
I think that dpk and klausfpga are right: 
heuristics are like anti-virus, they TRY to detect illegitimate actions. And here's the problem :
- I'm a web programmer and my script on my web-page is testing 10 cookies values with 10 alert/prompt... Maybe I want my script to be able to detect 15 values that would be OK, and with a bug, I accidentally test 1000 or infinite values, that block ALl TABS. With your fix, imagine that the alerts are blocked on the 12th loop, I'll search HOURS where the bug is!
- what if hackers use while(0) { alert(0); httpautenticate(); confirm(); extension installation() } with random ?

I think the best would be:
- (less important) to allow a script cancel with right click/tools menu or somewhere else, like opera/chrome.
- (really important) to disable modal: EVERYTHING in a tab must NOT block others tabs. Think with tabs like multi-task OS. Tabs on browser have become as important as programs!

Ant there's also this problem everywhere :
* I've 5 mail account on Thunderbird. When wifi is not detected, he blocks me with 5 alerts()
* when I copy files on windows, and some of them are read only or CRC errors: transfer is cancelled or you have to click XXX times on a stupid boutton.
* Sometimes, when I launch games, windows return to desktop to say : I must install spreech blabla, even if I never want it.
* On Word, EVERYTIME I make a table, it's asking me if I want to install automatic correction that I never want. (I'm on OOo now)
Johnny: ping?
also, this bug is exploited by malware authors to trick users into installing active-x controls.  so it's pretty serious.  a good solution is the page-modal dialog boxes... but i hear firefox is nowhere near getting that to work.   so another solution is needed in the near-term
if this is so serious as described above maybe some of you will contribute real money so a good dev will fix this

two of us did make a contribution. till then i have work arounds!

http://www.fossfactory.org/project/p157 done hope to see some of you sponsor
it.
wow really?  Can we use this money to execute all the asshats who are preventing good patches ever getting into the software?  The problem here is not a lack of contributors, the problem is an excess of bureaucracy.
(In reply to comment #238)
> wow really?  Can we use this money to execute all the asshats who are
> preventing good patches ever getting into the software?  The problem here is
> not a lack of contributors, the problem is an excess of bureaucracy.

I'm not sure exactly what or who you're referring to, but may I suggest you read the Bugzilla Etiquette (https://bugzilla.mozilla.org/page.cgi?id=etiquette.html) before throwing out such accusations in the future? Thanks!
Might I suggest that some fix, ANY FIX be committed for this decade old bug?  Thanks!
(In reply to comment #240)
> Might I suggest that some fix, ANY FIX be committed for this decade old bug? 
> Thanks!

A patch has been written for this bug and is undergoing review. Once the patch has met the reviewer's acceptance, it will be committed to the trunk. There are no set time frames on when the review process will be completed or when the patch will land on trunk, so you'll just have to be patient a little while longer.

Continually poking the bug won't help speed it along, so please refrain. Thanks!
blocking2.0: --- → ?
Keywords: helpwanted
I think the point of the discussion which Tushar Kapila started today is to ask, exactly what WILL speed it along?  Cause, you've gotta admit, 9 years is a little long don't ya think?
By Bugzilla standards, 9 years is merely ephemeral.
Comment on attachment 399166 [details] [diff] [review]
patch ver. 5.4

So this patch still doesn't pass mochitest, I pushed this to try just to verify what I saw when running locally (a while ago), and mochitest still gets stuck with a dialog asking whether or not to allow opening the dialog. If nothing else, we could add a pref to disable this functionality and set that pref when running mochitest, either globally or only around the tests that suffer from this problem.
Attachment #399166 - Flags: superreview?(jst)
Attachment #399166 - Flags: superreview-
Attachment #399166 - Flags: review?(jst)
Attachment #399166 - Flags: review-
Attached patch patch ver. 5.5 (obsolete) (deleted) — — Splinter Review 
So, the failures were from a recent test (one that was added after the previous patch), I've fixed that specific test by upping its privileges where necessary. I'm pretty sure it still tests what it needs to, but seeing as I can't see bug 504862, where the test was added, I can't tell for sure.

Try results:

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256467486.1256474962.5034.gz&fulltext=1 (success)

OSX: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256463604.1256472941.13047.gz&fulltext=1 (success)

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256462939.1256471037.20097.gz&fulltext=1 (success modulo random test_transitions.html failures, bug 522862)
Attachment #399166 - Attachment is obsolete: true
Attachment #408319 - Flags: superreview?(jst)
Attachment #408319 - Flags: review?(jst)
Attachment #408319 - Attachment is obsolete: true
Attachment #408319 - Flags: superreview?(jst)
Attachment #408319 - Flags: review?(jst)
Comment on attachment 408319 [details] [diff] [review]
patch ver. 5.5

Not actively working on this bug anymore.
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
How about adding the option to temporarily delay the execution of the script for 5seconds?

IMO that is enough time to regain control of the browser (close the offending tab/navigate to another site/hit 'stop' to halt the execution completely etc). This can be expanded to take into account whether the alerts (and similar modal popups) are triggered by a "fully" loaded page or "in-progress" page [loading = 3sec pause, loaded = 5sec pause] but this can be sorted later.



I read all the comments on Bug #59314 and the first ~150comments on this one (plus a few of the longer comments later on) so I apologise if I missed something somewhere but I don't believe that there are any current efforts to work on this?


For now at least (considering that this is a >9 year old issue), I consider getting "something" done is more productive than "nothing" - I hope that this will be a "happy medium".
i think a way to solve all modal blocking issues (and modal dialogs popping up from other tabs!) would be to implement some of the standard modal dialogs (alert, login/password, etc.) via HTML instead of using the Window Manager (web pages should not have control of making windows!).
This is about the possibility of interrupting an active Javascript in combination with the modal dialogs, to break these endless loops.

Bug 59314 (and similar ones like bug 123913) might help with the modal nature of these dialogs. Using html for dialogs might be bug 99583 (there are many more).
(In reply to comment #252)
> This is about the possibility of interrupting an active Javascript in
> combination with the modal dialogs, to break these endless loops.

the reason you cant interrupt the script (close the tab or hit "stop") is because there is a modal dialog preventing you from doing so, right?  my solution to this is to reduce the level of control the modal dialog has by making it contained within the page that called it by making it virtually modal.  this can be accomplished by implementing said modal dialogs in html on top (via z-index) of the page in that tab.  by doing this, all action is halting in that specific tab but you are free to change tabs or close that tab.

if you are just looking to automatically detect and stop code that gets stuck in an endless loop, you are on a fool's errand.  it's not the job of the browser to fix bad code, it's job is to execute the code it's given.  having that particular site get stuck is fine because that's how it was designed to function.  however, it's not ok when that site takes down the whole application.
Not blocking 1.9.3 on this, but I'd love to see this finalized.
blocking2.0: ? → -
Whiteboard: [sg:want P2][needs r/sr jst] → [sg:want P2][needs r/sr jst][wanted1.9.3]
At the risk of adding stop-energy to this already-old bug, I think the right long-term approach here is to make the various "modal" prompts that content can trigger be tab-modal instead of app-modal. If a script want to do |while(1) alert("haha")| it can, but the user can just close the tab or switch to another tab.

See bug 59314.
(In reply to comment #254)
> Not blocking 1.9.3 on this, but I'd love to see this finalized.

So what is it that's holding this bug back?

Natch added a fix to the affected mochitest; isn't this a desired solution? you prefer a pref?

Some detailed explanation would be desirable, so that someone might be able to fix this.
As has been previously mooted, quite obviously the correct approach here to correct the modality of prompts. Anything else is simply defective by design. If this is going to be fixed, it should be fixed correctly and not hacked around to mitigate the current pain-points.
Nick:

Spoken like a true academic.  This bug is hurting real people, right now.  It's a security issue.  It's unacceptable that this has remained unfixed for so long, fix it now, make it politically perfect later.
> Spoken like a true academic.  This bug is hurting real people, right now.  It's
> a security issue.  It's unacceptable that this has remained unfixed for so
> long, fix it now, make it politically perfect later.

I agree _completely_.

anyone who disagrees, please kindly read my well laid out argument here: http://adaptivetime.com/itsatrap.html
In a trunk version it's completely reasonable to prevent implementing workarounds so that the code isn't messed up. However, in an end-user-targeted product, like final versions of Firefox it is _highly_ unreasonable to be so conservative about workaround-like approach. An end user is NOT interested in how and why the program actually works. All they want of it is to work. So if You are not able to provide a long-lasting solution at once, implementing a workaround seems a must-have. Especially now when Firefox has an established, strong number of users and the number such malicious sites is rapidly growing.

See for example how Google works - they take what's written but when they discover something has design flaws they immediately fix it, even if it requires implementing workarounds for a while. In this way an end user doesn't even know that there has ever been any problem. That IMHO is a desired attitude.

I say it with a sad tone but bugs like this put a shadow over the quality of this Mozilla product. Especially that it's so simple to workaround and You would gain a lot more trust and focusing on Firefox fancy features instead of its few, but quite serious, flaws.
uh... i asked the reviewer, tab modal dialogs is another bug; if this patch wasn't wanted this bug would be won't fixed. I just wanted to know, what prevents THIS patch from landing.
Regardless of whether the JS-alert boxes are application-modal, window-modal or tab-modal, we should still:

* have the browser respond normally to the "quit" (⌘-q in Macintosh) function in the file menu – this is what Firefox already does – but (on the Macintosh) also respond the same way to any "quit process" signal that is sent from Activity Monitor.

* Keep the "close window" (alt-F4 in Windows or ⇧-⌘-W on Macintosh) or "close tab" (Ctrl-w in Windows or ⌘-w on Macintosh) functions in the File menu enabled (not greyed out).  There is no reason the user should be unable to kill the current tab.

* Add a new keyboard shortcut (for example, ctrl-break in Windows or ⌘-. on Macintosh) that, when activated, will halt the execution of all scripts in all
Firefox windows.

However I might also say that from what little testing I have done, it seems to me that Safari is worse than Firefox in this regard.
Workaround / temporary fix: Just two days ago I discovered AlertCheck ( https://addons.mozilla.org/en-US/firefox/addon/13176 ). 

This little extension mimics Chrome's & Opera's dialog, allowing the user to get out of the loops. I clicked on all the trap-links listed in this thread and my Fx was able to survive all of them :) (the ones stil online). 

See also bug #432687
@ Carlos Alén Silva
Thanks! :) I've waited for it for so long...
Sorry for spamming, but IMHO 11,633 downloads for a week is good enough to include such small extension in default distribution.
(In reply to comment #264)
You're welcome :) 

Workaround / temporary fix 2: 
I just found there's also RightToClick ( https://addons.mozilla.org/en-US/firefox/addon/12572 ), which let's you escape javascript loops and also re-enables right click in sites that disable it.
status1.9.1: ? → wanted
Flags: wanted1.9.2?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
status1.9.2: --- → ?
status2.0: --- → ?
Anyone able to own this bug and fix it in the new world that exists (or soon will) since it was filed?

/be
Another "trick" used by malware sites recently is to try to open bogus URLs in a loop, presenting the user with a "this URL is not valid" or similar message. This message is modal too, and cannot be disabled by the above-mentioned plugin.
"Sorry for spamming, but IMHO 11,633 downloads for a week is good enough to
include such small extension in default distribution."

popularity != quality.
Longpoke, that was an extremely thoughtless comment. If you’re unhappy with the quality of something, then by all means make constructive suggestions on how to improve it, but your remark was dismissive and unhelpful.
(In reply to comment #271)
> Longpoke, that was an extremely thoughtless comment. If you’re unhappy with the
> quality of something, then by all means make constructive suggestions on how to
> improve it, but your remark was dismissive and unhelpful.

I didn't imply anything about the quality, I just pointed out that his statement was wrong, especially when it comes to software.
Google Chrome has apparently already solved this, by adding a check box to stop javascripts on a page, when popups appear fast, at least I've seen it a few times, when I was debugging javascript.

That should not break any standards at all, let it operate normally, but if a second modal window appears within a short period (I think it was 10 seconds), then add a function to disable javascript in that dialog.
(In reply to comment #273)

See attached screenshots of Opera and Chrome. See also comment #263 and comment #266. 

It is obvious by now that this issue can be dealt with in a pretty straightforward manner. It is time we get a patch.
Opera 10.5 uses a tab-modalbox, for me it's the better solution.
>uiwanted

Let's get the checkbox in place just in case we don't have fully tab modal dialogs working for Firefox 4.  This is something that keeps coming up as a significant problem with the user experience of the Web, so we need to solve it regardless of how perfect the solution is.
Keywords: uiwanted
(In reply to comment #276)
> >uiwanted
> 
> Let's get the checkbox in place just in case we don't have fully tab modal
> dialogs working for Firefox 4.  This is something that keeps coming up as a
> significant problem with the user experience of the Web, so we need to solve it
> regardless of how perfect the solution is.

hold on there cowboy!  we have to overthink this problem and get a solution that only works sometimes which _might_ be ready in another 10 years.  dont worry, we are half way there.
This bug is 9 years 5 months old.  We're more than half way there.
(In reply to comment #278)
> This bug is 9 years 5 months old.  We're more than half way there.

please note that i said "another 10 years"

i figure sometime in 2020 there will be a lame attempt at an AI to detect bad js.
(In reply to comment #279)
> (In reply to comment #278)
> > This bug is 9 years 5 months old.  We're more than half way there.
> 
> please note that i said "another 10 years"
> 
> i figure sometime in 2020 there will be a lame attempt at an AI to detect bad
> js.

My money's on a KB article linking to this bug and saying, "Well, at least we tried."
Guys, you do not need an AI to detect bad javascript, you just need to return control to the user, that is what this is about.

It is also why I find it insane that this has not been addressed yet.

A lot of problems would be solved by simply having a tick box that stops javascript execution in all blocking popups, then let the user decide, what is a bad javascript.

You do not need 10 years to find a solution.

similarly with the issue with window advertisements popup, allow the user to define that no page, plugin or otherwise can open a new window or tab, unless whitelisted, and popups are no longer an issue, the user has control.

You need to focus on giving control to the users, and not automation!
The AlertCheck addon at https://addons.mozilla.org/en-US/firefox/addon/13176 seems to be something that should just be part of firefox.  

Google Chrome has almost the exact alert stopper.  On the second alert a check box with the text "Prevent this page from creating additional dialogs." is added to the alert.  People seem to understand what that means and how to use it.

Can something like this please be added to firefox?
Yes, this should be built in. It will be, enough's enough! :-/

Alex was good enough to agree to take assignment and ensure the UX is great. The full fix will require some platform help from jst and friends. Any JS work, I'm pretty sure we will jump to help (I am cc'ing dmandlin and gal).

This doesn't depend on the js-unwind bug.

/be
Assignee: nobody → faaborg
No longer depends on: js-unwind
I think it might be worth pointing out that this is not just an issue of nuisance modal dialogues, which bug 59314, or even something which might be easier to accomplish with currently common window managers such as letting the dialogues be window-modal (but not application-modal) but preserving access to and functioning of menu bar [and being careful to ensure the alert doesn't cover it (on platforms where it is attached to the window rather than the screen)], would solve. There are other cases which are not covered by either the run-away script timer or any solution to nuisance modal dialogues. For instance, while I was trying to analyze an obnoxiously written page to work around an unnecessary incompatibilities with some common security and privacy extensions, it would automatically refresh itself on any manipulation to the DOM. It had functions repeating themselves with setTimeout which would check that its functions and other objects hadn't changed. It had mapped over the certain native function of prototypes of primitive classes, such as Array.prototype.toString(). It was also abusing getters and setters in ways that made correcting its behaviour excessively time-consuming. The page needed JavaScript enabled to fully load, but not after that for anything necessary. If I could stop all of the page's scripts from continuing to run after it had loaded, I could easily solve the problems with the page by running some script of my own. Even if I couldn't run my own JavaScript on the page after stopping its own (which might be a more complicated feature to implement), just being able to stop them after they had stopped serving a useful purpose would be enough to gain some level of functionality without disabling the extensions with which it was forcing an artificial incompatibility.

The ability to stop all currently running scripts (including forgoing any pending setTimeout or setInterval functions, and inhibiting event handlers, and getters and setters) would not just be useful for debugging certain problem pages, but for web applications which react repeatedly to being changed for useful reasons as well (such as a web application which is reacting in complex ways to changes contentEditable elements as part of drawing up a technical document).
Will we see this any time soon? Websites should not be able to hijack the browser. Recently, someone wanted to play a joke on me and send me a link:

http://www.own3d.es

(Warning: the link exploits the browser's inability to close the tab by popping up a dialog, resulting in making you see content you might not want to see.)

Needless to say, I was *very* angry at Firefox for giving websites this kind of control over my application windows. I was forced to open a terminal and "killall firefox". I can't think of a reason why I'm not able to disable that behavior in the JavaScript settings.

The sooner we have a fix for this, the better. It is getting annoying, and more "you got owned" pages pop up on the net exploiting this. And the problem is, we are indeed getting "owned." This needs to stop.
(In reply to comment #286)
> Will we see this any time soon?

Could we see it soon, yes, as soon as this very day!  Will we, of course not!  Nikos, what you need to realize is that the mozilla developers have better things to do than think about us little people.  For example, they might want to level up on World of Warcraft or add people to their friends list on facebook, pretty much anything but do what they should do.


> Needless to say, I was *very* angry at Firefox for giving websites this kind of
> control over my application windows. I was forced to open a terminal and
> "killall firefox". I can't think of a reason why I'm not able to disable that
> behavior in the JavaScript settings.

i have added these two plugins to my own collection, one to let your stop popups and the other to let you click when people do mean things like popups to prevent you from clicking somewhere.

RightToClick - https://addons.mozilla.org/en-US/firefox/addon/12572/
AlertCheck - https://addons.mozilla.org/en-US/firefox/addon/13176/



thanks again to all those mozilla developers doing jack **** about this for about a decade!
(In reply to comment #287)
Gravis: Your comment is not helpful. Please see https://bugzilla.mozilla.org/page.cgi?id=etiquette.html - if I see you doing something like this again, I'll have your account removed.
(In reply to comment #288)
> (In reply to comment #287)
> Gravis: Your comment is not helpful.

on the contrary, i give Nikos a workaround for this unfixed bug and a couple of laughs.  i hope you can understand our frustration and learn to laugh at yourself.  if you could help the resolution of this bug actually be added into firefox, that would be super!

thanks! ^_^
"If you see someone not following these rules, the first step is, as an exception to guideline 1.4, to make them aware of this document by *private* mail. Flaming people publically in bugs violates guidelines 1.1 and 1.3."

I think the one pointing at etiquette should know what he's talking about in the first place. And I found Gravis' comment helpful as he gives the way to go around this bug, even if the way he expressed his disappointment. Well earned disappointment, as this is over 10 year old critical bug.
Everyone please chill. This bug will get fixed or I'll become a pirate and sail off into infamy, fight for the little people, and drink hard(er).

/be
I was just reading Slashdot and bugtraq today about a so called full disclosue of a Microsoft vulnerability, released by a "Google research engineer" after 5 days of no patch from MS. Sort of blackmail because a fix wasn't released fast enough.

We already have the full disclosure, but not the exposure. All it takes is for someone crazy to start a "Crash Firefox day" group on Facebook and distribute simple scripts to insert into innocent looking places.

This could possibly destroy all credibility of Firefox in most corporate and government organizations. 
Luckily nobody does that. (And seriously, I absolutely don't want this)

A semi-philosophical and rhetorical question:
How smart is it to leave a ticking time bomb for this long and not push a fix sooner than later? And this goes of course for everything that is a remote denial of services attack which will cause loss of unsaved work.

I can say I've been hit by this more time than I can count, add-ons such as NoScript and AlertCheck are absolute life savers.
Sorry for ranting here.
Seppo, this is not a discussion board. Please don't add any comments that don't help to solve the bug in itself. Or do you have a patch that you want to share with the world ? I refer to comment 291 (look up on Wikipedia who Brendan Eich is).
Jo, there's been dozens of patches put forward to fix this bugs over the years.  The Mozilla bureaucracy prevents any of them getting accepted.
I use a proper add-on so therefore I can safely chill.

@Jo, I know very well who Brendan is. Even if he was God of JavaScript himself, the fact remains "chilling" can be costly in times like these where bad reputation spreads like wildfire and gaining back that lost market share can become impossible once the sh*t hits the fan. Chill and wait or take a proactive response.

I'm sorry for again breaking netiquette and replying in this thread. From now on I will take this to an appropriate discussion board and not revisit this anymore.
> I refer to comment 291 (look up on Wikipedia who Brendan Eich is).

What a monumentally misplaced appeal to authority :)
Arguments from authority are not what I had in mind with comment 291. I was trying to appeal to the common goal of getting this bug fixed, which is less likely if there are noisy and abrasive comments. I was also throwing my support behind this bug getting fixed, where my authority may count for something, and where I also may be able to help with code-level issues.

But in any case, non-coding talk that amounts to sarcasm or posturing in bug comments do not help get this bug fixed.

Gravis, you were way out of line and you added nothing of value in all of comment 277, comment 279, and even comment 287 which repeated links to add-ons mentioned already in comment 263 and comment 266. You seem like the model of a "poisonous person" (http://video.google.com/videoplay?docid=-4216011961522818645#). I'll back beltzner in revoking your bugzilla access if I see anything but usefully technical comments from you.

Seppo, bugzilla bugs are not "threads" in a "discussion board". Frustration is no excuse. I'm annoyed this bug is still unfixed, but I'm telling you, you are not helping. My comments at this point aren't helping either, so I'm going to shut up and work backstage to help get a reviewable patch up. But I have one request flag to set here:

This is clearly not just a hot button issue for a few people -- it's a common enough usability bug (developer accident as much as malicious DoS attack), and it is a competitive issue. I think it should be fixed for Firefox 4. Nominating blocking.

We haven't heard from faaborg since comment 276, and it isn't obvious dolske or whoever can do what faaborg said to do saw that amid all the noise. Maybe dolske should take assignment of this bug. Probably better to file dependency bugs and do the work in them, without all this noise.

/be
blocking2.0: - → ?
>We haven't heard from faaborg since comment 276

Sorry about that, here's a general update.  As you are aware (but perhaps not the masses of people cc'd on this bug) there are two separate issues:

1) providing a check box to suppress future alerts (this bug)
2) making the alerts themselves tab modal (separate bug, which has a work in progress patch from dolske)

These don't necessarily have to land in sequence, although I think we should get both done before Firefox 4, since even with a tab modal alert you may wish to view or interact with content after the first alert is fired.

I have mockups complete that I need to post them to dolske's bug when he is ready to implement the UI, essentially we are going to be using the same panels we are creating for the rest of our notifications, but placed in the center of the content area.  I need to ping Neil about making some changes to our panel support to allow for this (we can get it to move with its parent, but we need an attribute to control this, details in bug 554925)

For this bug, someone is going to need to take up the patch to land the checkbox (which could appear in either a window modal or tab modal dialog box).  I spoke to jst about it had he indicated that aside from tweaks to the code and getting tests to pass, there isn't a more fundamental reason why it can't land.

>amid all the noise

indeed, sorry for not commenting more regularly with updates.  It's not that work isn't happening, just that this bug has turned into more of a ranting message board and is mostly being avoided.

Also on a more personal level to the people ranting: for what it's worth we are working on the weekend, and pretty clearly not playing video games.
(In reply to comment #298)

> 1) providing a check box to suppress future alerts (this bug)
> 2) making the alerts themselves tab modal (separate bug, which has a work in
> progress patch from dolske)
> 
> These don't necessarily have to land in sequence, although I think we should
> get both done before Firefox 4, since even with a tab modal alert you may wish
> to view or interact with content after the first alert is fired.

I am currently pushing to get the tab-modal work (bug 59314) done for Beta 1 or Beta 2 (ie, within the next ~month). I think that will resolve the most common issues people have with modal prompts, though the ability to disable prompts without closing the tab will still be useful in some cases.
Tab modal will in fact be even more efficient than using check-box like opera/chrome because the attack is based on modal dialog, that is not only alert/confirm/prompt dialog but also all others modal dialog (security warning/http authentication/...)

For special cases/developpers,we can maybe add an about:config value.
blocking2.0: ? → betaN+
Attached patch Updated to tip, with various tweaks (obsolete) (deleted) — — Splinter Review 
This is an updated patch for this bug, based on the previous versions in this bug. The changes are that this stores the time limit in a pref (primarily so that we can disable this during mochitest runs), deals with window.print(), renames some things, and some other minor tweaks. This still needs tweaks to tracking modal dialog state properly while showing the print dialog at least...
Attachment #462681 - Attachment is patch: true
Attachment #462681 - Attachment mime type: application/octet-stream → text/plain
Attached patch Updated fix. (deleted) — — Splinter Review 
This is a yet more updated version of Nochum's fix for this bug. This deals with alert(), confirm(), prompt(), showModalDialog(), and print(), and this makes the time limit settable in a pref and makes our mochitest harness set that pref to 0 to disable this in our automated tests which trigger extra dialogs in certain cases.

Jonas, I've looked this over, but I could appreciate another look here.
Assignee: faaborg → jst
Attachment #462681 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #472820 - Flags: review?(jonas)
This actually needs to block beta6 since the patch adds localizable strings, and is a feature addition, in a way.
blocking2.0: betaN+ → beta6+
Comment on attachment 472820 [details] [diff] [review]
Updated fix.

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>+  if (sSuccessiveDialogTimeLimit == 0) {
>+    PRInt32 t = nsContentUtils::GetIntPref("dom.successive_dialog_time_limit",
>+                                           SUCCESSIVE_DIALOG_TIME_LIMIT);
>+
>+    if (t <= 0) {
>+      sSuccessiveDialogTimeLimit = PR_INT32_MIN;
>+    } else {
>+      sSuccessiveDialogTimeLimit = t;
>+    }
>+  }
> }

So is the intent that 0 means "don't have a time limit"? If so, leaving it at 0 or a negative value should work. So always setting sSuccessiveDialogTimeLimit to the pref value seems like it should work.

>+nsGlobalWindow::DialogBlockingEnabled()
>+{
>+  nsGlobalWindow *topWindow = GetTop();
>+  if (!topWindow) {
>+    NS_ERROR("DialogBlockingEnabled() called without a top window?");
>+
>+    return false;
>+  }
>+
>+  topWindow = topWindow->GetCurrentInnerWindowInternal();
>+  if (!topWindow ||
>+      topWindow->mLastDialogQuitTime.IsNull() ||
>+      nsContentUtils::IsCallerTrustedForCapability("UniversalXPConnect")) {
>+    return false;
>+  }
>+
>+  TimeDuration dialogDuration(TimeStamp::Now() -
>+                              topWindow->mLastDialogQuitTime);
>+  PRBool underDuration =
>+    dialogDuration.ToSeconds() < sSuccessiveDialogTimeLimit;
>+  if (underDuration) {
>+    if(topWindow->GetPopupControlState() > openControlled) {
>+      topWindow->mDialogAbuseCount++;
>+
>+      return true;
>+    }
>+    topWindow->mDialogAbuseCount++;
>+
>+    return topWindow->mDialogAbuseCount > MAX_DIALOG_COUNT;
>+  }

Just do:

topWindow->mDialogAbuseCount++

return topWindow->GetPopupControlState() > openControlled ||
       topWindow->mDialogAbuseCount > MAX_DIALOG_COUNT;


>+nsGlobalWindow::IsDialogDisabled()
>+{
>+  nsGlobalWindow *topWindow = GetTop();
>+  if (!topWindow) {
>+    NS_ERROR("IsDialogDisabled() called without a top window?");
>+
>+    return true;
>+  }
>+
>+  topWindow = topWindow->GetCurrentInnerWindowInternal();
>+
>+  return !topWindow ||
>+         (topWindow->mDialogDisabled &&
>+          (topWindow->GetPopupControlState() > openAllowed ||
>+           topWindow->mDialogAbuseCount >= MAX_DIALOG_COUNT));
>+}

Why openAllowed here, but openControlled in nsGlobalWindow::DialogBlockingEnabled?

Also, it would be nice to have better names for these functions. There is no intuitive difference between 

DialogBlockingEnabled
IsDialogDisabled
AllowDialog

Based only on the names I would have expected the first two to always return the same thing, and the last to always return the opposite of the others. At the very least, document them thoroughly in the header file.

Maybe rename DialogBlockingEnabled to DialogOpenAttempted and AllowDialog to ConfirmDialogAllowed.


Also, some more tests would be nice.

r=me with that
Attachment #472820 - Flags: review?(jonas) → review+
Why does this depend on bug 391834?
Whiteboard: [sg:want P2][needs r/sr jst][wanted1.9.3] → [sg:want P2][has review][wanted1.9.3]
I don't see how it does, removing dependency.
No longer depends on: 391834
Attached patch Followup fixes. (deleted) — — Splinter Review 
Jonas, this adds tests, adds a few changes that are needed to be able to test this (more Enter/LeaveModalState, use the prompt service, don't get a prompter that's already been created), and renames and comments a few things per your comments.

This does not explain openAllowed/Controlled/Abused, I have not had the time to investigate their meaning, and probably won't have time for beta7 to do that.
Attachment #475374 - Flags: review?(jonas)
Comment on attachment 475374 [details] [diff] [review]
Followup fixes.

>+  // Returns true if we've reached the state in this top level window
>+  // where we ask the user if further dialogs should be blocked.
>   bool DialogBlockingEnabled();

I still don't think this function name is very descriptive since there is no indication that it will increase the count of dialogs that have been opened and stuff like that. This is also not mentioned in the comment. Something like

// Call this when a modal dialog is about to be opened.
// Returns true if we've reached the state in this top level window
// where we ask the user if further dialogs should be blocked.
bool DialogOpenAttempted();


r=me with that.
Attachment #475374 - Flags: review?(jonas) → review+
Attached patch For checkin. (deleted) — — Splinter Review 

        Attachment #476153 -
        Flags: review+

    
    

    
  
Marking FIXED. For any followup issues, please file *new* bugs and mark them dependent on this bug!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
How come the ' Description of how to close a tab with a running endless alert loop.' attachment still breaks this?

    
    

    
  
bthaxor, *please* file a separate bug with appropriate steps to reproduce what you're seeing etc. This bug by no means covers every aspect of this problem, further issues should be tracked in individual bugs.

    
    

    
  
Sorry, I just thought it would be best to post this here since the attachment exists in this bug - will do.

    
    

    
  
MAX_DIALOG_COUNT = 10 is only showing the check box at 12th prompt

javascript:i=1;while(1)alert(i++);

I wish I am able to see the check box in the very first prompt.
Or at least show it in the 3rd prompt. 12th is too long to wait.

Or provide a about:config item.

    
    

    
  
Why 12 and not 2 or 3 ?

    
    

    
  
Again, please file new bugs on details surrounding this. This bug is FIXED.

    
    

    
  
For evil website there is a technique to circumvent this bug fix
see attachment 476707 [details] at bug 597934
Blocks: 597934

    
  
Blocks: 598226

    
  
Blocks: 598231

    
  
Blocks: 598246

    
  
Blocks: 598536

    
    

    
  
(In reply to comment #318)
> Why 12 and not 2 or 3 ?

Submitted 
Bug 598536 - 
Please show "Prevent...creating addi. dialogs" at second alert/confirm/prompt

    
  
Blocks: 599662

    
  
Depends on: 600189

    
    

    
  
Thanks for fixing this in the source tree.  Is the fix expected to appear in the next Firefox 3.7 update, and/or the Firefox 4 beta?
Depends on: 611566

    
  
Depends on: 619443

    
  
Depends on: 623447

    
  
Depends on: 681505

    
  
Depends on: 633154

    
  
Depends on: 692331

    
  
Depends on: 1416761

    
    

    
      
  
Flags: needinfo?(jstenback+bmo)

    
    

    
      
  

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: