333769 - Hang when trying to view source of a page (window-1255 encoding)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Régis Caspar]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060412 Firefox/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060412 Firefox/3.0a1

The page given as a demonstration of the problem generates a lot of content using javascript and there's a very long line of script into the page source. 
When trying to view source, browser hangs but after something like 2min here, source is displayed. 

Problem disappears if you change the page content-encoding or turn on "Wrap Long Lines" preference.

Reproducible: Always

Steps to Reproduce:
1. Turn off "wrap long lines" in source viewer
2. Visit URL
3. Edit -> View Page Source (or press CTRL+U)

Actual Results:  
browser hangs then displays page source

Expected Results:  
Page source is displayed almost immediately

Workaround: check "Wrap Long Lines" preference in source viewer.

Comment 1

[Régis Caspar]

Attachment: Slim testcase

Comment 2

[Daniel C]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060412 Firefox/3.0a1

For me, if I leave it for 1 minute 30 seconds, then the CPU usage comes back down from 100% and the source is displayed.

However, I'm going to go ahead and confirm this because it's an excessive amount of time for a 55 KiB file (not overly big), just complex in nature).

Comment 3

[Ria Klaassen (not reading all bugmail)]

I don't see a hang in non-cairo builds.

Comment 4

[Uri Bernstein]

On a (non-Cairo) trunk Mac build, viewing the source for the testcase took me 3 minutes and 15 seconds (195 seconds, during which the CPU is maxed).

So this might not really be a Cairo-only thing.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060410 Firefox/3.0a1

Comment 5

[Uri Bernstein]

Something very wrong is going on here in ComputeTotalWordDimensions / ComputeWordFragmentDimensions, although I don't understand this code well enough to say exactly where the problem is.

ComputeTotalWordDimensions is calling ComputeWordFragmentDimensions repeatedly, for each of the bidi continuation frames. But ComputeWordFragmentDimensions doesn't actually measure the text in that frame. Instead, it measures what seems to be a block of 4096 characters from the text (containing the frame), and therefore it keeps returning the width of that block to ComputeTotalWordDimensions, which keeps accumulating this in addedDimensions, which therefore reaches ridiculously high values.

Measuring the same (quite long) text over and over again is what's taking the long time (at least on my Mac). I'm not sure why the completely bogus returned dimensions don't have any effect on rendering.

Comment 6

[Uri Bernstein]

Attachment: testcase (without view-source)

This is the <script> element from the original testcase, HTML-escaped, and embedded in <div style="white-space:pre;">.
Rendering this testcase (not just it's source) takes a very long time.

Comment 7

[Uri Bernstein]

Attachment: patch?

This fixes it for me, by fixing what I described in the previous comment.
Notice also the comment in: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.160&mark=2890-2891#2875
(which was later removed).

I still don't feel I know this code well enough, so I can't vouch for the correctness of the patch.

Comment 8

[Uri Bernstein]

Attachment: patch?

The correct file, this time (hopefully).

Comment 9

[Uri Bernstein]

I just noticed that the nsTextTransformer part of the patch is identical to what Simon suggested for bug 177442 (three and a half years ago).

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 218526 [details] [diff] [review]
patch?

looks good to me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Uri, you're going to land this, right?

Comment 12

[Uri Bernstein]

(In reply to comment #11)
> Uri, you're going to land this, right?
> 

Yes, but I want to get an OK from smontagu first (just to make sure there wasn't a good reason he didn't do this for bug 177442), and I also want to make sure I'm around to deal with possible regressions - meaning that I'll probably land it this weekend or early next week.

Comment 13

[Simon Montagu :smontagu]

Sorry, I didn't know you were waiting for me. I held back in bug 177442 because I wanted to add similar logic to nsTextTransformer::ScanPreData_B, but by all means go ahead with this.

Comment 14

[Uri Bernstein]

(In reply to comment #13)
> Sorry, I didn't know you were waiting for me. I held back in bug 177442 because
> I wanted to add similar logic to nsTextTransformer::ScanPreData_B, but by all
> means go ahead with this.
> 
I wasn't really waiting for you (and you couldn't have known, because I didn't tell you). I didn't want to land this this week anyway, as I'm often not at a computer where I can do debugging/checkins. I just hoped to catch you sometime on IRC or something and ask you.
Anyway, thanks, and unless someone considers this urgent, I'll land it within a few days.

Comment 15

[Uri Bernstein]

Checked in:
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.571; previous revision: 1.570
done
Checking in layout/generic/nsTextTransformer.cpp;
/cvsroot/mozilla/layout/generic/nsTextTransformer.cpp,v  <--  nsTextTransformer.cpp
new revision: 1.112; previous revision: 1.111
done

Since the fix is not cairo-related, I changed the component to Bidi.
Can someone please verify that this indeed fixed the performance problem on Windows?

Also, it's very likely that cairo did actually regress the performance of bidi text-measuring on Windows, and while the current patch hopefully negates this regression in this case, we might want to look for other cases where the performance regression is apparent.

Comment 16

[Régis Caspar]

Verified. Source is displayed immediatly with "slim testcase"

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060422 Minefield/3.0a1 ID:2006042207 [cairo]

Comment 17

[jag (Peter Annema)]

Either this or bug 334319 bumped up Tp by 3-5%.

Comment 18

[Uri Bernstein]

(In reply to comment #17)
> Either this or bug 334319 bumped up Tp by 3-5%.
> 

Hm... almost certainly this one, I would say.

Should I back it out, or is there some investigation that could be done first? How do I find out which pages specifically took the hit (or was this across the board)?

Comment 19

[Uri Bernstein]

According to http://build-graphs.mozilla.org/graph/rawdata.cgi?tbox=argo.mozilla.org&testname=pageload&days=1
It seems the regression is entirely (or at least mostly) in lxr.mozilla.org which went from ~700 to ~2000.

Comment 20

[Uri Bernstein]

The problem seems to be that for the non-bidi case, the frame content offsets are not yet set when we reach this code (at least for the first time), so |nextFrameStart + contentLen < nextFrameEnd| is always false (nextFrameEnd is always 0).

Comment 21

[Uri Bernstein]

Attachment: patch for Tp regression

This fixes the issue described in previous comment, which hopefully means it would fix the Tp regression.

Comment 22

[jag (Peter Annema)]

For future reference: you can also find the per-page times in the full build log files (a bit easier to read):

IDX PATH                   AVG   MED   MAX   MIN   TIMES ...

17  lxr.mozilla.org        1752  1710  1934  1694  1934  1710  1694  1716  1710

17  lxr.mozilla.org        2734  2690  2905  2681  2905  2690  2681  2706  2688

Comment 23

[Uri Bernstein]

Regression patch checked in. I'll keep an eye on Tp to see how this goes.

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.572; previous revision: 1.571
done

Comment 24

[Uri Bernstein]

The fix for the Tp issue worked, as far as I can tell. Thanks to jag for noticing the problem, and to roc for the quick review.