Closed Bug 311994 Opened 19 years ago Closed 4 years ago

Remove the slow script dialog

Categories

(Core :: DOM: Core & HTML, enhancement, P5)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows XP
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1654325

People

(Reporter: timeless, Unassigned)

Details

Attachments

(1 file)

MOZILLA_1_8_BRANCH patch, highly windows specific
8.02 KB, patch
Details | Diff | Splinter Review 
what's the other option [wrt the slow script dialog]?  complete UI hang for a
few mins?

the proper way to behave is to process the ui for n timeslice each time the
dialog wants to come up. so if the user feels it's slow, the user clicks the
stop button, which is processed during the fraction in which the dialog would
otherwise be present. also, if the user tries to click into the content area,
you kill the running script.

bz thinks that might be possible... just have to push an event queue manually...

i'd suggest that while a script is running on the page, do not trigger event
handlers on the page if the user moves the mouse.
I like the idea of leveraging the STOP button UI for this.  The same thing
should be used to interrupt the synchronous mode of XMLHttpRequest and
document.load.
It would be nice if close-tab and close-window commands were available too.
it turns out that my employer is interested in paying to get this fix developed
and in cvs.mozilla.org.
Can we enable the UI without also enabling the content area? What happens if the
user clicks somewhere on the webpage, or scrolls the view? Do we fire events in
the middle of the heavy JS-loop or synchronous xmlhttp-load? I think this could
potentially make webpages very confused.
VisualBasic used to have a way for a longrunning function to process events on
the queue. You'd make a functioncall like ProcessEvents() and it would then
process all the events on the queue and then return.

It's not a terribly beautiful design though.
venkman has a way to disable windows. i believe we'd want to disable the topmost
connected docshell of the same type (we can't let a slow script in one frame get
out of sync w/ another frame in the same frameset).
I don't think clicking in the content area should stop the script, it's not
discoverable and rather annoying if you merely wanted to focus the window.

Let's say we process events at a certain interval (and that interval can be a
lot shorter IMHO than the current delay before showing a dialog). Should we
worry about introducing race conditions that currently don't exist? What are
chrome and content contention areas?

When processing events, should we skip over content timer events (but don't drop
them) so they don't run while content script is running? Or would we actually
want to process them so we can potentially have content script set its own "this
is taking too long, let's kill this loop" timeout?
I think there should be some sort of clear visual indication to the user that
something is going on. Otherwise, it's unlikely that the user would know to use
the stop button when they discover that the browser is slow.
A few suggestions:
- Show the dialog, but only once per script (with a message informing the user
about the stop button)
- Show the dialog, with a checkbox saying "never tell me about this again" (a pref).
- (suggested by timeless on IRC) Use an infobar (OK for me), or an icon in the
status bar (not really visible enough, IMO) to communicate the situation to the
user.
How about changing the appearance of the throbber and/or stop button after a
script has been running for a few seconds?
FWIW, opening CZ with several or more autojoins on a slow Linux box (<~500MHz)
routinely triggers this annoyance when one of the servers is freenode, which
apparently uses script to flood the server tab.
fwiw, bug 230909 just checked into the branch a fix that would allow the
duration of "script business" to be set, via pref (dom.max_script_run_time), to
virtually infinity. Right now the default was set to 5 seconds or something.
There has been discussion in that bug about adding a "don't show me this again"
checkbox to the slow script dialog coupled with the ability to turn-off the
script check by domain.

At the least I think some kind of non-modal notification of the issue is
warranted. For certain cases (cited in the other bug) there are times when it is
neccessary to let scripts run a long time and the need to constantly click away
a modal dialog to continue either creates a hassle or in some cases completely
invalidates the work. I like the info-bar idea (like pop-ups right?), I also
agree with changing the stop button and throbber to indicate a long-running
script. I don't think a small image in the status bar (ala the lock or cookies)
is significant enough to be discoverable.

I also agree that the ability to interrupt the script via stop/close tab/close
window would be good, although I worry about problems regarding half completed
transactions, but maybe that wouldn't be an issue.

There has been a great reduction in prefs and I think the addition of a pref for
this is the wrong direction. I shudder at the thought of another "white list" of
sites that are allowed to run scripts that are slow/intensive/long.
taking bug
Assignee: general → mozilla
Status: NEW → ASSIGNED
The fix for this (currently) is going to remove the code for the modal dialog.
In its place we will process events for the UI, but only the events for the Stop
button. Not sure how this will happen. Currently looking at calling
ProcessPendingEvents() on the UI_THREAD_EVENT_QUEUE, as pushing an eventqueue
and processing those events did not work. If I can get events coming in then I
should be able to handle just the events that will stop the execution of the script.

Brendan had some issues on IRC the other day and I'm hoping he'll raise them
here, so we can decide if this is the best course of action.

It should be noted that the ability to make the dialog appear only after
EXTREMELY long periods of time is checked into the branch. The question in my
mind is which model is a better one? The ability to halt a script through the UI
without even the possibility of getting a modal dialog, or the ability to set
the "timeout" until the dialog is launched. In my mind this is a feature aimed
at power users, as I agree that "normal" scripts should never run long enough to
show the dialog. How do we make it most flexible for developers and testers?
some clarification on what timeless was requesting in this bug (from an irc
conversation w/him today)

1) the ability to stop a script 
2) doesn't not want to mask poorly performing scripts (pointing to some kind of
notification in the UI)
3) don't show the user the modal dialog
4) don't stop the script(s) from running (without adding pref-setting hacks all
over)

whoops, adding brendan

Also found that we don't seem to get _any_ events delivered to us while JS runs
(no mouse events in nsWindow::ProcessMessage() anyway), so will need to find how
to pause in the DOMBranchCallback to see if we can let them come in. trying a
sleep to see if that works...
Alright, so I can get events in, but the call currently wait()s, which means everything stops unless you are constantly mousing around on top of the window. Not ideal behavior.

Because of the platform differences and the difficulty in isolating a specific widget to allow events on it looks like the only way to enable the stopping of scripts is to allow the ESC keypress to stop scripts. It looks possible with the addition of a method to nsAppShell, something that might go over like the Titantic. 

Basically the new method would allow a caller to peek at the system message queue based on msg type and parameters. The caller can chose to process the message or just be notified it is in the queue. 

I'll post a patch when I get it working but for now I'm looking for feedback on changes to nsAppShell and impact on linux/mac. 
Changing appshell won't fix embedding.  It'd really be rather nice to actually have a fix that works for Camino too.
Working patch to interrupt a slow script using the ESC key rather than show a dialog. Only processes the ESC key after the delays already built into nsJSEnvironment::DOMBranchCallback to let scripts run for a few seconds (max_script_run_time et al)

This patch adds a method to nsAppShell and implements it for Windows only. The method signature would need to be changed to apply cross platform (to abstract lParam and wParam for example).

This patch is more to indicate the type of functionality I am thinking of. It seems very fragile to me to implement it in this fashion. For instance, there is no check for keyboard accelerators (alt, shift, ctrl, etc.) and I dread to think what this might due to accessibility and hardware that defines global keybindings (some kvm switchboxes can be configured to switch through hotkeys). 

I have also not even touched the cross platform support. I looked at the GTK2 nsAppShell and it was so different I didn't know where to begin. 

So, in short, this patch works and proves that we can process events and stop a script from running away. At least on the windows platform.

Comments?
I think that's a great proof of concept!

Abstracting things out to be cross platform is obviously needed, and embedding matters too. Maybe the right way to abstract this is to have a method on some object that embedders can provide and have a "ActionWasInterrupted" method (with some better name) and let the embedding app, not the DOM code, decide what user action causes the scripts to be interrupted. For our code, we could have an implementation that does exactly what the patch here does, and over time extend it to peek for a click on the stop button or whatever, embedders are free to do whatever they want. And we could even leave the current dialog code in there as a fallback if the embedding app (or our own apps on some platforms) doesn't provide an implementation of this new "ActionWasInterrupted" method.

Thoughts?
Sounds reasonable, although less discoverable than a dialog (don't forget that by the point this is relevant to the user, the user's impression is that the browser has hung -- his first thought would just be to End Task or kill -9 the process).
What is the relationship between this, bug 78089, and bug 230909? Seems like there should be at least dependency, if not dupe.
Hi, guys I just created this Feature request entry which is kind of related 

https://bugzilla.mozilla.org/show_bug.cgi?id=343464

Its about creating a list of scripts/extensions/urls that you want to run longer than the dom.max_script_run_time is set to.

Don't know how feasible this would be though
Assignee: mozilla → general
Status: ASSIGNED → NEW
bug 78089 is about improving the text of the dialog.

this bug is about removing the dialog entirely. they are not at all duplicates. they're not even related. you can fix that bug before fixing this one. if you fix this one, the other becomes something of an orphan bug (wontfix against the last branch that still has the dialog).

bug 230909 was about adding a button to the dialog. as with bug 78089, it could be fixed before this bug is fixed, but not really after since it doesn't make sense. as it happens, it was resolved. but again, there's absolutely no relation, other than the fact that they all touch one area of code (that shows a certain dialog).

adrian.sweeney@mills-reeve.com: Bug 343464 relates to bug 230909 comment 15 and not this bug.
Assignee: general → nobody
QA Contact: ian → general
* The dialog should continue to run the JavaScript script in the background while the dialog is displayed (not suspend it waiting for user input).  If the script completes while the dialog is displayed, then the obsolete options should gray out and an automatic message appear saying "The script has finished in the background, this window will dismiss in 10 seconds." which counts itself down.  This is so an unattended refresh does the correct thing!

* The user should be given the more complete options:
 * Stop Script
 * Continue a while longer
 * Continue until Finish this time only
 * Configure JavaScript execution options (a shortcut to some options dialog)


Long term fixes:

* Fix Mozilla so that webpage JavaScript's are run in their own thread, which leads into allowing separate tabs to have their their own JS threads as necessary with potential for a JavaScript execution worker threadpool.

* When JavaScript is table to execute outside of the UI thread, then while JavaScript is running an excessive amount of time, an icon should appear at the bottom right hand corner of the browser pane indicating that it is still executing.  This would also allow the user to right click on it and perform actions like the popup dialog does.

It might be useful to add the total amount of execution time (and possible rough memory footprint as a gauge) to a new "Page Info" tab "Script".  Which might also track useful info in relation to the number of AJAX requests made do far in the future.


Sorry to hijack the bug report but the feature is useful, your time-slice sounds great (anything that simulate per-tab JavaScript execution concurrently will do, if real threads can't be used just yet) would be better implemented with the above suggestion IMHO!

> * The dialog should continue to run the JavaScript script in the background

The point of the dialog is to keep the script from freezing the UI.  If we could do what you suggest here we wouldn't need the dialog at all.

> * Fix Mozilla so that webpage JavaScript's are run in their own thread,

There are existing bugs on this.  For that matter, there are existing bugs on all the other things you mention.
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML

We now offer to stop scripts in the child process via a non-modal notification, and the modal parent-process one was disabled in bug 1654325.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: