726637 - Firefox View Source Locks Up when "Wrap long lines" is on and a very long line of markup is used.

Status: NEW

(Core :: Layout, defect)

Description

[Steve Schmidt]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.1) Gecko/20100101 Firefox/10.0.1
Build ID: 20120208060813

Steps to reproduce:

After FF 10.0.1 update this morning.  Loaded company application web site - navigated to desired page.  Right-click... selected "View Frame Source"




Actual results:

Firefox locked up - both the Source window and the window that it came from.  Had to issue stop from Windows.


Expected results:

Frame Source should have displayed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Steve, I assume this is reliably reproducible?

Can you attach the source of the relevant subframe to this bug?

Comment 2

[Steve Schmidt]

Attachment: Source for view source problems

Yeah - I know it's large.  Uploading in two parts due to your size restriction. This size never presented problems prior to recent upgrade.  Chrome handles it OK.

Comment 3

[Steve Schmidt]

Attachment: Source for view source problems Part 2

Comment 4

[Steve Schmidt]

Works *slightly* better after a reboot.  Still takes a very long time.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Zipped-up version of the single HTML file

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, so I see this taking a very long time, presumably on those two long lines (or is it only one line in the original file?).  90+% of the total content of the file is on those lines, with no linebreaks.

Steve, do you have "wrap long lines" turned on in View Source?

I also tried profiling (with that option on); the profiler shows almost all the time (90% or more) is spent in inline reflow.  In particular, there's lots of destroying of inline frames, which leads to nsBlockFrame::DoRemoveFrame, which has to do lots of nsLineBox::IndexOf and then you lose.  40% of the wall-clock time is in nsLineBox::IndexOf itself.  The other parts that use time:

  15% destroying generated content nodes for frames.
   8% vm faults

But with that option on I see us being very slow here back to September builds that would have become Firefox 9.  So I'm guessing Steve does not have that option on.

Comment 7

[Steve Schmidt]

It was originally one long line before I split the file in two for upload.

"Wrap Long Lines" is on, as well as "Syntax Highlighting"  It runs much faster (tolerably so) without the Syntax Highlighting on.  It really runs "lickety-split" without wrap, but I think I'll keep that on.

I'm not sure how/where to turn profiling on/off.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  One long line makes sense.

It's odd that you were not seeing this freeze up before... What Firefox version were you on before updating to Fx10?  I assume you had "Wrap Long Lines" there too?

There is no profiler to turn on; I just used an external C profiler.  I meant that the "wrap long lines" option was on for me when I was profiling.

Comment 9

[Steve Schmidt]

I'm a ColdFusion developer and my Firefox versions have been pretty close to current since I inherited this PC last August.  I think that "Wrap" has almost always been on.  I almost certainly didn't have this particular sequence of data when using the View Source previously.  I usually use FireBug inspection to tell me what I need to know, but this time I put some HTML comments in place to track some internal application stuff.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  I'd read comment 0 as saying that the problem was new in Firefox 10, but I think that was just a misunderstanding.

roc, mats, any idea why we're destroying frames here to start with?  Seems a bit odd....

Comment 11

[Mats Palmgren (inactive)]

Some stats on the number of frames:

PresShell::InitialReflow, after frame construction, before reflow:
total 53300, text 33039 (0 text continuations)

fully reflowed (resulting in 8949 lines):
total 1198745, text 653778 (33200 text continuations)

calls to nsFrame::DestroyFrom:
total 1001, text 264

so I wouldn't worry about the frame destruction from a performance
point of view.  I looked at a few, they were empty next-in-flow
text frames being destroyed because the frame was complete:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp#1004

I noted some frames for the line numbering:

  Inline(span) ... <
    Block(_moz_generated_content_before) ...  pst=:before <
      line<
        Text(0)"7293"@0x69d8d08 [run=(nil)][0,4,T]  next=0x69d8d78 [content=0x6362730]
        Text(1)" "@0x69d8d78 [run=(nil)][0,1,T] [content=0x6362850]
      >
    >
  >

has two text nodes.  Also, :before generated content display:inline-block,
not sure how good that is from a reflow perspective.

Overall, I don't see anything surprising here, reflowing 1.2M frames with
~33k line breaks is slow.

Perhaps the View source content / styles can be optimized better for reflow?
And if the content is really big, turn off some syntax highlighting, line
numbering, line wrapping, etc. as a measure to avoid hanging the UI.

Comment 12

[Mats Palmgren (inactive)]

(Regarding the empty next-in-flow text frames, I seem to recall we intentionally
left those in the line in some part of the code that deals with line-breaking
as an optimization.  I don't know if that's the cause of those frames here though.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> so I wouldn't worry about the frame destruction

Well, except destroying a frame has to walk all frames that come before it in the block, right?  So we're doing that 1000 times....  Of course maybe I just ended up unlucky in when I profiled and the overall load is gated on something else.  But I also saw the quote-and-counter stuff spending a bunch of time on destruction for me...

Can we somehow prevent the creation of those next-in-flow frames?

> I noted some frames for the line numbering has two test nodes

The relevant style is:

65   content: counter(line) " ";

(in viewsource.css).  So yes, two text nodes.

> Perhaps the View source content / styles can be optimized better for reflow?

How?  We can change those however would help, but I'm not sure what would help here.

Comment 14

[Mats Palmgren (inactive)]

> Can we somehow prevent the creation of those next-in-flow frames?

I'll look into it...

> 65   content: counter(line) " ";
> 
> (in viewsource.css).  So yes, two text nodes.

That should be easily fixed.

> > Perhaps the View source content / styles can be optimized better for reflow?
> 
> How?  We can change those however would help, but I'm not sure what would
> help here.

I assume we have control over the generated markup so there's no reason
to shoot ourselves in the foot.  We should of course optimize reflow
as much we can, but meanwhile....

It seems what is killing us here is a huge single line markup that
view-source puts in a single block with "word-wrap: break-word".

Comment 15

[Mats Palmgren (inactive)]

Attachment: Use padding to pad

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It seems what is killing us here is a huge single line markup that
> view-source puts in a single block with "word-wrap: break-word".

Yes.  How else can it handle this situation?  We do have control over the generated markup; how would you make it better?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 596884 [details] [diff] [review]
Use padding to pad

r=me but maybe we should be filing bugs blocking this one and fixing them, instead of trying to do multiple different things here?

Comment 18

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> Yes.  How else can it handle this situation?  We do have control over the
> generated markup; how would you make it better?

If the number of generated bytes or elements for a block > N, end this block and
start a new one.  This should look mostly OK, but the line wrapping for that
single line of source markup will not be entirely correct.  Source line numbers
should still be correct.

If the total number of generated bytes or elements > N, then generate an error
message of some sort, and then send EOF.

And as I suggested earlier, degrade the syntax highlighting gradually when
the number of generated bytes or elements reach certain limits.

I don't think correctness is important in edge cases like this so we can
be as harsh as needed.

Comment 19

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> > so I wouldn't worry about the frame destruction
> 
> Well, except destroying a frame has to walk all frames that come before it
> in the block, right?  

Yeah, you're right.  I get 100% CPU for ~60 seconds, Shark tells me ~66% of that
is spent in nsLineBox::IndexOf, of which pretty much all comes from
nsBlockFrame::DoRemoveFrame.

Still, there's a lot that can be done to make the generated markup leaner.
I did a quick hack to generate something like this instead:
<ol>
 <li>This is source line 1
 <li>This is source line 2
 ...
</ol>
<ol start="1000"> This is source line 1000
...

It's more than twice as fast.  With ~4.5% in DoRemoveFrame.

I'll see if I can figure out why we delete these NIFs though...

Comment 20

[Boris Zbarsky [:bzbarsky]]

> It's more than twice as fast.  With ~4.5% in DoRemoveFrame.

That's odd; why would that make things better?  You're still ending up with a single <li> containing the really huge line, right?

Comment 21

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #20)
> That's odd; why would that make things better?  You're still ending up with
> a single <li> containing the really huge line, right?

Oops, I forgot to mention I also implemented the max-bytes per line thing:
For a single source line I wrapped every 10k source bytes in a <div> block.

Comment 22

[Mats Palmgren (inactive)]

Actually, looking at the patch I actually timed above, it was every 100 bytes...
Sorry, I was playing around testing different numbers...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, wrapping every 100 bytes is definitely no good.  That'd break common cases.  Wrapping every 10k bytes would be just fine.

Comment 24

[Mats Palmgren (inactive)]

Fwiw, here's the Shark numbers on my MBP 2011:

      original: 61s (62% DoRemoveFrame,  8% nsGenConList::DestroyNodesFor)
+  padding-fix:     no measurable difference from above
+  line-cursor: 20s (0%  DoRemoveFrame, 15% nsGenConList::DestroyNodesFor)
+  using <ol> : 12s (0%  DoRemoveFrame,  0% nsGenConList::DestroyNodesFor)

The "line-cursor" patch is in bug 727831.  "line-cursor" is the patch in this bug.
"using <ol>" is the approach in comment 19.

Comment 25

[Mats Palmgren (inactive)]

* "padding-fix" is the patch in this bug.

Comment 26

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.