218511 - Mozilla hangs on http://www.joki-foto.de/fotoshop/fehler.htm

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Xuân Baldauf]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030718
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030718

When accessing http://www.joki-foto.de/fotoshop/fehler.htm, mozilla seems to
loop endlessly. No user interaction will be possible anymore. I suspect a loop
within the JavaScript code of that page. But even if there is such a loop, the
user should be able to continue browsing instead of killing mozilla.


Reproducible: Always

Steps to Reproduce:
1. Access http://www.joki-foto.de/fotoshop/fehler.htm
2. Try to do anything with mozilla (i.e. open a new browser windows)
Actual Results:  
Mozilla hangs. It eats all available CPU time.

Expected Results:  
Mozilla should not hang. Mozilla should still do user interaction. Mozilla
should  enable the "Stop" button so that some looping JavaScript code can be
interrupted if the user desires that.

Comment 1

[dolphinling]

Works for me 2003090505 Linux after stalling for 45 seconds.

I suspect this is due to the 1.66 megabyte JS file it links to at
http://www.joki-foto.de/fotoshop/search.js

JS console gives no errors.

Comment 2

[José Jeria]

WFM Gecko/20030902 Firebird/0.6.1+. Does not hang, it just takes long time to
load the page. 

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not likely to be a JS engine bug per se... over to General until we have a
minimal testcase that shows the problem.

If this is just a perf issue, perhaps a profile of the pageload is in order?

Comment 4

[Boris Zbarsky [:bzbarsky]]

.

Comment 5

[Jon Henry]

I'm working up a reduced testcase for this. As Jose noted, the page does load
eventually, but it takes well over a minute even on a reasonably new machine.

In a nutshell, the perf problem looks like it's caused by the page creating a
massive array of arrays in Javascript. The source code that creates the arrays
is roughly 1.7 megabytes of text. The first few lines look like this:

var articles = new Array(
new Array('1 Agfa Pan 100 120','150656','','','2.87','article_g000_000.html',''),
new Array('1 Agfa Pan 100 135/24','577189','','','2.81','article_g000_001.html',''),

and so on for hundreds of lines. While this is not great coding practice, IE6
does manage to render it in about 10 seconds as opposed to 2 minutes so it seems
like something could be improved. If it will help I can attach the full 1.7
megabyte testcase, or a smaller version that still gets the point across.

This may be a dupe of or related to bug 171262.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Does it just create the array, or does it create DOM nodes from it?  I would
assume the latter...

If you have a functioning testcase that you can zip up (due to the size this may
be needed) and attach here that shows the pageload issue, please go for it.

Comment 7

[Jon Henry]

Attachment: Testcase

Here is the zipped testcase I created.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, how interesting.  This _is_ a JS engine issue after all.  Though I do
wonder why the fix for bug 13350 is not kicking in.

The profile shows:

Total hit count: 10809
Count %Total  Function Name
10666   98.7     GetChar
 27   0.2     FindCharInReadable
  7   0.1     nsTextFragment::SetBidiFlag
  6   0.1     memmove

Everything else is below the 0.1% level.

Of the 10666 hits in GetChar, we have (more or less; the sum will be greater
than 10666 because a few of those hits are actually under js_AtomizeChars, not
GetChar):

   572 coming from js_GetToken
   174 coming from js_PeekToken
   131 coming from UnaryExpr
  1189 coming from js_MatchToken
  8751 coming from UnaryExpr

It looks like the only callstack that ends up in UnaryExpr is:

UnaryExpr
MulExpr
AddExpr
ShiftExpr
RelExpr
EqExpr
BitAndExpr
BitXorExpr
BitOrExpr
AndExpr
OrExpr
CondExpr
AssignExpr

10704 of the total 10809 hits are under AssignExpr.

Moving on up, it looks like most of that stuff is under

AssignExpr
Variables
Statements
js_CompileTokenStream  (10707 of the total hits).

That probably explains why the DOM can't interrupt this mess -- this is an
atomic compilation operation....

Comment 9

[Boris Zbarsky [:bzbarsky]]

The issue seems to be that the script is all a single line.  After running:

  perl -pi -e 's/new Array/\nnew Array/g;' testcase.htm

the file loads in Mozilla in about five seconds (still freeze up for those five
seconds, though).

Comment 10

[Brendan Eich [:brendan]]

Big fun -- thanks for the profile, bz.

(No one should put everything on one line!  That's just wrong ;-)

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix (apply this)

Reviewers: diff -w version coming right up.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch (review this)

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 142783 [details] [diff] [review]
diff -w version of last patch (review this)

I like pathology. r=shaver

Comment 14

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, with that patch 6 pageloads give me:

Total hit count: 1087

Count %Total  Function Name
171   15.7     FindCharInReadable
 78   7.2     GetChar
 45   4.1     nsTextFragment::SetBidiFlag
 41   3.8     memcpy
 31   2.9     FindInReadable
 30   2.8     memmove
 28   2.6     js_GetToken
 21   1.9     js_MatchToken
 20   1.8     js_SearchScope

So GetChar is still up there, but it's much better now... (like nearly 4 orders
of magnitude better).

I filed bug 236272 on looking into making the the code that's calling
FindCharInReadable (CTextToken::ConsumeUntil) a litte faster.