86355 - [FIX]Mozilla hangs (100% cpu) when doing view source (bidi page)

Status: RESOLVED FIXED

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

Description

[Matthias Versen [:Matti]]

Reporter:
Please read http://www.mozilla.org/quality/bug-writing-guidelines.html and 
use in the future http://www.mozilla.org/quality/help/bug-form.html t file a new 
bug.

win2k build 20010617.. (CVS opt)
Ok I get a hang if I do this :
Load that URL. (this page use java, but the java applet is empty for me)
Open view source - hang

Comment 1

[doctor__j]

Disabling view-source syntax highlighting would solve this problem.

I remember there's a coloring-the-bad-HTML-code-causing-hang bug filed already.

Comment 2

[Asa Dotzler [:asa]]

over to XPApps GUI Features for triage.

Comment 3

[sairuh (rarely reading bugmail)]

dup

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

Comment 4

[John Morrison]

reopening ...

Comment 5

[John Morrison]

Yes, turning off the 'syntax coloring' for view-source does alleviate the 
'hang' when viewing the source of this page (and it is a known problem that 
syntax coloring is not fast).

However, I repetitively broke in the debugger (win2k) while this was churning,
and every single time it was with this stack trace below, iterating through
a list of frames.

Reassigning to Bidi for consideration/analysis.

nsBidiPresUtils::RemoveBidiContinuation(const nsBidiPresUtils * const 
0x01e644a0 const  nsDirectionalFrame::`vftable', nsIPresContext * 0x036c60f8, 
nsIFrame * 0x0373fb40, nsIFrame * 0x00000000, nsIContent * 0x036a2860, int & 
5346, int & 27) line 785 + 22 bytes
nsBidiPresUtils::Resolve(nsBidiPresUtils * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsIPresContext * 0x036c60f8, nsIFrame * 
0x0000001a, nsIFrame * 0x005b9278, int & 0) line 317 + 25 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x00000001, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 704
nsBlockReflowContext::DoReflowBlock(nsBlockReflowContext * const 0x01e644a0 
const  nsDirectionalFrame::`vftable', nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Initial, nsIFrame * 0x035b917c, const nsRect & {...}, int 
1239056, int 0, int 1239040, nsMargin & {...}, unsigned int & 0) line 573
nsBlockReflowContext::ReflowBlock(nsBlockReflowContext * const 0x01e644a0 const 
 nsDirectionalFrame::`vftable', nsIFrame * 0x00000002, const nsRect & {...}, 
int 0, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 340 + 34 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}, nsLineBox * 
0x00000000, int * 0x0012e99c) line 2960
nsBlockFrame::ReflowLine(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}, nsLineBox * 
0x036fd0c8, int * 0x0012e99c, int 1) line 2223
nsBlockFrame::ReflowDirtyLines(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}) line 2027
nsBlockFrame::Reflow(nsBlockFrame * const 0x00000000, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 795
nsBlockReflowContext::DoReflowBlock(nsBlockReflowContext * const 0x01e644a0 
const  nsDirectionalFrame::`vftable', nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Incremental, nsIFrame * 0x035b6cdc, const nsRect & {...}, int 
1240812, int 120, int 1240796, nsMargin & {...}, unsigned int & 0) line 573
nsBlockReflowContext::ReflowBlock(nsBlockReflowContext * const 0x01e644a0 const 
 nsDirectionalFrame::`vftable', nsIFrame * 0x00000002, const nsRect & {...}, 
int 1, int 0, int 1, nsMargin & {...}, unsigned int & 0) line 340 + 34 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}, nsLineBox * 
0x00000078, int * 0x0012f078) line 2960
nsBlockFrame::ReflowLine(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}, nsLineBox * 
0x035b6d64, int * 0x0012f078, int 1) line 2223
nsBlockFrame::ReflowDirtyLines(nsBlockFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBlockReflowState & {...}) line 2027
nsBlockFrame::Reflow(nsBlockFrame * const 0x00000000, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 795
nsContainerFrame::ReflowChild(nsContainerFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsIFrame * 0x035b6aac, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 
0, int 0, unsigned int 0, unsigned int & 0) line 747
CanvasFrame::Reflow(CanvasFrame * const 0x0368c970, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 579
nsBoxToBlockAdaptor::Reflow(nsBoxToBlockAdaptor * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBoxLayoutState & {...}, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0, int 0, int 0, int 9480, int 6450, int 1) line 869
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x00000000, 
nsBoxLayoutState & {...}) line 523 + 40 bytes
nsBox::Layout(nsBox * const 0x035b69b8, nsBoxLayoutState & {...}) line 985
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x00000005, 
nsBoxLayoutState & {...}) line 379
nsBox::Layout(nsBox * const 0x0368cd14, nsBoxLayoutState & {...}) line 985
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x00000000, 
const nsRect & {...}) line 591 + 9 bytes
nsGfxScrollFrameInner::LayoutBox(nsGfxScrollFrameInner * const 0x01e644a0 const 
 nsDirectionalFrame::`vftable', nsBoxLayoutState & {...}, nsIBox * 0x0368cd14, 
const nsRect & {...}) line 1037 + 17 bytes
nsGfxScrollFrameInner::Layout(nsGfxScrollFrameInner * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsBoxLayoutState & {...}) line 1150
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x0368caac, 
nsBoxLayoutState & {...}) line 1047
nsBox::Layout(nsBox * const 0x0368caac, nsBoxLayoutState & {...}) line 985
nsBoxFrame::Reflow(nsBoxFrame * const 0x0368ca78, nsIPresContext * 0x036c60f8, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 781
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x0368ca78, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 738
nsContainerFrame::ReflowChild(nsContainerFrame * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsIFrame * 0x0368ca78, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 
0, int 0, unsigned int 0, unsigned int & 0) line 747
ViewportFrame::Reflow(ViewportFrame * const 0x0368c938, nsIPresContext * 
0x036c60f8, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 538
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x036d21b0, 
nsIPresContext * 0x036c60f8, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 145
PresShell::ProcessReflowCommand(PresShell * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', nsVoidArray & {...}, int 1, nsHTMLReflowMetrics 
& {...}, nsSize & {...}, nsIRenderingContext & {...}) line 5717
PresShell::ProcessReflowCommands(PresShell * const 0x01e644a0 const  
nsDirectionalFrame::`vftable', int 1) line 5772
ReflowEvent::HandleEvent(ReflowEvent * const 0x01e644a0 const  
nsDirectionalFrame::`vftable') line 5630
PL_HandleEvent(PLEvent * 0x036fd2b8) line 591
PL_ProcessPendingEvents(PLEventQueue * 0x1002aa70) line 520 + 6 bytes
_md_EventReceiverProc(HWND__ * 0x00ced528, unsigned int 4200147, unsigned int 
12859920, long 0) line 1071 + 10 bytes
nsAppShellService::Run(nsAppShellService * const 0x00c43a10) line 418
main1(int 1, char * * 0x00322db0, nsISupports * 0x00324210) line 1139 + 9 bytes
main(int 1, char * * 0x00322db0) line 1437 + 25 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x00133226, 
HINSTANCE__ * 0x00400000) line 1455 + 21 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e87903()

Comment 6

[John Morrison]

tweak summary.

Comment 7

[Simon Montagu :smontagu]

I have been looking into this. All the <span>s in the source with the syntax 
higlighting make the bidi resolution very involved, especially since the whole 
source is one huge block frame. We should find a way to bypass all that 
processing, maybe by letting the code that outputs the source do its own bidi 
reordering in advance.

Comment 8

[Frank Tang]

simon- isn't it true that it is possible for peopel to write a html just like
what the view source process generated ? View Source is simply a filter which
transform one html to another html. Do we have some flaw in our bidi processing
which cause infinite loop? 

Comment 9

[Zach Lipton [:zach]]

Mass-move all BiDi Hebrew and Arabic qa to me, [e-mail]. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 

Comment 10

[Shoshannah Forbes]

win98 Mozilla build ID: 2001091703 , syntax coloring turned off:
Attempting to view a source of a Hebrew page hangs the "view source" window, but
not the rest of Netscape.
Viewing English pages works fine.

Comment 11

[John Morrison]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

I just did a profile of view source on this page (no hang, just took a bit). 
The result was definitely pointing at bidi as the main perf hog.  About 60% of
the time was spent in (not under, but _in_)
nsBidiPresUtils::RemoveBidiContinuation.  About 83% of the total hits were in
nsBidiPresUtils::Resolve.

So do we have any traction here?  You say that view source being a single block
frame makes this slow.  There is currently a proposal to split up the single
<pre> into multiple <pre> elements (splitting every 500 tokens or so, but only
on newline tokens).  Would that help the situation here?  Anything else we can do?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'd also like to reiterate Frank's comments.  In my mind, view source is the
perfect "big text document with lots of markup" testcase.  If we can improve
performance here, chances are we also improve performance on bidi pages across
the board.

Comment 14

[Simon Montagu :smontagu]

>If we can improve performance here, chances are we also improve performance on
>bidi pages across the board.

I can't argue with that, but let me stress that view source is very untypical of
bidi documents. A typical document will tend to have runs of text predominately
in one direction interspersed with occasional (usually short) runs in the other
direction, so |runCount|, which determines the number of iterations in
|nsBidiPresUtils::Resolve| is typically quite small (10 would be unusually large).

In view source on the other hand, |runCount| can easily run into 3 or 4 figures,
which is why we get stuck in |nsBidiPresUtils::Resolve| for such a long time.
Obviously anything we can do to improve performance inside the main for loop
there will help a lot for view source, but may not be a significant win in other
contexts.

I will experiment with turning on the DUMP_TO_FILE option in
nsViewSourceHTML.cpp and reformatting the output file to see the effect of
splitting it into multiple <pre> elements.

By the way, you can get good realworld examples of largish bidi pages with lots
of markup by setting preferences at www.google.com to Interface Language: Hebrew
and Number of Results: 100

Comment 15

[Simon Montagu :smontagu]

Attachment: view source dumped to file

Comment 16

[Simon Montagu :smontagu]

Attachment: Attachment 58061 with each line wrapped in <pre></pre>

Comment 17

[Simon Montagu :smontagu]

Comparing pageload of attachment 58061 [details] and attachment 58062 [details], it looks as if
increasing the number of <pre> elements will do wonders for this bug. I'm seeing
about a 300% improvement.

Comment 18

[Dariusz J. Garbowski]

Win 2000, Mozilla 2002010303:
URL: http://www.scan.co.uk/allprice.htm

Viewing page source with syntax highlight takes long time to show it (and "eats"
CPU power) even on Pentium4/1.5 GHz. Syntax highlight off, and everything is ok.

Other interesting behaviour is that when the source is rendered mouse pointer is 
"jumping" instead of smooth moving, but... when the source is scrolled to the 
bottom - mouse pointer moves ok.

[There are other strange things about this webpage and Mozilla, but not 
connected with this bug]

Comment 19

[Boris Zbarsky [:bzbarsky]]

That's not likely to be related to this bug, since the page in question has no 
BIDI content on it.  This bug is specific to BIDI pages.

Comment 20

[Dariusz J. Garbowski]

OK, so I opened new bug entry #118167.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: patch implementing ideas from comment 12 and comment 17

This is based on Andreas Otte's work for bug 57724, which seems to be unlikely
to land anytime soon.

The patch speeds up the http://www.ynet.co.il site by a factor of 5 and a
google search result page (Hebrew, 100 results, search for "mozilla") by a
factor of 13.  It does not seem to affect the time taken to show the source of
cnn.com (a non-bidi page) in any sort of measurable way.  I also tried timing
load of source for http://www.toronto.com (subject of bug 114420).  There I
just timed to "first visible source" not to "pageload complete".  There was a
bit of a difference in favor of the code with this change.

All the profiles are at http://www.zbarsky.org:8000/~bzbarsky/viewsource/
The filenames are of the form jprof-whenSiteTokens.#.html.  For example,
jprof-afterynet128.1.html is loading the ynet.co.il site with the patch (hence
"after") with NS_VIEWSOURCE_TOKENS_PER_BLOCK set to 128, and that's data point
#1 for that configuration.

This brings me to the last issue.  I tested various values of
NS_VIEWSOURCE_TOKENS_PER_BLOCK.  This is the constant that determines how often
we start new <pre> blocks.  I got down to 8 before I stopped testing -- at 8
tokens/block we're putting one or at most two lines of source inside each
block.	The patch I'm attaching has the constant set to 8, because that value
produced the best results on the test pages in question.  More testing on a
variety of pages and with various values of that constant would be much
appreciated.  Simon?  rbs?  Thoughts?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Correction, it's Andreas Schneider's work that the patch is based on.  I need to
learn to keep two thoughts in my head at once...

Comment 23

[rbs]

Comment on attachment 67620 [details] [diff] [review]
patch implementing ideas from comment 12 and comment 17

These two stay next to each other:
+	   result = mSink->OpenBody(bodyNode);
+	   if(NS_SUCCEEDED(result)) mHasOpenBody=PR_TRUE;
+	 }
+	 IF_FREE(bodyToken,theAllocator);	 

r=rbs, patch looks okay since someone who wants to go back to the old world can
pick a big value for NS_VIEWSOURCE_TOKENS_PER_BLOCK. But the default of one
block frame per line looks overkill, since this comes at the price of
additional memory. If you test something like 15 lines (i.e., the average when
keying pageup/pagedown) and see that it is okay, it might be sufficient.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.1

Move the mHasOpenBody line back as rbs suggests, make it easy to disable the
whole setup (by setting NS_VIEWSOURCE_TOKENS_PER_BLOCK to 0), only set
NS_VIEWSOURCE_TOKENS_PER_BLOCK nonzero when IBMBIDI is defined, make the token
count increment only for newline/text/whitespace tokens, raise the threshold
which triggers a new block.  Now each block has 2-35 lines on the pages I have
tested (since a single token can be many lines, eg a <script> tag's contents). 
Performance is a bit worse than with my original patch, still 4 times faster on
ynet.co.il, 10 times faster on google.

Comment 25

[rbs]

Comment on attachment 71583 [details] [diff] [review]
Patch v 1.1

Just remove the numerous #ifdef's and replace tests such as
+	 if (mTokenCount > NS_VIEWSOURCE_TOKENS_PER_BLOCK)
with 
+	 if (NS_VIEWSOURCE_TOKENS_PER_BLOCK > 0 &&
+	    mTokenCount > NS_VIEWSOURCE_TOKENS_PER_BLOCK)

(since the partitionning is good, let everybody benefit from it by default, not
just when bidi is on)

r=rbs applies from now on.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v1.2

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 71612 [details] [diff] [review]
Patch v1.2

transferring review

Comment 28

[Boris Zbarsky [:bzbarsky]]

Taking this, since I'm doing the fix anyway.

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 71612 [details] [diff] [review]
Patch v1.2

sr=jst

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 71612 [details] [diff] [review]
Patch v1.2

a=asa (on behalf of drivers) for checkin to 0.9.9.  This was approved in email,
I'm just flagging the bug.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Checked in.  Page is now most affected by bug 98118.  Marking this one fixed.

Comment 32

[Henri Sivonen (:hsivonen)]

(In reply to comment #21)
> This brings me to the last issue.  I tested various values of
> NS_VIEWSOURCE_TOKENS_PER_BLOCK.  This is the constant that determines how often
> we start new <pre> blocks.  I got down to 8 before I stopped testing -- at 8
> tokens/block we're putting one or at most two lines of source inside each
> block.

Was there a reason to make the breaking work by counting tokens instead of putting each line in a <pre> without counting tokens?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yes.  The more <pre> the more frames and DOM nodes, the more memory usage and CPU time needed to show the page.  Since we only need the multiple <pre> to work around bidi being dumb, we end up with needing a tradeoff between bidi taking too much time and normal layout taking too much time/memory.