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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout: Text
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Régis Caspar, Assigned: Uri Bernstein (Google))

Tracking

({perf})

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: cairo, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 218213 [details]
Slim testcase

Comment 2

12 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
I don't see a hang in non-cairo builds.
Component: View Source → GFX: Thebes
Product: Firefox → Core
Version: unspecified → Trunk
Whiteboard: cairo
QA Contact: view.source → thebes
(Assignee)

Comment 4

12 years ago
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
(Assignee)

Updated

12 years ago
Severity: minor → normal
Keywords: perf
(Assignee)

Comment 5

12 years ago
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.
(Assignee)

Comment 6

12 years ago
Created attachment 218517 [details]
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.
(Assignee)

Comment 7

12 years ago
Created attachment 218525 [details] [diff] [review]
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.
Attachment #218525 - Flags: review?(roc)
(Assignee)

Comment 8

12 years ago
Created attachment 218526 [details] [diff] [review]
patch?

The correct file, this time (hopefully).
Attachment #218525 - Attachment is obsolete: true
Attachment #218526 - Flags: review?(roc)
Attachment #218525 - Flags: review?(roc)
(Assignee)

Comment 9

12 years ago
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).
Blocks: 177442
Comment on attachment 218526 [details] [diff] [review]
patch?

looks good to me.
Attachment #218526 - Flags: superreview+
Attachment #218526 - Flags: review?(roc)
Attachment #218526 - Flags: review+
Uri, you're going to land this, right?
Flags: blocking1.9a1?
(Assignee)

Comment 12

12 years ago
(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.
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.
(Assignee)

Comment 14

12 years ago
(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.
(Assignee)

Updated

12 years ago
Assignee: nobody → uriber
(Assignee)

Comment 15

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Component: GFX: Thebes → Layout: BiDi Hebrew & Arabic
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Reporter)

Comment 16

12 years ago
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

12 years ago
Either this or bug 334319 bumped up Tp by 3-5%.
(Assignee)

Comment 18

12 years ago
(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)?
(Assignee)

Comment 19

12 years ago
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.
(Assignee)

Comment 20

12 years ago
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).
(Assignee)

Comment 21

12 years ago
Created attachment 219446 [details] [diff] [review]
patch for Tp regression

This fixes the issue described in previous comment, which hopefully means it would fix the Tp regression.
Attachment #219446 - Flags: superreview?(roc)
Attachment #219446 - Flags: review?(roc)

Comment 22

12 years ago
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
Attachment #219446 - Flags: superreview?(roc)
Attachment #219446 - Flags: superreview+
Attachment #219446 - Flags: review?(roc)
Attachment #219446 - Flags: review+
(Assignee)

Comment 23

12 years ago
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
(Assignee)

Comment 24

12 years ago
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.

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: thebes → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.