30942 - Browser should remain responsive during most infinite loops

Status: RESOLVED INCOMPLETE

(Core :: General, defect)

Description

[Jesse Ruderman]

(Note: I present some presumptions here as facts.  Unless I totally blow 
something, please respond with non-flames.)


There have been many (exploitable) infinite loop bugs in mozilla, and their 
rate of appearance doesn't seem to be slowing down much.  Several involve 
layout: a reflow moves something down, which forces it to be moved back up, 
which forces it to be moved down again, etc.  Some involve javascript: perhaps 
there's a while(1) loop in the code.  Some involve history and page reloading.

Most of the time, the layout-related problems are fixed by reviewing the math 
that determines what happens when, leading to a fix specific to one type of 
infinite loop.  But HTML is complicated, and I doubt the layout engine will 
ever be safe from all types infinite loop attacks.

A better solution to this problem would be to add code in strategic places, 
such as reflow iterations, that force the browser to check if any user input 
(such as an attempt to close the browser) would make continuing the 
infinite/slow reflow moot.

In order to keep allow users to continue noticing reflow bugs (since they won't 
have to ctrl-alt-del the browser anymore), something should be added to the 
statusbar to make mozilla show when the browser is:

(1) idle
(2) waiting for the server to send (more) information
(3) waiting for user to respond to a modal dialog
(4) doing a normal reflow
(5) doing an incremental reflow
(6) executing javascript
(7) loading additional dlls or an external application

I propose that the spinning/moving bar on the left side of the statusbar be 
replaced or supplemented by a small box that changes color to indicate what the 
browser is doing.  Release versions might use fewer colors, perhaps light blue 
for 2 and 3, and dark blue for 4-7.  (This idea is based on a similar box in 
the dos game Civilization, which would switch between light grey and dark grey 
as the AI players went through various move phases and would get stuck on a 
specific color if a crash occurred.)

Users should quickly figure out that the browser is busy while the box/bar is a 
darker color because they won't be able to scroll, so they should still report 
infinite-reflow and similar bugs when they encounter them.


Some examples of bugs relating to infinite loops:
http://bugzilla.mozilla.org/buglist.cgi?bug_id=20283,24195,26618,30030,30830

Bug #20283  a href="javascript:history.go(0)"
Bug #24195  infinite reflow after adding content to scrollable frame
Bug #26618  infinite reflow in addressing ... more than one recipient
Bug #30030  infinite loop if pressing "back" leads to javascript redirect
Bug #30830  infinite loop in javascript code freezes mozilla

Some examples of slow (but not infinite) iteration problems:
http://bugzilla.mozilla.org/buglist.cgi?bug_id=17325,30091

Bug #17325  too many reflows on slashdot pages (with a fast connection)
Bug #30091  "select" form elements cause too many reflows

Comment 1

[Gervase Markham [:gerv]]

Dunno what the score is with confirmation of Enhancements, but this seems 
sensible, so confirming. It touches a lot of stuff, so leaving in 
Browser-General.

Gerv

Comment 2

[Doron Rosenberg (IBM)]

infinite reflow is related to 31574, and that is a crasher as well.

Marking dupe, as solving that should fix this.


*** This bug has been marked as a duplicate of 31574 ***

Comment 3

[Doron Rosenberg (IBM)]

ok, I was too quick on marking this a dupe.

moving to layout, think that might be the correct component.

Comment 4

[Ben Bucksch (:BenB)]

Bug #25472 was a joke.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

sounds like what is needed here is some sort of popup like IE's that says 
something to the effect of:

"A script on this page is taking a long time to complete execution. [ Continue ] 
[Stop running script ]"

(with better wording than that...)

Comment 6

[Jesse Ruderman]

I think it would be better if mozilla could remain responsive (able to 
navigate, close, etc.) while the script is running.  Otherwise, malicious page 
authors are likely to find a way around the code that determines whether the 
script is hogging resources or preventing user interaction.

Comment 7

[Matthew Mastracci]

I guess I'll just throw in another suggestion: don't allow the user to interact
with the page while a script/alert box is running (edit text boxes, click on
links, activate mouseovers - essentially disabling the page UI itself, perhaps
with a transparent event sink like a glasspane over it), but *do* enable the
stop/back/forward buttons.  They can scroll through the page, but not modify
anything on it - something that could possibly screw up the script.  If the
action would invalidate the currently running script(hitting back/stop/forward,
entering a URL), pop up a warning saying so.

Comment 8

[Matthew Mastracci]

Adding myself to CC: list.

Comment 9

[leger]

Adding crash keyword.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Moving to DOM Level 1 and reassigning.  I think this is a duplicate, though.

Comment 11

[Jesse Ruderman]

This doesn't really belong to a single component afaict.  It's a general 
request for prevention against attacks (and other things) that cause the 
browser to freeze.  It would need to be done in layout, javascript, and plenty 
of other places.  If you guys decide to fix it, this bug would probably become 
a meta-bug, with dependencies in various components.

cc [e-mail], the security person.

consider changing summary from "Infinite reflows, loops should not crash 
browser" to "Browser should remain responsive during most infinite loops"

Comment 12

[Ben Bucksch (:BenB)]

> A better solution to this problem would be to add code in strategic places, 
> such as reflow iterations, that force the browser to check if any user input 
> (such as an attempt to close the browser) would make continuing the 
> infinite/slow reflow moot.

I completely agree. You should always insert calls to the event loop after some
ms. It is not even hard to implement, if you e.g. call it once during a reflow
and in any central place in the compiled JS code. It should be done soon,
though, because this might have some side-effects, so needs testing.


Changing SUMMARY to davidr's suggested one.

Assuming davidr's claims are correct (especially that |while(1)| in a web script
or infinite reflows cause a freeze), I don't think, this is an enhancement
request, but a design bug, changing SEVERITY to major.

I suggest to file separate bugs for layout, JS and so on.

Comment 13

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer
workingon this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work in
some way -- please attach your concern to the bug for reconsideration.

Comment 14

[David Krause]

This bug blocks some infinite reflow loops in chatzilla. Bug 45576

Comment 15

[timeless]

Bug 45576 - Hang on joining #mozillazine
I'm concerned. I've marked helpwanted. If you don't like this bug I'll assign 
it to nobody.  Added keywords, nothing in this world gets done w/o nsbeta3.

Bug reassigned to default victim. Again, if you don't like this bug please send 
it somewhere. nobody is asking for it.

This is a catchall, but Ben's right, we should at the very least be able to do 
time counts on threads.

init does stuff like this, if it needs to respawn the console tty too many 
times it throttles it [odd case: no user is avail to prompt because interface 
is being throttled, but ...].

Comment 16

[Ben Bucksch (:BenB)]

> Ben's right, we should at the very least be able to do time counts on threads

What I meant was no counter, but to make sure, the event loop is called by just
inserting calls at the right place. With only that done (rightTM), the infinite
loops would still occur, but the app wouldn't freeze, which is already a
progress. More importantly, even in normal operation, the app were more
responsive. Maybe, my suggestion was orthogonal to this bug, dunno.

Comment 17

[Jesse Ruderman]

I don't know how appropriate it is to say that this bug "blocks" various freeze 
bugs.  Having this feature implemented well would make mozilla remain 
responsive during freezes, so, for example, you could close a slow-rendering 
webpage without killing mozilla.  Most of the individual frezze bugs should 
still be fixed, however, because they might be triggered by non-malicious 
webpages that we actually want to display completely.  Instead of being 
critical/crash bugs, they would become major or normal bugs (with the perf 
keyword?), but they'd still be there.

Comment 18

[Ben Bucksch (:BenB)]

Seems like no (or too few) event loop calls are done during the frame
construction.

Comment 19

[Judson Valeski]

minusing per PDT rules.

Comment 20

[Frank Tang]

I don't think this is a crash bug.

Comment 21

[Richard Waid]

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

Comment 22

[Robin Green]

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

Comment 23

[Marko Macek]

I have one comment: doing recursive calls to event handling can cause hard to
find/reproduce bugs.

Has anyone considered the possibility to run XUL in one thread and the HTML in
another one? They should be largely separate worlds because of security if
nothing else. (No sharing of JS contexts or anything.)

Comment 24

[Mitsu Hadeishi]

I want to add in general that keeping the browser responsive at all times should
become a major priority, not just to deal with infinite loops, but to deal with
all sorts of delays.  For example, I have broadband and I typically set my
Google preferences set to return 100 items per page --- this greatly reduces the
time spent hitting the "next" button when I am looking for content.  However,
using Mozilla it can take several seconds before the browser decides to allow me
to click on a link in the search.  What's even more annoying is that when I hit
the "back" button to return to the search page, again the page is unresponsive
for a few seconds.  This on a Pentium II 400 laptop with 192 megs of RAM. 
Suffice it to say that IE performs much better in this respect, and this is one
of the main subjective performance issues that differentiates IE from Mozilla.

This occurs not only with Google searches but with many pages: links are not
active, it is impossible to even scroll a page, etc.  There are far too many
situations where potentially long processes that should be handled in the
background are completely tying up the main user interface thread.

Fixing this problem might require a great deal of work, I don't know, but I
think it ought to be a priority.

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Mitsu,
   Increasing performance and responsiveness is our top priority, as a matter of
fact. Thanks for your comments. However, this discussion is not relevant to this
particular bug - we try to keep discussion in bugs focused on the specific issue
at hand.

Comment 26

[Mitsu Hadeishi]

Sorry --- I had originally thought to enter a new bug, but I figured it would be
better to find a related bug and enter the comment there.  It seemed to me
(perhaps I am wrong) that in general, a single general approach would solve both
the problem of keeping the browser responsive during infinite loops and during
any extended operations.  I.e., the general principle being that nothing should
hold up the user interface thread for more than a very brief interval, and all
operations should be thread-safe.

If there is more appropriate bug or RFE which is already open where I should
place these comments, I'd be happy to do so.

Comment 27

[Brian 'netdragon' Bober]

See also bug 128666 about showing the display during tight loops.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[Scott Lamb]

This bug is very visible on large doxygen output with the tree browser. I've
created an example at <http://apt.slamb.org:8080/~slamb/std-doxygen/> (the
standard C++ library as included with g++ 3.0.4). It takes 270 seconds to load
on my 1GHz Athlon with 768MB of RAM. During that time, Mozilla is completely
unresponsive - it doesn't even handle expose events.

Comment 30

[Jesse Ruderman]

Scott: that's a general problem with loading content from a hard drive, from a
fast server, or from the cache.  See bug 129640, "Incremental rendering is
busted on fast connections".

Comment 31

[Scott Lamb]

The problem also occurs when loading over a modem. The problem is definitely
related to the Javascript that creates the tree frame. You haven't loaded the
page - give it a try and you'll see what I mean. If you disable Javascript, it
will load nearly instantly (without the tree). I think this is a good match for
(6) in this bug's description.

Comment 32

[Jesse Ruderman]

Sorry Scott, you're right.

Comment 33

[Matthias Versen [:Matti]]

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

Comment 34

[Hixie (not reading bugmail)]

Moving all threading bugs to XPCOM. See bug 160356.

Comment 35

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 37

[Andrew McCreight [:mccr8]]

This bug is 22 years old, and is mostly about a fairly general concern rather than some kind of specific issue. We do have the hang watchdog now which can be used to kill JS on a page if it is running too much, and in general e10s has stopped content JS from making the UI unresponsive.