Closed Bug 335058 Opened 14 years ago Closed 14 years ago

script timeout too small in firefox

Categories

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

x86
Windows XP
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ratan.work, Assigned: jst)

References

Details

(Keywords: dev-doc-complete, fixed1.8.0.7, fixed1.8.1)

Attachments

(5 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060409 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060409 Firefox/1.5.0.2

We have been seeing "Warning: Unresponsive Script" dialog box a lot in yahoo mail beta and it happens a lot when some cpu intensive application is running on users box and they tries to check mail.

The value of dom.max_script_run_time in FF 1.5.0.2 is 5 second. It seems to be too low compare to what IE offers. Even though user can change it in about:config but it is too hard to tell millions of users to do so.

To compare it with IE:
IE timesout after executing 5,000,000 statements on current page.
http://support.microsoft.com/?kbid=175500

Wouldn't it be a good idea to set dom.max_script_run_time to 60 sec or
something like that in FF by default (I mean something reasonably high)? The
value of 5 sec seems to be too low.

Some of the bugs already filed on it are:
https://bugzilla.mozilla.org/show_bug.cgi?id=250841
https://bugzilla.mozilla.org/show_bug.cgi?id=330293
https://bugzilla.mozilla.org/show_bug.cgi?id=308205
https://bugzilla.mozilla.org/show_bug.cgi?id=320409
https://bugzilla.mozilla.org/show_bug.cgi?id=320020
https://bugzilla.mozilla.org/show_bug.cgi?id=320409



Reproducible: Sometimes
Some what related bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=311994
*** Bug 335060 has been marked as a duplicate of this bug. ***
we bumped this up in minimo due to slower processors on mobile devices, and it seems to be working well there.  might be worth considering for firefox 2.0 as well due to more use of intensive Ajax apps on the web.
Assignee: nobody → jst
Flags: blocking1.9a1?
Flags: blocking1.8.0.4?
Flags: blocking-firefox2?
mrbkap, brendan: what do you think?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I have mine set to 1800 and don't see problems on public web sites. Of course that is too high, but 60 seems reasonable.
IE is not comparable, because as far as I know, it uses COM apartment threading to avoid problems hanging the whole app due to a runaway or rogue script.

Locking up our UI for 60 seconds, without the Stop button even being live, is a bad idea.  Even 10 seconds is long.

This bug's reporter has heard it before: for cross-browser best results, break up long computations via setTimeout.  This sucks in known ways for the few and elite hackers who write such long-running JS.  It does not suck for the millions of end users, and they're the audience to optimize for, given the current browsers.

Things will get better as we evolve JS and our execution model a bit, so don't get me wrong: we should be and are working on making things better for JS hackers. But I do not believe jacking up the limit, given the whole app locks up with no UI responsiveness, is the right answer.

/be
Brendan,
The problem with the time based script timeout is that if users' computer is running some CPU intensive task at the same time as Firefox then they will see the "unresponsive script dialog" too often if they are browsing around. We have done optimizations using setTimeout to avoid it and even though we see it at times when CPU intensive tasks like virus scan, backup is running on windows box.

If time based script timeout is hard to fix then can we move FF script timeout to IE modal i.e. timeout based on number of statements executed and that way we can avoid FF script timeout dependency on other tasks in the system.

Correct me if I知 wrong on the dependency of CPU intensive task and firefox as that is what we have observed.

Thanks for your response.
Ratan
Yahoo Mail Team
(In reply to comment #7)
> Brendan,
> The problem with the time based script timeout is that if users' computer is
> running some CPU intensive task at the same time as Firefox then they will see
> the "unresponsive script dialog" too often if they are browsing around. We have
> done optimizations using setTimeout to avoid it and even though we see it at
> times when CPU intensive tasks like virus scan, backup is running on windows
> box.

Sounds like the OS is starving Firefox, but let's say that happens and we have to work around it.

> If time based script timeout is hard to fix then can we move FF script timeout
> to IE modal i.e. timeout based on number of statements executed and that way we
> can avoid FF script timeout dependency on other tasks in the system.

We don't count statements.  We used to monitor backward branches and returns in the bytecoded JS VM, but that was fixed using wall-clock time, because counting backward branches does not really count time at all, and the UI is locked up the whole while.  That is, you could have 256K backward branches that run in < 1sec, or take > 60sec, depending on what goes on between the branches.

> Correct me if I知 wrong on the dependency of CPU intensive task and firefox as
> that is what we have observed.

How much computation are you doing between setTimeouts?  If it's still a lot, any way to use a smaller quantum?

/be
>How much computation are you doing between setTimeouts?  If it's still a lot,
>any way to use a smaller quantum?

We have done the changes already to accommodate FF. Even though we continue to see the dialog and keep getting user complains. If users have to change the script timeout value to avoid "unresponsive script dialog" to get better web surfing experience then I think it defeats the purpose of keeping it too low.

I was wandering if it is possible to:
1. Log something in JavaScript console about the script that is causing this problem? This will help engineers.
2. In 'unresponsive script dialog', display users a preference to change this value. This will help web surfing users and they can make their own decision.
3. Worst case, allow javascript to hear this event from FF. This way we can display users an appropriate messsage as how to fix it.
    e.g.
    window.onunresponsiveScript = userAlertAboutPref;
4. Worst case, allow javascript to change this value for a browser session   ( for that domain or all domain in that session)
     window.max_script_run_time = 20000; //20 second, should be greater than default

Thanks
Ratan

Component: General → DOM
Flags: blocking-firefox2?
Product: Firefox → Core
Version: unspecified → Trunk
QA Contact: general → ian
Too late to test the implications and fall-out in 1.5.0.4, unblocking for this release.
Flags: blocking1.8.0.4? → blocking1.8.0.4-
*** Bug 342734 has been marked as a duplicate of this bug. ***
*** Bug 330293 has been marked as a duplicate of this bug. ***
Just my 2 cents.   The timeout happens only when Firefox is opened.  I have no problems once the program is loaded.   I believe that's when Firefox checks for updates, and here lies what I think could be the problem:  the dns lookup for mozilla takes about 20 seconds to be found. I counted the time connecting to the web site several times. So could that be what is really timing out.
comment #13
Timeouts happens even after Firefox is opened as I have seen it myself and have user reports of seeing it in Yahoo Mail Beta.
Let me know if you guys need more info. Any plan to fix it in the next release as it was taken out of 1.5.0.4 release.
*** Bug 345493 has been marked as a duplicate of this bug. ***
The script watchdog barking at our own lengthy extension-manager JS is a problem, I agree.  Separate bug, arguably.  That is, we could disable the watchdog during EM startup, and still leave this bug for later.

Cc'ing Rob Strong and Mike Connor.

/be
Flags: blocking1.8.1?
Does this actually happen with the extension-manager JS? I haven't seen it in quite some time and have tested operations with 100+ extensions being installed, etc. without experiencing this.
(In reply to comment #17)
> Does this actually happen with the extension-manager JS? I haven't seen it in
> quite some time and have tested operations with 100+ extensions being
> installed, etc. without experiencing this.

Depends on OS and what all is running, competing for time slices and VM.  It's possible to give Firefox too little real time and have the watchdog bark.  Unless we think we'll save someone from an iloop but in EM or similar, we should disable the watchdog during startup, I think.

/be
The default max_script_run_time is definitely too low. As you can see in bug #345493 it's triggered by simply reading a Slashdot page such as:

http://yro.slashdot.org/article.pl?sid=06/07/21/0448225

I've had it happen on other legit websites too but I forgot to report the problem.

Vital stats: Firefox-1.5.0.4, Linux Fedora Core 5, P4 2.4GHz - so this system is clearly not too slow. CPU idle is typically around 95...99% except when Firefox starts chewing that page.
Could we leave it 5 seconds for content and make it larger (30 or 60 seconds) for chrome JS?
Flags: blocking1.8.1? → blocking1.8.1+
Ideas, easy ones for 1.8.1 star'ed:

- Bump to 10 seconds for all, or (see below) for content (*)
- Split chrome and content as dbaron proposed (*)
- Use process-virtual time, not real time (?)
- Put up some modal UI that's polled from the branch callback often enough to seem responsive
- Poll the Stop button

/be
> it's triggered by simply reading a Slashdot page

Er... in a vanilla no-extensions install?  I see no slow script dialog on that page, with a MUCH slower machine (p3-733).
This makes us differentiate between chrome and content code (at the top of the JS stack) and gives us different prefs to controll the two limits. I set the default limit for chrome to 20, which is really high but hey, chrome code shouldn't iloop  (or call content code that could iloop) to begin with. I don't know that these are the defaults we want, but I picked those numbers as defaults to have a starting point at least.
Attachment #230677 - Flags: superreview?(brendan)
Attachment #230677 - Flags: review?(bzbarsky)
Comment on attachment 230677 [details] [diff] [review]
Set different time limits for chrome and content, and bump content limit to 10 seconds.

r=bzbarsky, but I think we should leave the non-chrome pref at 5 seconds unless that 5 is being a _really_ major problem.  10 seconds is a _very_ long time for which to freeze up the browser....
Attachment #230677 - Flags: review?(bzbarsky) → review+
(In reply to comment #24)
> (From update of attachment 230677 [details] [diff] [review] [edit])
> r=bzbarsky, but I think we should leave the non-chrome pref at 5 seconds unless
> that 5 is being a _really_ major problem.  10 seconds is a _very_ long time for
> which to freeze up the browser....

Problem reports from the field suggest our use of real time is a bug.  If we used process-virtual time, we would be better off.  Given this flaw, even 10 seconds on an overloaded system, with Firefox deprioritized for VM reasons, may not be enough wall time.  Not sure we can get process-virtual time easily on all platforms.  Cc'ing wtc, hope he is not too busy still and can give some advice.  Thanks,

/be
Comment on attachment 230677 [details] [diff] [review]
Set different time limits for chrome and content, and bump content limit to 10 seconds.

The patch is fine given the real-time design.  Not sure we can do better for Fx2.

/be
Attachment #230677 - Flags: superreview?(brendan) → superreview+
Comment on attachment 230677 [details] [diff] [review]
Set different time limits for chrome and content, and bump content limit to 10 seconds.

>-    sMaxScriptRunTime = LL_INIT(0x40000000, 0);
>+    t = LL_INIT(0x40000000, 0);
>+  } else {
>+    t = time * PR_USEC_PER_SEC;
>+  }

One nit: given the righteous removal of LL_* elsewhere, do we still need LL_INIT, or would 0x4000000000000000LL not work on all platforms?

/be
Brendan,

I'm not sure if we can get process-virtual time on all platforms.
I believe we can get process-virtual time on Unix (including Mac
OS X and Linux) and Windows.

The LL_INIT macro is actually intended for initializers. But that
limitation only applies when PRInt64/PRUint64 is a struct.  So
it is fine now to use LL_INIT anywhere to construct a PRInt64/PRUint64
constant, even though the "INIT" in the name looks strange.  Older
versions of MSVC (such as 6.0) use i64/ui64 as the suffix for
PRInt64/PRUint64 constants.  I believe they are the only compilers
that don't support the LL/ULL suffix.
Same as above but with LL_INIT removed.
Comment on attachment 231009 [details] [diff] [review]
Patch that was checked in.

This was checked in on the trunk, but I'll leave the bug open in case we want to explore alternative fixes for the trunk. Requesting 1.8.1 approval for this change.
Attachment #231009 - Flags: superreview+
Attachment #231009 - Flags: review+
Attachment #231009 - Flags: approval1.8.1?
Comment on attachment 231009 [details] [diff] [review]
Patch that was checked in.

Johnny,

>-    sMaxScriptRunTime = LL_INIT(0x40000000, 0);
>+    t = 0x4000000000000000LL;

Have you tested this code under MSVC 6.0?  It doesn't
support the long long type and the LL suffix.  You need
to use the __int64 type and the i64 suffix with MSVC 6.0.
(In reply to comment #31)
> Have you tested this code under MSVC 6.0?  It doesn't
> support the long long type and the LL suffix.  You need
> to use the __int64 type and the i64 suffix with MSVC 6.0.

No, I have not, but Mozilla doesn't build with MSVC 6.0 any more for other reasons either, so I don't think we really care any more. If we do, I'm happy to revert that change tho.
(In reply to comment #32)
> No, I have not, but Mozilla doesn't build with MSVC 6.0 any more for other
> reasons either

If this patch is going in on the 1.8 branch, I believe that we still use MSVC 6.0 there.
0x400..00LL turned tinderbox red too, so I had to undo that change on the trunk as well.
Attachment #231009 - Flags: approval1.8.1?
Attachment #230677 - Flags: approval1.8.1?
Comment on attachment 230677 [details] [diff] [review]
Set different time limits for chrome and content, and bump content limit to 10 seconds.

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH
Attachment #230677 - Flags: approval1.8.1? → approval1.8.1+
FYI
we are getting more reports of script timeout (unresponsive script) on Yahoo Mail Beta and I was wandering if anything got changed in FF 1.5.0.5 that can make it worse in new release.
I will keep an eye on it and update the thread again.
Fixed on trunk and 1.8 branch.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Doing my regular "nightly" build with MSVC7.1, I get the following build error on the branch with the patch that was checked in:

-nologo -W3 -Gy -FdnsJSEnvironment.pdb  -DNDEBUG -DTRIMMED -O2p -Ob2 -GATF7 -GL -arch:SSE2 -MD            -DX_DISPLAY_MISSING=1 -DMOZILLA_VERSION=\"1.8.1b1\" -DMOZILLA_VERSION_U=1.8.1b1 -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWINVER=0x400 -D_WIN32_WINNT=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_X86_=1 -DD_INO=d_ino -DMOZ_DEFAULT_TOOLKIT=\"windows\" -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ_SVG=1 -DMOZ_SVG_RENDERER_CAIRO=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_MORK=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1 -DMOZILLA_1_8_BRANCH=1 -DMOZILLA_LOCALE_VERSION=\"1.8b5\" -DMOZILLA_REGION_VERSION=\"1.8b5\" -DMOZILLA_SKIN_VERSION=\"1.8\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/i/Mozilla/mozilla/dom/src/base/nsJSEnvironment.cpp
nsJSEnvironment.cpp
i:/Mozilla\mozilla\dom\src\base\nsJSEnvironment.cpp(516) : error C2039: 'JS_IsSystemObject' : is not a member of 'operator``global namespace'''
i:/Mozilla\mozilla\dom\src\base\nsJSEnvironment.cpp(516) : error C3861: 'JS_IsSystemObject': identifier not found, even with argument-dependent lookup
make[5]: *** [nsJSEnvironment.obj] Error 2
make[5]: Leaving directory `/cygdrive/i/mozilla/objdir/dom/src/base'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/i/mozilla/objdir/dom/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/i/mozilla/objdir/dom'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/cygdrive/i/mozilla/objdir'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/i/mozilla/objdir'
make: *** [build] Error 2
Compile finished at 
The current time is: 18:54:46.48
 Build failed 

Any help?
I suspect that this checkin also broke the 1.8 Camino builds:
http://tinderbox.mozilla.org/Camino/
Comment on attachment 231177 [details] [diff] [review]
Branch patch that was checked in.

This patch has caused bustage in Camino for the 1.8 Branch.
It seems that removing the "ac_add_options --disable-jsd" option from the camino mozconfig fixes the bustage. Perhaps there is a better dependency work around (maybe an |#ifdef|)?
Status: RESOLVED → VERIFIED
It seems that removing the "ac_add_options --disable-jsd" option from the camino mozconfig fixes the bustage. Perhaps there is a better dependency work around (maybe an |#ifdef|)?
Oops sorry mouse caught the radio button.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
FWIW, this is the change I did to fix my 1.8 branch debug build on Linux...
(In reply to comment #45)
> Created an attachment (id=231295) [edit]
> FWIW, this should fix the 1.8 bustage...
> 
> FWIW, this is the change I did to fix my 1.8 branch debug build on Linux...
> 

This patch fixes the Camino 1.8 branch bustage as well.
(In reply to comment #45)
> Created an attachment (id=231295) [edit]
> FWIW, this should fix the 1.8 bustage...
> 
> FWIW, this is the change I did to fix my 1.8 branch debug build on Linux...
> 

I can also confirm that this fixes the problem on my local build (MSVC7.1) on WinXP.
I'm not familiar with this code, but if the function in question is now being used for things that aren't related to JS debugging, then it probably needs to be moved to a non-JS-debugging header, rather than bringing the debug header into a core JS file.
Mats, you should probably ask for review and get this patch in the tree...
(In reply to comment #48)
> I'm not familiar with this code, but if the function in question is now being
> used for things that aren't related to JS debugging,

That's not relevant -- JS_IsSystemObject was always part of jsdbgapi.h.

The bug that bit Camino was a latent one, a dependency on nested header includes. Since nsJSEnvironment.cpp directly calls JS_IsSystemObject, it must directly include "jsdbgapi.h".

> then it probably needs to
> be moved to a non-JS-debugging header, rather than bringing the debug header
> into a core JS file.

nsJSEnvironment.cpp is a core DOM file, rather -- but the main point is that "dbg" or not, JS_IsSystemObject and other reflection or non-main-API exports have been and will continue to be provided via jsdbgapi.h.  That's not the bug, since that organization, however distasteful, has not changed.  What changed was the direct dependency on JS_IsSystemObject from nsJSEnvironment.cpp being added without the necessary #include.

I'll + the bustage fix -- mats, please check in ASAP.

/be
Attachment #231295 - Flags: review+
Attachment #231295 - Flags: approval1.8.1?
Removing the fixed keyword so we don't lose track of that patch...
Flags: blocking1.9a1?
Keywords: fixed1.8.1
Blocks: 346527
Comment on attachment 231295 [details] [diff] [review]
FWIW, this should fix the 1.8 bustage...

Looks like dougt just checked this into the 1.8 branch (without approval!).
Attachment #231295 - Flags: approval1.8.1?
Keywords: fixed1.8.1
It would be nice if the timeout prompt had a checkbox saying "don't ask again during this session" or something like that.
It can be _very_ annoying to click on the pop-up 30 times just to complete the rendering of a page. I'd like the browser to just remember my last decision and apply it automatically.
(In reply to comment #53)
> It would be nice if the timeout prompt had a checkbox saying "don't ask again
> during this session" or something like that.
> It can be _very_ annoying to click on the pop-up 30 times just to complete the
> rendering of a page. I'd like the browser to just remember my last decision and
> apply it automatically.
> 

See bug 347365 for a different-but-complementary approach to solving this same problem.
Is this safe enough for the 1.8.0.x branch? 
Flags: blocking1.8.0.7?
(In reply to comment #56)
> Is this safe enough for the 1.8.0.x branch? 
> 

I don't know how to define "safe enough", but I do know that there have been requests by Fx/Tb 1.5.0.x users (me among them) to make the preference easier to find, because that same popup was so annoying. See for instance bug 347365 (Fx) and bug 347476 (Tb), which are not dupes of this one but relate to the same topic. I don't like bugspamming any more than anyone else, I just hope that some fix will be found for the 1.8.0.x/1.5.0.x branch before it becomes obsolete. Keep up the good work, you guys. :-)
We don't want this code change in 1.5.0.x, but if you want to increase the default pref (only) to 10 to accomodate popular sites that seems safe enough and we'd consider approving that.

Would that be good enough?
Flags: blocking1.8.0.7? → blocking1.8.0.7-
(In reply to comment #58)
> We don't want this code change in 1.5.0.x, but if you want to increase the
> default pref (only) to 10 to accomodate popular sites that seems safe enough
> and we'd consider approving that.
> 
> Would that be good enough?

Anything's better than the current situation.
Default max_script_run_time increase in 1.8.0.x/1.5.0.x branch will be appreciated as I saw it twice today on Firefox/1.5.0.6

I'm assuming FF 2.0 (1.8.1.x branch) will set it to following default as fix seems to be marked as 1.8.1 blocker.
dom.max_chrome_script_run_time = 20 sec;
dom.max_script_run_time =  10 sec

I can see the warning dialog appearing everywhere if I throttle cpu on my box using 
slow or winThrottle
I have verified in nightly build that (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060817 BonEcho/2.0b1) chrome script and content script run time are set to new values:
dom.max_chrome_script_run_time = 20 sec;
dom.max_script_run_time =  10 sec

if possible then please increase content script timeout to 10 sec in FF 1.5.0.7.
Brendan: are you OK with just the time-limit change for the 1.8.0 branch?
Attachment #234346 - Flags: review?(brendan)
Attachment #234346 - Flags: approval1.8.0.7?
Comment on attachment 234346 [details] [diff] [review]
1.8.0 branch patch (just the pref change)

Sure, in practice it's not going to cause hardship to wait another 5 seconds, and it sounds like it will help in some hard cases.

/be
Attachment #234346 - Flags: review?(brendan) → review+
*** Bug 349087 has been marked as a duplicate of this bug. ***
Comment on attachment 234346 [details] [diff] [review]
1.8.0 branch patch (just the pref change)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #234346 - Flags: approval1.8.0.7? → approval1.8.0.7+
Pref-only change landed on the 1.8.0 branch.
Keywords: fixed1.8.0.7
Duplicate of this bug: 320409
There is no documentation on MDN yet for the introduced 'dom.max_chrome_script_run_time' pref by this bug.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.