230909 - ability to disable "this script is running slowly" dialog

Status: RESOLVED FIXED

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

Description

[Robert Ginda]

There are (at least) two cases where I'd like to disable this dialog...

1. When the script in question is part of a XUL app.
   Venkman has cause to run code that triggers this dialog.  If the user
   selects the wrong option, Venkman gets all confused.  Chrome authors
   should be allowed to shoot themselves in the foot here.

2. When profiling code in a web page.
   This came up in the jsdebugger newsgroup today.  Someone is has created
   a series of stress tests, and is attempting to use Venkman to profile
   them.  When this dialog comes up it invalidates everything.


The code that pops up the warning dialog is here:
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#403

Comment 1

[Robert Ginda]

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

Comment 2

[Mikko Virkkilä]

I guess this should be a hidden pref? 

I just made a patch for that part of the code in bug 145523, so I think I can
give this one a shot if no one objects too much.

Any suggestions as to what the pref should be called?

Comment 3

[Mikko Virkkilä]

Something like the following could be inserted after this line
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#435

nsresult rv;
nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));
if(NS_SUCCEEDED(rv)){

  PRBool skipDialog;
  if
(NS_SUCCEEDED(prefs->GetBoolPref("javascript.hide_kill_script_dialog",&skipDialog))){
    if(skipDialog)
      return true;
  }
}

Comment 4

[Nicholas Allen]

Having a pref would fix case #2 (although profiling is often easier in the
standalone js shell) but it doesn't fix case #1.

Comment 5

[William J. Edney]

I'd like to toss my hat in the ring here and support a fix for this.

A question I have, though, is under which conditions does this get triggered?
CPU busi-ness, stack depth?? We tend to see it more often than we'd like when
debugging TIBET code under the V-man. We tend to get deeper stacks than most
apps (probably 12 frames on average, maxing at around 25-30 frames).

It would be really nice to be able to turn it off, from script as well as
setting a pref.

Cheers,

- Bill Edney
- Technical Pursuit Inc.

Comment 6

[Nicholas Allen]

bedney-

Currently this dialog is shown after a certain number of callbacks are made from
the JS engine to the DOM code.  I'm in the process of changing it to show based
on wall clock time.

Comment 7

[Albert Feller]

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

Comment 8

[Brendan Eich [:brendan]]

So nallen and jst whacked the DOMBranchCallback, and now it uses wall-clock time
and a generous bound.  Is this bug still a problem?  Hannibal says so:

<Hannibal>
http://lxr.mozilla.org/mozilla/source/extensions/irc/js/lib/command-manager.js#179
<Hannibal>
http://lxr.mozilla.org/mozilla/source/extensions/irc/js/lib/command-manager.js#235
<Hannibal> for me, these got triggered in a debug build of XULRunner
<Hannibal> on a slow machine
<Hannibal> I can't guarantee they're a die-hard testcase :(
<brendan> ok
<Hannibal> brendan: console flooded = teh slow
<Hannibal> merely saving console output to a logfile fixed the problem :s
<brendan> what's flooding it?
<Hannibal> warnings that those .key, .tip and some other strings for commands do
not exist
<Hannibal> they only exist for a few commands, all the other ones cause spewage
<brendan> strict js warnings, or something from string bundle code?
<Hannibal> string bundle

/be

Comment 9

[Karsten Düsterloh]

In our firm, we're using a JavaScript/chrome based TCP connection that kept
constantly triggering this dialog so often that I finally commented it out the
entire trigger code to make it shut up...
I'm not in the office for a few more days and so I can't test recent changes
right now, but a pref to keep it quiet would definitely be good thing(tm).

Comment 10

[William J. Edney]

Brendan -

Yes, it continues to be a problem for us, but less so than before.

I have two suggestions here:

1) Note https://bugzilla.mozilla.org/show_bug.cgi?id=247225. This bug is
preventing folks from setting the 'dom.max_script_runtime' parameter from
about:config. Something about the preference not being dynamic - I confess I
don't understand that part of the Mozilla codebase well enough to understand the
difference. Fixing this bug to allow that would be a good start.

2) In an email to you on 2004-10-15, I mentioned the following. You said you
thought it was a good idea, so I'll reopen the issue by quoting myself here:

---------------------------------------

Something that we would propose that would help us here in TIBET-land, though,
is the ability to have a preference such that the user can 'turn off' this
message for a particular domain or code source, much like how the dialog works
for turning on "UniversalBrowserRead", etc.

E.g. "A script on this page is causing mozilla to run slowly....blah blah... To
not see this dialog again for this domain
"http://www.technicalpursuit.com/tibet...", click here." And then have a
'Remember this decision' checkbox too.

---------------------------------------

I'll throw that one into the mix for discussion.

Thanks for looking at this issue!

Cheers,

- Bill

Comment 11

[Scott Shattuck]

I'd like to add my strong support to the request that any "final solution"
include the ability to turn the dialog off via the dialog itself as mentioned
below. 

In particular I think we have to have a way to allow mozilla-based applications
to prompt the user a single time per application execution and have the user
click "It's ok (it's my corporate timecard app thanks)" and "Remember this
decision".

That's the only way I can see us continuing to build true business applications
that don't annoy the user while leveraging the real power in the mozilla platform.

Comment 12

[timeless]

fwiw we recently had a spider which was thinking about 25000 document.links, it
hit the warning every 5000 or so, as it happens, our code wasn't very smart, so
i'm kinda thankful for the warning. oh, and of course our code does have the
ability to automatically respond to the warning dialog (although we haven't gone
as far as doing that, but we might), so i'm not as concerned as most of the
people here (perhaps having a callback which universalxpconnect consumers can
use to answer this question would be nice for the rest of the people involved).

Comment 13

[Brendan Eich [:brendan]]

Turning off the dialog from the dialog is cool, we should do that.

Pref'ing this off may be good, but let's not hide real bugs in our code where we
fail to reset the start time when we should.  JS that executes for whole seconds
is probably doing something wrong, or we are failing to note script "start"
points in our native code.

/be

Comment 14

[Johnny Stenback (:jst)]

Attachment: Add a "Never show this dialog again" checkbox.

This adds a "Never show this dialog again" checkbox to the dialog and makes
that change the value of "dom.max_script_run_time" to 0, which will now make us
not show the dialog again.

Comment 15

[timeless]

the never option should really be restricted by domain (where chrome://foo/bar
is a domain). I'd be happy to never show the dialog for chatzilla, but i want
the message for www.evil.com

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 179227 [details] [diff] [review]
Add a "Never show this dialog again" checkbox.

timeless: you wanna show the patch to use localstore or whatever to memorize
the choice per-site?  We were trying to keep this simple.

jst: any chance you could use LL_INIT(0x4000000, 0) instead of 0 for
sMaxScriptTime when disabling the check, and avoid the LL_IS_ZERO?  With 64-bit
usec time format, it would take 146235 years from the epoch (zero hour 1970) to
make t + 0x4000000000000000 overflow into a negative PRTime.

/be

Comment 17

[William J. Edney]

Guys -

I'd also like to cast my vote for 'per site' (as per my email). Otherwise, folks
are gonna be reluctant to switch it off on the sites that they need to since
they'll want to have the option of switching it off for 'evil' sites, even after
they've visited the slow, but non-evil site.

Of course, since I'm not doing the work, this is easy for me to ask for:-).

Thanks guys!!!

Cheers,

- Bill

Comment 18

[Brendan Eich [:brendan]]

Typo:

> jst: any chance you could use LL_INIT(0x4000000, 0) instead of 0 for

I meant 0x40000000 of course.

/be

Comment 19

[Hixie (not reading bugmail)]

If there are bugs that mean this warning is triggered incorrectly then we should
of course fix them, but other than that I really don't think we should have a
way to disable this.

If you have JS that is running for so long that it triggers this warning, that
means your UI is locked for long periods of time. That is very bad UI, and
indicates a bug in that JS. IMHO.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to comment #19)
> If you have JS that is running for so long that it triggers this warning, that
> means your UI is locked for long periods of time. That is very bad UI, and
> indicates a bug in that JS. IMHO.

Well, the reasons rginda gave in comment #0 seem valid to me, and it's also
possible that code that usually does not trigger this warning normally starts
doing so when using debug builds, for example (I had that when a debug started
loads of messages to the prompt (console)). Which is plain annoying.

Comment 21

[timeless]

the reason that debug builds cause this is that while the code used to count
callbacks, it now deals in wall clock time, which is hurt by assertions and
warnings doing extra console io and similar stuff (worst case, actually
_landing_ in the debugger).

Comment 22

[Johnny Stenback (:jst)]

Attachment: Sane as above w/o the added LL_IS_ZERO() check

Same thing, with Brendan's suggestion.

I'm sortof not seeing enough people caring about making this per site to make
it worth the added code we'd need to do that, so for now I'd argue that this is
"good enough" for the developers that ever run into this. One can always use a
different profile when doing funky stuff that triggers this dialog, or
whatever.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 179330 [details] [diff] [review]
Sane as above w/o the added LL_IS_ZERO() check

sr=me, but I'm fried -- test twice, cut once ;-).

/be

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 179330 [details] [diff] [review]
Sane as above w/o the added LL_IS_ZERO() check

>Index: dom/src/base/nsJSEnvironment.cpp
>===================================================================

>+static int PR_CALLBACK
>+MaxScriptRunTimePrefChangedCallback(const char *aPrefName, void *aClosure)
>+{
>+  // Default limit on script run time to 5 seconds. 0 means let
>+  // scripts run forever.
>+  PRInt32 time = nsContentUtils::GetIntPref(aPrefName, 5);
>+
>+  if (time == 0) {
>+    // Let scripts run for a really, really long time.
>+    sMaxScriptRunTime = LL_INIT(0x40000000, 0);
>+  } else {
>+    PRTime usec_per_sec;
>+    LL_I2L(usec_per_sec, PR_USEC_PER_SEC);
>+    LL_I2L(sMaxScriptRunTime, time);
>+    LL_MUL(sMaxScriptRunTime, sMaxScriptRunTime, usec_per_sec);
>+  }

Don't we still want to ignore negative values like we used to?

>-  // Force the default for unreasonable values
>-  if (time > 0)
>-    maxtime = time;

Comment 25

[Johnny Stenback (:jst)]

Fix checked in. I made us treat negative values the same way we treat 0.

Comment 26

[Jesse Ruderman]

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

Comment 27

[Jesse Ruderman]

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

Comment 28

[timeless]

i'm so glad to see this patch didn't make the 1.8 branch, which means that when 
people say "oh, you can just turn it off by setting the pref to zero" what they 
really mean is "you can set the pref to 5 and bang your head against a wall for 
5seconds".

i'm not trying to set the pref to 0, but i can't seem to get the pref to do 
anything useful at any other value and i don't appreciate it when people tell me 
to 1) use the stable branch and 2) set prefs that don't work on that branch.

let's get this onto the branch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

jst, do you think this is reasonable branch material?  It looks to me like it
might be, but we're pretty late in the cycle...

Comment 30

[William J. Edney]

timeless,

When you say that you don't really see the pref doing anything useful, could you
be running into what I describe in comment #10 in this bug report (bug 247225)?

As I state in that comment, I'm not sure why 'dom.max_script_runtime' can't be
set through about:config (something about it not being dynamic), but in any case
that seems to me to be a bug that should be fixed along with this one or maybe
even before this one?

Sorry I keep bringing this other bug up everyone
(https://bugzilla.mozilla.org/show_bug.cgi?id=247225), but it seems to me that a
first cut of this whole problem is the ability of the user to use about:config
to set this value, rather than having to root around inside of their all.js.

Maybe I'm missing something here...

Cheers,

- Bill

Comment 31

[Scott Shattuck]

It's difficult for me to express the level of my disappointment that none of
these fixes appear to have found their way into the 1.5 beta cycles. It's
October of 2005 and this bug was filed in January of 2004, 21 months ago.

If you serious about "Mozilla as an application platform" then you _must_
provide a way to allow your platform to be configured for the needs of
applications -- and this dialog is one of the most universally frustrating
things about the mozilla platform for an _application_ developer.

Enterprise customers don't run nightly builds, they certify to major releases.
Without these fixes I'm going to have to wait for yet another major release
before I can tell potential customers that there's a way to turn off this
dialog. In the interim I can continue to tell them "IE doesn't have that
problem", but that's hardly the proper solution.

Isn't there some way to A) fix this so it responds to changes in about:config (i
can set signed.applets.codebase_principal_support so it can't be about
security), B) complete the work to make this domain-specific so applications
that are intranet-based which do "real work" can interoperate with those random
excursions onto the internet at large and C) get the current fixes put into a
build someone's actually going to get in a major release sometime in 2005?

I'll accept that perhaps on C is possible at this stage, but seriously I can't
believe I'll have a copy of IE7 before I have a fix for this damn bug.

Comment 32

[William J. Edney]

So the good news is that it looks like this bug also fixes 247225, so at least
we have a solution for our customer base to 'kick up' their threshold, but only
if this makes it into the release.

As per Scott's comments, we'd really like to see this go into FF 1.5 / Mozilla 1.8.

Is there a way to make this happen? I've offered (and delivered!) on beer for
other bugs, so I'm making the same offer here :-). In fact, last time it was for
one case of beer. I'll double that offer to two (yes 2!) cases of beer :-)!

Thanks!

Cheers,

- Bill

Comment 33

[Brendan Eich [:brendan]]

I didn't realize this missed the 1.8 branch.  I will argue for it, the patch is
an improvement.

Do we have an independent bug where the script running slowly dialog gives a
false positive, due to something not resetting the timestamp?

/be

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 179330 [details] [diff] [review]
Sane as above w/o the added LL_IS_ZERO() check

This is a clean miss on the l10n freeze, *unless* there is an existing
"Remember this decision" or "Never ask again" string -- which there probably
is!  jst, can you check?

/be

Comment 35

[Daniel Veditz [:dveditz]]

There is a existing "remember this decision",
(http://lxr.mozilla.org/mozilla/source/toolkit/locales/en-US/chrome/cookie/cookieAcceptDialog.properties#17),
but I bet the l10n folks would prefer a separate copy. Would be better for the
code than having to load a second unrelated bundle, too. "Remember this
decision" isn't really what this is doing, this only remembers a decision to
continue. For that matter it isn't quite doing the patch's "Never show this
dialog again"

I'm not keen on letting users globally disable this dialog so easily. Per site,
maybe, that'd at least limit the damage. If I'm completely outvoted and the
checkbox remains then I beg you to make it much more explicit in hopes
inexperienced users don't check it. "Turn off unresponsive script checking
(warning: if you do malicious pages will be able to cause browser hangs)"

Really, please get rid of the checkbox.

Comment 36

[Benjamin Smedberg]

Comment on attachment 179330 [details] [diff] [review]
Sane as above w/o the added LL_IS_ZERO() check

I don't think we should take this, for the late-l10n and for the reasons
dveditz mentioned.

Comment 37

[William J. Edney]

Well, as someone trying to use Mozilla to write real apps (that is,
high-throughput, transactional worker, runs-all-day, enterprise applications)
and as one of the main instigators behind this bug, let me explain my rationale
for wanting this functionality:

We have parts of applications written for enterprise customers that cause 30 to
60 seconds of processing time (that is, JavaScript processing) to occur.
Thankfully, we can put up an animated gif and, because it runs in a separate
thread, it will animate for the user. Unfortunately, if the user must then sit
there and click a button every 5 seconds to 'continue', this makes the app
almost unusable for them (otherwise, they can be doing other things and just
glance at the screen to see if the app is ready to move on).

I had initially voted for a 'per-domain' fix, but with the checkbox. That way,
our users can cancel it once for their enterprise domain, but they still get the
dialog for other domains. Our apps run with a great user experience for our
users. They still get the dialog box for other domains, which it would seem to
me to make everyone else involved here happy (or at least happier).

I was willing to accept the 'all domains' fix because we really need this to
happen. Mozilla runs our apps wonderfully and this is one of the few warts.

Cheers,

- Bill

Comment 38

[Bob Clary [:bc] (inactive)]

(In reply to comment #37)
wouldn't setting dom.max_script_run_time to a reasonably high value do the
trick? I find 120 seconds to be sufficient for all except the most extreme cases.

Comment 39

[William J. Edney]

Bob -

Well, it would except for a few things:

1) My user base is really 'basic', that is I'd like not to have to expose
'about:config' to them.

2) Even if I did go ahead and expose 'about:config' to them, there is a bug,
noted here by myself in various comments
(https://bugzilla.mozilla.org/show_bug.cgi?id=247225) that shows that this
parameter cannot be configured by using about:config. That bug has been marked
'fixed' by Boris Zbarsky with the final comment being that the bug fix here
fixes that bug. Hence, a bit of Catch-22 here.

3) I could provide a small UI myself to allow them to set the value, but then
I'd have to worry about XPConnect privileges to read and write the prefs.

4) That would still not satisfy some of the concerns here because it wouldn't be
per-domain. Once the user sets it to 120 seconds, all sites get the new setting
(although, as I stated before, this is of lesser concern to me given my
deployment environment).

Cheers,

- Bill

Comment 40

[Brendan Eich [:brendan]]

Can we have a patch that fixes the pref-changed notification bug, so Bill's
users can at least use about:config?  That would be a UI-less change.  jst's out
Monday so if anyone else can help, please jump in.  

/be

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: branch patch

This is just a port of the parts of jsts patch that allows the pref to work.

Should be very safe as it is very simple. I've verified that it works using the
testcase in bug 247225.

Comment 42

[Hixie (not reading bugmail)]

The problem is that any script that triggers this alert is going to be making
the UI completely unusable anyway. *This is bad.* It means the user can't open
another tab or anything. It even kills his other windows since we are
single-threaded across the board when it comes to UI.

The correct solution here is to make the application not take so long, e.g. by
breaking up the work into chunks and using setTimeout() to chain the chunks
together. (By doing this you can also give much better feedback about how long
it is going to take.)

IMHO this simply isn't a Firefox bug, it's a bug with the script. Enterprise or
not, if JS takes that long, it's broken.

Therefore I'd like to echo Daniel Veditz's comment 35 and ask that we back out
the checkbox from the trunk. At the absolute minimum it should be per-domain.

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 199098 [details] [diff] [review]
branch patch

too late for this kind of change on the branch. it also seems that this doesn't
accomplish what the main proponent wants and there are other complaints.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 199098 [details] [diff] [review]
branch patch

The other complaints doesn't apply to the branch-patch. Don't know if this
helps the proponent or not though, i'll leave that to him.

Removing review requests for now since this was a branch-only patch. If anyone
still want to r/sr they are welcome to.

Comment 45

[William J. Edney]

I guess I'm taking on the role of 'proponent', so that's fine :-).

I'll take this patch, even though it more properly should go with
https://bugzilla.mozilla.org/show_bug.cgi?id=247225. Maybe this patch should be
attached to that bug?

To me, both of these issues should be fixed. The fact that I couldn't set this
parameter using about:config was troublesome, which is why 247225 is a separate bug.

Beggars can't be choosers. If we can get this first patch in (thanks for the
fast work Jonas!) then I can live with that. We can then continue to debate the
merits of the dialog box / checkbox / per-domain shutoff.

But in any case, this patch fixes what I truly consider to be a bug (with the
longer-term fix being more of an 'enhancement').

Therefore, the 'proponent' would love to see this make RC1, if it can get the
proper blessings from the drivers.

Thanks guys!!

Cheers,

- Bill

Comment 46

[Brendan Eich [:brendan]]

Comment on attachment 199098 [details] [diff] [review]
branch patch

Asa, don't minus this just yet, it's not controversial. It just fixes the pref
to work as all prefs should -- to be something you can set via about:config and
have take effect immediately.

/be

Comment 47

[Brendan Eich [:brendan]]

(In reply to comment #45)
> I'll take this patch, even though it more properly should go with
> https://bugzilla.mozilla.org/show_bug.cgi?id=247225. Maybe this patch should
> be attached to that bug?

You're right, sorry this got off track -- my fault for pinging sicking asking
for a minimal fix for the branch.  Not sure it matters now, but if people like,
we can move the patch over to bug 247225.

On the issue motivating this bug, I have to agree with Hixie.  JS has a simple
execution model, run to completion, which implies not taking too long in any
script or other execution unit.  If you work around this design's limitations,
or make peace with it and become one with it (to put a better spin on things),
you'll have a better cross-browser app -- and your users will have a better
experience, without needing a controversial and potentially harmful option to
disable the dialog.

What do you say?

/be

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199098 [details] [diff] [review]
branch patch

Looks good.

Comment 49

[Brendan Eich [:brendan]]

Comment on attachment 199098 [details] [diff] [review]
branch patch

This is more like it -- it just moves the init-time code to a pref-changed
callback, calls that from init after arming it, and adds the if (time == 0)
case to set the max-runtime value very far in the future, effectively disabling
the dialog.  Again, only via about:config tweaking, but now consistent with
non-buggy prefs.

/be

Comment 50

[William J. Edney]

Brendan -

Regarding comment #47, hmmm.... As you well know, we sort of do 'extreme
JavaScript' ;-).

Our main problem was the about:config bug that you all are currently reviewing
the patch for. If that gets approval, at least I don't have to tell my users to
find their all.js file and edit the setting manually, which gives me heart
palpitations just thinking about it.

So I guess we could give on this other. Not sure if there's still issues with
the original comment by rginda that opened this bug.

If you guys do decide to back out this bigger patch from the trunk, could we
make sure that the smaller branch patch, if approved, finds its way into the
trunk ;-)?

Thanks everyone!

Cheers,

- Bill

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 199098 [details] [diff] [review]
branch patch

Checked in on branch.

Comment 52

[Daniel C]

This bug is open, but has keyword fixed1.8. Can someone add something to the status whiteboard indicating why please? It's not easy to work out from the comments.

Comment 53

[:Gijs (out for now; he/him)]

(In reply to comment #52)
> This bug is open, but has keyword fixed1.8. Can someone add something to the
> status whiteboard indicating why please? It's not easy to work out from the
> comments.
> 

From what I can tell from the comments, you can disable the warning on branch (1.8  gecko, which is what the keyword is for) by setting the appropriate patch to 0.
People are still arguing over whether we need a different fix for trunk (eg. a tickbox in the warning dialog, or a whitelist on an url basis, etc.), which is why the bug is not marked Resolved - Fixed.

That's how I read the comments, anyway.

Comment 54

[William J. Edney]

That is correct.

The branch patch here for 1.8 is a portion that was extracted from the bigger patch attached previously. It really fixes bug #247225, which was a bug that prevented the user from setting the dom.max_script_run_time parameter from the about:config (thanks again to everyone involved here! :-) )

The debate continues about the larger trunk patch, which adds a checkbox to the 'this script is running too long' dialog to 'never show it' again and to let the script run as long as it wants to.

This feature is currently being debated. I believe the only consensus reached at this current time is that, if such a thing is allowed, it be done on a 'per domain' basis, which the current trunk patch does not do.

Cheers,

- Bill

Comment 55

[Bryce Mozilla Nesbitt]

Here's a suggestion for what could be presented in user-land:
+----------------------------------------------------------+
|::::::::::::::::::::::::::::::::::::::::::::::::::::::::::|
+----------------------------------------------------------+
|  .   A script in this document has been running without  |
| /!\  pause for more than 30 seconds.  It has used 64M    |
| """  of memory.                                          |
|      Do you want to continue running this script?        |
|                                                          |
|        [Keep running for 30 more seconds]                |
|        [Stop it now] [Leave it running]                  |
+----------------------------------------------------------+

Comment 56

[u119943]

I'd like to add my vote for a tick-box to "white-list" a domain.

I reported bug #317710 which apparently is a dup of this one, and as a user I feel it would be most useful if I could just tell Firefox "I trust this domain, let it move on".

Comment 57

[u119943]

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

Comment 58

[Justin Bradford]

I've been annoyed at this dialog before, but due to the occasional script under degenerate conditions, rather than a specific script that always takes too long. When I click "Continue Running" it's because I mostly trust the script and figure it just needs a little more time, but it's annoying to have to reaffirm that every 5 seconds.

A whitelist approach would be useful for a few exceptional cases, but it's a bad thing for this occasional problem (and I suspect this is a much more common case).

What would be nice is to have the timeout increase with each "Continue" click. 5, 10, 30, 60 seconds, etc.

Even better would be, following the initial decision to continue, provide a UI to indicate that a script is running and a way for the user to stop it whenever they want. Maybe a notice bar is put up saying "A script has been running for an unusually long time." with a "Stop Script" button.

And perhaps that could be put up automatically after 5 seconds (or whatever the timeout value is), rather than even requiring the initial prompt? 

It seems this would solve everyone's problem (if you have a long running script, simply warn the user first so they don't cancel it, and then they don't have to sit there clicking continue every 5 seconds, either)

I'm not sure what the limitations are for preempting the script engine (and especially for putting up a notice bar during such an interrupt), but if you can interrupt it to put up the Cancel/Continue dialog, something like this must be possible: perhaps to cancel you have to hold down the escape key, and the current interrupt mechanism could be used to simply poll the state of that key.

Comment 59

[Robert Ginda]

That (comment #58) sounds like a great idea.  Tricky to implement, but (probably) not impossible.  From a UE perspective, it could be as simple as leaving the current dialog as is, but allowing the script to continue while the dialog is up.

The hardest part might be determining when the long running script is no longer running.  In the event that the user never cancelled the script, you'd need this to know when to make the dialog go away.

Comment 60

[u119943]

(In reply to comment #59)
> leaving the current dialog as is, but allowing the script to continue while the
> dialog is up.

I like comment #58 very much too but my gripe with the addition above is that the whole point of this pop-up is to allow users to protect themself against melicious JavaScript code (at least that's my feeling about this) so maybe a good go-between might be to suspect the script during the initial pop-up then after the user says "keep going" move the warning about a long-running script to the status line with an option to suspect it.

Comment 61

[u119943]

(In reply to comment #60)
> good go-between might be to suspect the script during the initial pop-up then

I ment "suspend", of course. Not "suspect".

Comment 62

[Justin Bradford]

I think the problem is that all of this is (and must be) happening in the UI thread, and two related (potential) problems then restrict our options:

1. I don't believe it is possible to "push the context" of the Javascript interpreter within a thread (ie. no multi-tasking primitives within the interpreter's private context itself). In other words, we can't save the state of the long-running script, run some other scripts for a while, and then return to the long-running script. Since the chrome uses Javascript, this would mean we couldn't do things like put up a notice bar or even change the status bar text during this "interrupt".

2. I believe a call into the js interpreter is completely "blocking" in the sense that the main event dispatch loop won't happen until the script completes. If so, this would mean that even making the warning dialog non-modal (assuming all of mozilla's platforms even support them), would not do us any good (the event for clicking on the dialog's "stop" button would never get processed).

We could get around the second issue by making the interpreter occasionally process the event queue, but at the very least (assuming #1 is correct), we would need to filter events to avoid triggering another call into the interpreter. I wouldn't be surprised if this required significant new platform-dependent code and also create all sorts of ugly issues for embedders.

(I'm assuming these things are true, because if not, I don't see how an endless loop in a page's javascript could cause a lock-up. At worst, it would burn CPU cycles, but the DOS angle could be easily handled by prioritizing Javascript contexts -- that is, chrome always wins. Or perhaps it is possible, but introducing a simple preemptive scheduler at this point would just complicate the expected behavior too much for "consumers" of the interpreter?)

I haven't dabbled in this particular region of mozilla code before, and the only experience I have with mozilla's event loop is from an embedding perspective (from a few years ago), so let me know if I'm totally wrong here.

I suspect the only realistic "non-blocking" approach would be:

1. The current modal dialog (after 5 or x seconds) with a new "Continue Indefinitely" button (and explanatory text).
2. Probably no additional UI status/warnings.
3. If continuing indefinitely (this state would be stored in the JSContext?), DOMBranchCallback would just poll the state of the escape key, rather than putting up another modal dialog. If it's down, the function returns false (aborting the script).

One question is whether all of mozilla's platforms can make a non-blocking query of a key's current state, and second is whether there is already a cross-platform API for that or if that's something one would need to write.

A super obvious UI would be nice, but I imagine many users already respond to an unresponsive browser by pounding on the escape key.

And if this just isn't doable, it looks like the "timeout back-off" idea (5, 10, 30, 60 seconds, etc) might be trivial: just add a continued counter to the JSContext (increment with each decision to continue from the modal dialog), and make the duration test use sMaxScriptRunTime with a multiplier based on that count. (Though I am assuming the JSContext is unique to a particular document's scripts. Is it better to hang the counter off of JSScript? I seem to recall that not all calls to this callback provide that argument.)

Anyway, my computer's CPU hasn't had a decent work-out in a while, so if, unlike myself, anyone here actually knows what they are talking about and could give me some feedback, I'd be interested in working on this.

Comment 63

[Robert Ginda]

(In reply to comment #62)
> I think the problem is that all of this is (and must be) happening in the UI
> thread, and two related (potential) problems then restrict our options:
> 
> 1. I don't believe it is possible to "push the context" of the Javascript
> interpreter <...>

Venkman does this all the time.  Unless there is something tricky about the state of the engine during this loop detection callback, this shouldn't be an issue.  Have a look at http://lxr.mozilla.org/mozilla/source/js/jsd/jsd_xpc.cpp#2942, which is how venkman services the event loop when the debug target is stopped.

> 2. I believe a call into the js interpreter is completely "blocking" in the
> sense that the main event dispatch loop won't happen until the script
> completes. <...>

It shoudl be possible to service the event queue whenever the loop-detection callback fires (whenever the dialog would have been displayed.)  Probably you'd do something like...

  * When The Callback fires for the first time, push a new js context and open The Dialog.

  * Disable the window that is running the slow script.  You'll want to be sure that the window isn't going to get any new events while you're personally servicing the event queue.  If it got any events, it'd break the single-threaded view of the world that js programmers normally count on.  The event handler would appear to run at the same time as the long running loop.

  * Service events until you somehow know that The Dialog is fully loaded.  If the dialog is xul of any sort, it's going to require all sorts of networking events to happen before it's ready for use.

  * Once The Dialog is loaded, make The Callback fire more aggressively than usual so you can service the event queue without too much delay.  Return the "continue" value from the callback.

  * The Dialog must be responsible for communicating to The Callback the fact that the user wants to stop the script.

  * Each time The Callback fires, drain the event queue.

  * If during the draining of the queue The Callback is told to stop the script, hide The Dialog and return the "no mas" value from The Callback.  Otherwise return the "continue" value.

  * If the the script runs to completion hide The Dialog.  This is the tricky part.  The js engine isn't going to call you back and tell you that the slow script is done.

It'll probably all end up being harder than it sounds, and there may be a roadblock or two that makes it totally unworkable.  I'd guess it's worth a little poking around though, to see if it'd really work.

Comment 64

[Brendan Eich [:brendan]]

The problem is not the JS interpreter (SpiderMonkey).  It is thread-ready and it separates execution model from data model.

The problem is also not the JS run-to-completion execution model.  We do not want scripts and event handlers preempting or nesting in one another, even if only when one is long-running.  If you start serving UI events, you are going to have to prevent violations of the run-to-completion execution model.

The JS run-to-completion execution model means any scripts that can share data (that can access the same DOM or window object, or the global variables stored as properties of window objects) must not be preempted or interrupted by another script that might mutate that data.

Venkman allows violations of this model, but it's special -- it's a debugger.

The problem could be blamed on the single-threaded, main-thread-only nature of most Gecko code.  That won't be fixed quickly -- see bug 40848.

We could let chrome scripts run, violating the content world's JS execution model at the limit.

We could even let content scripts run if they were loaded in windows that belong to disconnected components in the graph of all JS objects.  This is the "window group" idea that I pitched to Mike McCool in 1998, and that he started prototyping in the classic codebase (see http://lxr.mozilla.org/classic/source/lib/libmocha/lm_win.c).  If we can compute window groups -- disconnected components in the object graph, each rooted by windows that can reach one another, but that cannot reach other groups' windows -- then we might get somewhere.

The challenge with window groups is the "join" operation -- when two or more formerly disjoint groups connect via window.open or some DOM operation, and now can read and write one another's data.

So, two ideas:

1.  Let chrome scripts run, even though they are loaded in chrome windows that can reach their own (and other) content windows.

2.  Identify window groups, disconnected from one another in the JS object graph, and let their scripts interleave and nest freely.

/be

Comment 65

[Justin Bradford]

Does the current code does work as:

Take an event from the queue, dispatch it, trigger a script, execute the script to completion, and repeat.

And for concurrent script execution we'd need:

The ability to partially execute a script, then put it on a wait queue to process later during idle time. And when servicing the event queue, we would need to determine whether dispatching a particular event would violate the run-to-completion model of any scripts on the queue, and if so, queue that event to be dispatched later when it's no longer blocked. (And clearly the "queue the event" would have to be at a deeper level [eg. queue a dom manipulation], but it doesn't change the basic algorithm.)

Determining whether an event is blocked could be a simple chrome exception rule or based on connectivity in the dom object graph, but I assume the "scheduling" infrastructure also needs to be written, right?

Also, on the object graph "join problem", I don't think it is possible to preserve the run-to-completion model without introducing the potential for deadlocks. Well, not unless script execution could be "rolled-back", which obviously isn't going to happen -- dom manipulations would need to be transactional. Imagine that patch.

Anyway, script executions are meant to be brief and atomic, and I don't think changing that assumption is worth the effort (ie. rewriting everything). And I definitely don't think web developers would be too happy if the next rev of javascript introduced concurrency issues and corresponding primitives to address them.

I think the immediate issue is simply that there are occasions when the user is okay with their browser being non-responsive for periods of time longer than 5 seconds, but they definitely don't want to confirm that every 5 seconds.

A whitelist works in some cases, but there is also the intermediate "maybe" case. Maybe it just needs a minute, and maybe I'm going to have to kill by browser. The maybe case technically works now, but it's annoying. You can click continue every 5 seconds, or you can open your browser to attack (or incompetence) during general browsing.

So does the "hidden" (it would be explained in the initial prompt) option to hold down a key to abort a script sound like an okay solution? Second, can we actually do it? You can query the state of a key on the core platforms, but is that universally true?

If that won't work, or people don't like it, how do I make the "timeout back-off" idea work on a per-page (or document or docshell or whatever is the appropriate thing) work? What does a JSContext and a JSScript (and for that matter, the JSContext stored in thread-local storage) correspond to?

Comment 66

[Brendan Eich [:brendan]]

(In reply to comment #65)
> And for concurrent script execution we'd need:
> 
> The ability to partially execute a script, then put it on a wait queue to
> process later during idle time.

If there are native method frames on the main thread stack, you can't unwind through them.  It would be better to nest.

> And when servicing the event queue, we would
> need to determine whether dispatching a particular event would violate the
> run-to-completion model of any scripts on the queue, and if so, queue that
> event to be dispatched later when it's no longer blocked. (And clearly the
> "queue the event" would have to be at a deeper level [eg. queue a dom
> manipulation], but it doesn't change the basic algorithm.)

We defer stuff till the current event-driven control flow unwinds often enough that it's not a hack ;-).

> Determining whether an event is blocked could be a simple chrome exception rule
> or based on connectivity in the dom object graph, but I assume the "scheduling"
> infrastructure also needs to be written, right?

Yes.

> Also, on the object graph "join problem", I don't think it is possible to
> preserve the run-to-completion model without introducing the potential for
> deadlocks. Well, not unless script execution could be "rolled-back", which
> obviously isn't going to happen -- dom manipulations would need to be
> transactional. Imagine that patch.

The DOM does need to be transactional in at least a semi-ACID way.  I'll blog about that shortly.

Yeah, we were gonna sacrifice run-to-completion for the loser of a two-way join and let the winner upset its invariants, in order to avoid deadlock.  This was 1998, in the Netscape 4.5 era codebase.  Ancient history.

> Anyway, script executions are meant to be brief and atomic, and I don't think
> changing that assumption is worth the effort (ie. rewriting everything). And I
> definitely don't think web developers would be too happy if the next rev of
> javascript introduced concurrency issues and corresponding primitives to
> address them.

We can't change the execution model.  I keep saying this.  Of course, I have no idea whether IE preserves it.  I think so (via COM apartment threading), but any insights are welcome.

> If that won't work, or people don't like it, how do I make the "timeout
> back-off" idea work on a per-page (or document or docshell or whatever is the
> appropriate thing) work? What does a JSContext and a JSScript (and for that
> matter, the JSContext stored in thread-local storage) correspond to?

This is ok, short term.  I think rather than reopening this bug, a new bug should be filed referencing this bug.

I'm still interested in ways around the dialog that do not entail free-threading Gecko....

/be

Comment 67

[Justin Bradford]

queueing vs nesting doesn't really change the big issues, though:

> Yeah, we were gonna sacrifice run-to-completion for the loser of a two-way
> join and let the winner upset its invariants, in order to avoid deadlock.
> This was 1998, in the Netscape 4.5 era codebase.  Ancient history.

> We can't change the execution model.  I keep saying this.

Well, first I agree with latter statement, but the two are definitely in conflict.

And it's not ancient history. A deadlocked forced join will often be okay (and you can even take a pseudo-Hippocratic oath to "do as little harm as possible"), but the second you introduce this, some code some where is going to break and then you'll get an RFE for concurrency primitives in javascript.

You can't have concurrent scripts and preserve the run-to-completion model (ie. atomic execution model) unless you are able and willing to rollback execution.

> The DOM does need to be transactional in at least a semi-ACID way.

Wow. I agree that would be good, but, well, I sort of figure it would be less demanding on bugzilla to post an entirely new copy of the mozilla source tha
to post that patch. It can be done incrementally, but damn. What about the atomic code that needs to modify then read from a service (where the modification has an effect on the subsequent read, potentially propagating through multiple objects) -- and then needs to roll that change back? And meanwhile, no other thread of execution should see the modification.

General transactional programming is still largely an academic curiousity right 
now. Do you really imagine that massive change happening across the mozilla codebase any time soon? And the effect on web and extension developers? Hell, do all of mozilla's platforms even support universal atomic primitives at the hardware level (ie. CAS or equivalent and the necessary memory fences)?

> I think rather than reopening this bug, a new bug should be filed referencing this bug.

To be clear, the bug was in "reopened" status when I came across it. I did accidentally change the state to "assigned" without making me the assigned to, which I intend to fix with this post. Cross your fingers and pray to/curse at bugzilla.

Anyway, I think our discussion should be in some other bug, but that's because we are building lovely blue sky around the issue of concern -- we don't like clicking on continue every X seconds.

How do we fix that?

Comment 68

[Brendan Eich [:brendan]]

(In reply to comment #67)
> queueing vs nesting doesn't really change the big issues, though:

Sure, it was just a point of fact -- if we knew there weren't any native frames nesting on the JS stack, we could do what you wrote.  It might even be worth the trouble to avoid nesting too deeply (we have a simple-minded thread stack limit to prevent d.o.s. crashers, set at 500K currently).

> > Yeah, we were gonna sacrifice run-to-completion for the loser of a two-way
> > join and let the winner upset its invariants, in order to avoid deadlock.
> > This was 1998, in the Netscape 4.5 era codebase.  Ancient history.
> 
> > We can't change the execution model.  I keep saying this.
> 
> Well, first I agree with latter statement, but the two are definitely in
> conflict.

Then vs. now.  What's the conflict?

> And it's not ancient history. A deadlocked forced join will often be okay (and
> you can even take a pseudo-Hippocratic oath to "do as little harm as
> possible"), but the second you introduce this, some code some where is going to
> break and then you'll get an RFE for concurrency primitives in javascript.

I know, that's why I keep saying we can't change the execution model.  I've been saying this since about 1998.  Is this horse dead yet?

> Wow. I agree that would be good, but, well, I sort of figure it would be less
> demanding on bugzilla to post an entirely new copy of the mozilla source tha
> to post that patch. It can be done incrementally, but damn. What about the
> atomic code that needs to modify then read from a service (where the
> modification has an effect on the subsequent read, potentially propagating
> through multiple objects) -- and then needs to roll that change back? And
> meanwhile, no other thread of execution should see the modification.

Wait for my blog.  This isn't the venue you're looking for.

> General transactional programming is still largely an academic curiousity right 
> now. Do you really imagine that massive change happening across the mozilla
> codebase any time soon?

This is longer range than current plans (Firefox 2 and Firefox 3 / Gecko 1.9). Long-range planning, what a concept.

> And the effect on web and extension developers?

Backward compatibility is necessary on the web.  New models generally require opt-in.  Occasionally you can fix a bug and get away with it, because the bug was so bitey that people didn't count on it, or were broken even as they counted on it.  Eventually, the bad old dependencies die off.

> Hell,
> do all of mozilla's platforms even support universal atomic primitives at the
> hardware level (ie. CAS or equivalent and the necessary memory fences)?

Yes, so far -- see http://lxr.mozilla.org/mozilla/source/js/src/jslock.c.

> > I think rather than reopening this bug, a new bug should be filed referencing this bug.
> 
> To be clear, the bug was in "reopened" status when I came across it. I did
> accidentally change the state to "assigned" without making me the assigned to,
> which I intend to fix with this post. Cross your fingers and pray to/curse at
> bugzilla.

Sure, no problem -- I wasn't sure who did what, but a new bug seems better no matter whodunnit.

> Anyway, I think our discussion should be in some other bug, but that's because
> we are building lovely blue sky around the issue of concern -- we don't like
> clicking on continue every X seconds.
> 
> How do we fix that?

New bug please?  This one was fixed by a patch that allows disabling the dialog, which is exactly what its summary requests.  Life will be better in a new bug.

/be

Comment 69

[Justin Bradford]

Hey, sorry, I didn't mean to get in a fight.

When I came across this bug it was already "reopened" and, as it still states in status line, it was fixed1.8 and uiwanted. For 1.8 there is a modifiable about:config pref, but going forward there was some discussion on "the dialog". Should it simply be whitellsted away? The bug reporter liked my non-prompting ideas, and this bug was definitely not in a fixed state when I found it, and it's been that way for a long time.

I was just trying to be helpful in the sense of offering to write code to improve the situation. Is this a resolved bug? There was no indication that was so, and I think at least some of the CC's would dispute that.

Our dicussion so far should definitely be in some other bug, but I just want to figure out how to best resolve the every 5 seconds modal dialog for long-running scripts. That's what I (intended) to sign up for. How should I fix that?

Comment 70

[Brendan Eich [:brendan]]

(In reply to comment #69)
> Hey, sorry, I didn't mean to get in a fight.

The first rule of bugzilla club is ...

Really, we're not fighting, but a new bug with a summary saying what you want, in one line if possible, and referring to this bug, would be swell.  The binary exponential backoff idea sounds good, and maybe we can do other smart things in the short run.  But not here, this bug seems fixed to me (I turned off the dialog, myself, and haven't lost any sleep yet).

/be

Comment 71

[u119943]

(In reply to comment #70)
> the short run.  But not here, this bug seems fixed to me (I turned off the
> dialog, myself, and haven't lost any sleep yet).

By this are you reffering to this code in http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#612

611       if (prefBranch) {
612         prefBranch->SetIntPref("dom.max_script_run_time", 0);
613       }

?

If so - am I right to understand that this code is available only in 1.8 and not in the current "consumer" releases?

Can I set dom.max_script_run_time to 0 in 1.5.0.3 and get the same effect?

Thanks,

--Amos

Comment 72

[Justin Bradford]

Cool. I've taken the liberty of resolving this bug to "fixed" then.

I'll definitely file those other bugs, but they will, unfortunately, have to wait until my utter hatred for the mozilla organizatin's employees decays a bit. Maybe when I just very much dislike you...

Amos: It is my understanding that you can set the timeout to 0 and eliminate the prompts in 1.5. But please let us know if that doesn't work. That would (presumably) be a legitimate topic for this bug -- but these days who really knows.

Comment 73

[Brendan Eich [:brendan]]

(In reply to comment #72)
> Cool. I've taken the liberty of resolving this bug to "fixed" then.
> 
> I'll definitely file those other bugs, but they will, unfortunately, have to
> wait until my utter hatred for the mozilla organizatin's employees decays a
> bit. Maybe when I just very much dislike you...

What exactly did I say that was so bad?  Yeesh.

/be

Comment 74

[Brendan Eich [:brendan]]

(In reply to comment #73)
> What exactly did I say that was so bad?  Yeesh.

Ok, "Is this horse dead yet" (comment 68) was a little snarky -- sorry about that.  But "hatred"?  Come on, that's too strong unless something else not actually in these bugzilla comments is going on.

Anyway, thanks for resolving this bug, and if you can forgive me for using the wrong words, please do file those other bugs.

/be

Comment 75

[William J. Edney]

Justin -

I really debated about responding to this bug, as I didn't want to add to the noise at the end of this bug, but I can't help it. I have to say something.

Many of the people signed up for this bug are key players in the Mozilla organization and, while I've never met many of them personally, I wouldn't have a product or a company without them and their efforts. These people (yes, there are real humans with feelings lurking behind those keyboards) have saved the Internet. Really. Do you think the folks in Redmond would be contemplating IE7 if not for FF / Mozilla?

These people have been nothing but kind and helpful to me and my company. When it made sense for Mozilla as a whole, they even added specific features to Mozilla just for me. Brendan, jst, peterv, bzbarsky, rginda and others I don't know as well are responsible for making sure that the Internet stays open, free and follows standards. One day I'll figure out a way to thank them.

Thanks for showing interest in this problem (its a real problem for me too). I'm sure with reasoned debate on whatever bug you'd like to open for this we can come to a solution for everyone.

Cheers,

- Bill

Comment 76

[David]

I use qooxdoo, http://qooxdoo.org/, a Javascript library for building UI.
It is very easy to have a routine with over 5 seconds with this library, and this isn't bad designed Javascript.

The JS code layouts the full page, and for big pages, it can take some seconds.

So, it becomes more important to be less annoying with this dialog.
It would be nice if it have an checkbox which says something like:  "Don't bother me anymore with pages from this domain", so that the user can deactivate this warning.