271789 - Firefox hangs for 30 seconds or more when accessing this URL

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Pedro Timoteo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M2)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M2)

When accessing this page, Firefox 1.0 (on WinXP) hangs for 30 seconds or more
(on an Athlon XP 2000+, 1 GB of RAM, updated drivers and everything). My first
thought is that it wasn't recoverable, but I waited and, after that time, I had
control of the browser again.

It doesn't crash the browser, but, of course, if I try to close the Firefox
window while it's "hung", Windows gives the "this program is not responding"
message.

Firefox has no problem with other sites, and I browse a lot.


Reproducible: Always
Steps to Reproduce:
1. Go to http://www.playagain.net
2. On the left frame, click on "M.A.M.E."
3. On the main frame, click on "MAME 0.89" (I think the link may be updated as
future M.A.M.E. versions are released)

Actual Results:  
Part of the list appears, but then Firefox (all tabs & windows) "hangs" for 30
seconds or more. After that, the browser begins to work again, including that
particular URL (it's a big list of links).


Expected Results:  
Even if the list takes a while to download, the browser should not hang.

Comment 1

[Martijn Wargers (dead)]

I'm seeing this also with:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041125
Firefox/0.9.1+

Comment 2

[:Gavin Sharp [email: [e-mail]]]

That page contains a large table, so this may very likely be a less severe case
of  bug 148338. Happens in Moz1.7 as well.

Comment 3

[Dan G. Lunde]

I can confirm this behaviour aswell in Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.8a5) Gecko/20041125 Firefox/0.9.1+

Comment 4

[Martijn Wargers (dead)]

Attachment: Testcase

The first table row has an extra table cell. This table cell had also
rowspan="5314", but that doesn't seem to matter in rendering speed.
Removing the extra table cell drastically improves rendering speed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Usual deal:

Total hit count: 217827
Count %Total  Function Name
174214   80.0     nsCellMap::GetDataAt(nsTableCellMap&, int, int, int)

The main caller of this is nsCellMap::GetEffectiveColSpan, though
nsCellMap::RowIsSpannedInto and nsCellMap::ColHasSpanningCells etc also contribute.

Note that I did the profile with a Dec 2 build, but I don't think anything has
gone in since then that would affect this (the testcase does not set a rowspan
or colspan).

Comment 6

[Martijn Wargers (dead)]

The testcase freezes current trunk builds still up.

Comment 7

[Bernd]

Boris could you please make me a fresh profile once I landed the rowisspanned thing?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile of that attachment with the patch to bug 313295 applied

Highlights:

Total hit count: 266376
Count %Total  Function Name
218996   82.2     nsCellMap::GetDataAt(nsTableCellMap&, int, int, int)

nsCellMap::GetDataAt called from:

  163727 nsCellMap::GetEffectiveColSpan(nsTableCellMap&, int, int, int&)
   37735 nsCellMap::RowIsSpannedInto(nsTableCellMap&, int, int)
   16524 nsCellMap::ColHasSpanningCells(nsTableCellMap&, int)

nsCellMap::GetEffectiveColSpan is called via nsTableFrame::GetEffectiveColSpan, which is called by:

   55826 CalcAvailWidth(nsTableFrame&, int, float, nsTableCellFrame&, int, int&, int&)
   48339 nsTableCellFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&)
   44030 nsTableRowFrame::ReflowChildren(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, nsTableFrame&, unsigned&, int)
   15939 nsTableFrame::CellChangedWidth(nsTableCellFrame const&, int, int, int)

Comment 9

[Bernd]

Attachment: speedup

Boris, this should improve the profile. Could you please test with it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

With that patch, loading this page takes about 2-3 seconds over here and the profile says:

Total hit count: 42278
Count %Total  Function Name
8437   20.0     __libc_poll
1021   2.4     nsStyleContext::GetStyleData(nsStyleStructID)
860   2.0     __i686.get_pc_thunk.bx
765   1.8     nsRuleNode::GetStyleData(nsStyleStructID, nsStyleContext*, int)
607   1.4     SelectorMatches(RuleProcessorData&, nsCSSSelector*, int, nsIAtom*, signed char)
474   1.1     nsCellMap::GetDataAt(nsTableCellMap&, int, int, int)
446   1.1     nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, nsMargin*, nsMargin*)
443   1.0     __libc_calloc

So looks good to me.  ;)

Comment 11

[Bernd]

Comment on attachment 201947 [details] [diff] [review]
speedup

While the patch has only 5 meaningfull lines, its a rather drastic change. I thought of it for a long time and wondered how easy it is to implement. So for normal cells nothing changes. Things change for the cell holes. Before the patch the hole initiated a search for a orinating cell with zero span above or to the left. We searched as long as we did hit cell holes. What we do now is we say once the search does not give a positive result, there is no zero span above and to the left, its a dead cell. If now another  hole initiates a search and hits the dead cell it will stop searching in this direction. Usually we walk from top left, so basically we will typicall never lookup more than one cell hole in each direction instead of searching the whole table.
The bad thing is that the zero spans arent well tested and will take ages till regressions from this will surface.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 201947 [details] [diff] [review]
speedup

So the idea is that the next time we'll hit this cellData and bail out, right?

What impact will this have on the size of the cellmap in typical cases?  I'm assuming it won't be much except in pathological cases like this, right?

Comment 13

[Bernd]

Under normal conditions where each cellmap entry corresponds to a originating cell or is spanned by a row- or col span we will not increase the cellmap. If however due to too many col tags, or row- colspans  hooles are created then we will fill the hools.

Comment 14

[Bernd]

fix checked in

Comment 15

[Bernd]

.

Comment 16

[Caleb]

Did this cause a regression with GMail ?

The "Labels" and "Invite a friend" boxes seem to take up the whole page (width-wise).

Comment 17

[Boris Zbarsky [:bzbarsky]]

My build with this patch shows google just fine.  Is there a bug for this regression?

Comment 18

[Caleb]

(In reply to comment #17)
> My build with this patch shows google just fine.  Is there a bug for this
> regression?
> 

I've just filed bug 316040 and CC'ed you to it.

Comment 19

[Andrew Schultz]

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

Comment 20

[Benoît Dejean]

Is this bug fixed ?
I'm using Mozilla/5.0 (X11; U; Linux ppc; fr; rv:1.8.1.1) Gecko/20061205 Iceweasel/2.0.0.1 and can very reproduce with http://ompf.org/stuff/embroider/sample/radius.html
(And btw, the more i load this page, the more memory firefox uses)

Comment 21

[Benoît Dejean]

I have opened https://bugzilla.mozilla.org/show_bug.cgi?id=371718 with a better description. Again, i don't think the problem is about optimizing rendering, it's about making rendering not block the UI.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Is this bug fixed ?

Yes, but it was fixed in November 2005 and you're using a build with a rendering engine from August 2005 with security fixes since then.

Then again, you're testing on a different page from the one this bug was filed on, so....

> it's about making rendering not block the UI.

There are existing bugs on that.