241454 - Table data overflowing due to <script> tag - gets fixed when a button is pressed

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Doron Rosenberg (IBM)]

Looks like an incremental reflow issue dealing with tables.

Note that the issue happens on an IBM web product, and in 1.4/1.6, it is only
reproducable on the server, not locally.  However, 1.7 branch (and trunk it
seems) can see this issue locally.  The issue did not happen in 0.9.4, but
probably not helpfull.

Probably a dupe in the end, but I have a nice 50 line testcase.

Comment 1

[Doron Rosenberg (IBM)]

Attachment: Testcase of the issue.

Comment 2

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

Definitely a bug there, but why do you say "text should overflow table border"?

And are you sure this can't be minimized further?

Comment 3

[Doron Rosenberg (IBM)]

Yeah, the text name wasn't a good choice :) It basically is there to show that
the text overflows becuase of the bug.

I tried to minimize it more, but that usually doest show the problem anymore. 
For example, removig height="100%" from the <td> "fixes" the issue.  I'll
continue to see if I can get it smaller.

Comment 4

[Doron Rosenberg (IBM)]

Attachment: even smaller testcase

A smaller testcase

Comment 5

[Bernd]

Attachment: reflow log for attachment 146856

so what happens here is that the row group is target of a incr. reflow (dirty)
so it resizes all its children, this will cause instead of initial reflow a
resize reflow on ther block inside the table cell. And this resize reflow does
not ask for the for the mew. The inner	block can hold its content so it goes
into the overflow

cell 026D59D0 r=2 a=48,UC c=0,UC cnt=890 
 block 026D5A30 r=2 a=24,UC c=24,UC cnt=891 
.
.
.	 
 block 026D5A30 d=24,264 o=(0,0) 2364 x 264
cell 026D59D0 d=48,288 

then the row is target for an incr. reflow and this causes  a incr. reflow on
the previously resized block and which is asked  politely to return a max
element width.
The block then says some f-word  and returns a 0 mew back and everything goes
into the overflow area.

cell 026D59D0 r=1 a=48,UC c=UC,UC cnt=907 
 block 026D5A30 r=1 incr. (Dirty) a=0,UC c=UC,UC cnt=908 
 block 026D5A30 d=0,264 me=0 m=0 o=(0,0) 2364 x 264
cell 026D59D0 d=72,312 me=72 m=72 o=(0,0) 0 x 0sto=(0,0) 2388 x 288

Comment 6

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

So is the problem that there's no initial reflow on the table cell block?

Comment 7

[Doron Rosenberg (IBM)]

Marking as important for IBM.

Comment 8

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

I think the thing to do here is to fix this:

> this will cause instead of initial reflow a resize reflow on ther block inside
> the table cell. And this resize reflow does not ask for the for the mew.

so that it *does* ask for the MEW in this case, possibly by inspecting the
reflow command tree to see if there are incremental reflows targeted at frames
further down the tree.

Comment 9

[Doron Rosenberg (IBM)]

Even though the testcase does work fine in 1.4-1.7a, the actuall web application
shows the broken behavior the test shows in 1.7 branch builds.

Comment 10

[Doron Rosenberg (IBM)]

Lemme rephrase - the testcase does not show the problem in 1.4-1.7a, however the
actuall web application still shows the overlapping problem

Comment 11

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

It seems to me that if we have a frame A that may do an incremental reflow of
its child frame B, where A asks for B's MEW even if A's parent did not ask for
A's MEW, then A must *always* demand B's MEW on every reflow. Otherwise bad
things can happen, like in this case a block descendant of B ends up missing MEW
information that the incremental reflow needs.

It should not be hard to make this true.

Comment 12

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

Attachment: fix

This fixes one instance of what I said, in nsTableRowFrame, which fixes this
bug.

The change in nsTableRowGroupFrame seems correct but I'm less sure about it.

Both of these changes should be quite safe for the branch, but they may
(probably will) impact Tp a bit. I'd really like bernd to take a look before we
check in though. And doron should test the change against the IBM apps in
question.

Comment 13

[Bernd]

I am not sure that I like the patch. First of all Requesting a MEW in the
rowframe without a matching SetPass1MaxElementWidth  and
aTableFrame.CellChangedWidth seems to be wrong. Second wouldnt it be more
correct initialize
desiredSize with aDesiredSize.mComputeMEW ? and then set watch cell to true?

Comment 14

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

> First of all Requesting a MEW in the rowframe without a matching
> SetPass1MaxElementWidth  and aTableFrame.CellChangedWidth seems to be wrong.

Why? I don't want to use the MEW. I just want to make sure that I can ask for it
later during incremental reflow.

> Second wouldnt it be more correct initialize desiredSize with
> aDesiredSize.mComputeMEW ? and then set watch cell to true?

Again no. I don't want to watch all cells. I'm not actually going to look at the
MEW in any more situations that we currently do. I just want to make sure that
it computes its MEW so later when I ask for its MEW during incremental reflow,
it will be able to do that incrementally.

Comment 15

[Bernd]

Ok next question:
why dont you place that in nsTableCellFrame::Reflow?
for instance at
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.cpp#850
setting it to PR_TRUE would cause only the blocks to report the MEW always.

Comment 16

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

You mean in the construction of kidSize here?
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.cpp#800

I think that would work. However that would disocciate the forcing of the MEW
computation from the code that is actually the reason for forcing the MEW. It is
the row code that needs MEW for selected cells even though the parent did not
ask for MEW, so I think it's a little cleaner to have the row code also be
responsible for forcing MEW computation for all cells.

Comment 17

[Doron Rosenberg (IBM)]

The fix seems to fix the issue on the actual web application as well, so my
testcase was actually to the point :)

Comment 18

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

Bernd and I had a discussion on IRC. The unresolved question here, that needs
more input, is what the rules should be for when a frame is allowed to ask its
children for the MEW.

The patch currently attached to this bug assumes that the rule is (1) "If a
frame will *ever* ask a child for MEW, it must *always* ask the child for MEW."
This is the easiest for child frames (e.g., blocks) to handle.

Another possibility is (2) "If a frame will *ever* ask a child for MEW, it must
ask the child for MEW in the initial reflow." This is fairly easy for child
frames (e.g., blocks) to handle, because they can store the MEW-required state
in a flag during the intial reflow, and then consult that flag later to see if
MEW data needs to be computed and cached for later use.

A third possibility is (3) "A frame can always ask its child for MEW at any
time" (i.e., there is no rule :-) ). I believe this is the current de facto
situation. This would require some changes in blocks, at least. For example, we
could record on each line whether we know the MEW for that line; if the caller
asks for the MEW, then we need to scan each line and for any line where we don't
currently know the MEW, we'll have to mark that line dirty and reflow it to get
the MEW. This might not be too expensive if we can fold this line traversal into
an existing line traversal.

Of course all this stuff will go away or be highly modified with dbaron's
refactoring. But something like one of these rules might persist.

I believe (1) gives us the smallest, safest change for the 1.7 branch. I think
it's also a reasonable rule in its own right. I'm not sure bernd agrees, yet...

Comment 19

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

> This is the easiest for child frames (e.g., blocks) to handle.

I meant to add "because the mComputeMEW flag tells the child not only whether
MEW is required for the current reflow, but also whether the MEW is asked for in
all previous and all future reflows"

Comment 20

[Bernd]

I agree that (1) is the smallest change. My previous experience with the MEW
table + block code interface tells me there is no safe change. For me any of the
rules, if it is a agreed on rule, is fine. At least it is much easier to ensure
that a certain rule is enforced in the code compared to a situation if there are
more than one unwritten rule which may clash (the current situation). It would
be nice if David could write down his vision of how the things should evolve. So
if you plan to go with (1) and the rtest doesnt fail, lets try it.

Comment 21

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

> This might not be too expensive if we can fold this line traversal into
> an existing line traversal.

Well, the MEW-availability-check traversal might not be too expensive. But it
might mean we end up reflowing some lines twice, the second time just to get the
MEW, where we could have just computed MEW in the first pass, and that could be
expensive.

Comment 22

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

Comment on attachment 149589 [details] [diff] [review]
fix

Requesting reviews on the assumption that this passes the regression tests,
which I believe doron is running :-)

Comment 23

[Doron Rosenberg (IBM)]

grep 'rgd failed' <regression.txt

regression test verify\5847.rgd failed
regression test verify\13196.rgd failed
regression test verify\14118-b.rgd failed
regression test verify\16173.rgd failed
regression test verify\28811.rgd failed
regression test verify\169620.rgd failed
regression test verify\127286-1.rgd failed
regression test verify\150652.rgd failed
regression test verify\163614.rgd failed
regression test verify\cell_heights.rgd failed
regression test verify\columns.rgd failed
regression test verify\col_span.rgd failed
regression test verify\col_span2.rgd failed
regression test verify\col_widths_fix_fixPer.rgd failed
regression test verify\one_row.rgd failed
regression test verify\test4.rgd failed
regression test verify\test6.rgd failed
regression test verify\bug101759.rgd failed
regression test verify\bug10269-2.rgd failed
regression test verify\bug106966.rgd failed
regression test verify\bug113235-1.rgd failed
regression test verify\bug1261.rgd failed
regression test verify\bug12012.rgd failed
regression test verify\bug14007-2.rgd failed
regression test verify\bug14159-1.rgd failed
regression test verify\bug14323.rgd failed
regression test verify\bug15247.rgd failed
regression test verify\bug16252.rgd failed
regression test verify\bug18955.rgd failed
regression test verify\bug20579.rgd failed
regression test verify\bug2479-5.rgd failed
regression test verify\bug25707.rgd failed
regression test verify\bug29058-3.rgd failed
regression test verify\bug2973.rgd failed
regression test verify\bug3166-7.rgd failed
regression test verify\bug3166-9.rgd failed
regression test verify\bug3681-1.rgd failed
regression test verify\bug43039.rgd failed
regression test verify\bug43854-2.rgd failed
regression test verify\bug44505.rgd failed
regression test verify\bug4427.rgd failed
regression test verify\bug47163.rgd failed
regression test verify\bug4739.rgd failed
regression test verify\bug51000.rgd failed
regression test verify\bug56024.rgd failed
regression test verify\bug56024.rgd failed
regression test verify\bug5797.rgd failed
regression test verify\bug5799.rgd failed
regression test verify\bug6404.rgd failed
regression test verify\bug650.rgd failed
regression test verify\bug69382-1.rgd failed
regression test verify\bug7243.rgd failed
regression test verify\bug78162.rgd failed
regression test verify\bug80762-2.rgd failed
regression test verify\bug82946-1.rgd failed
regression test verify\bug82946-2.rgd failed
regression test verify\bug96334.rgd failed
regression test verify\bug96343.rgd failed
regression test verify\97619.rgd failed
regression test verify\bug101674.rgd failed
regression test verify\attachment.rgd failed
regression test verify\tables_id.rgd failed
regression test verify\x_col_width_rel.rgd failed
regression test verify\x_colgroup_width_px.rgd failed
regression test verify\x_colgroup_width_pct.rgd failed
regression test verify\x_colgroup_width_rel.rgd failed
regression test verify\x_table_class.rgd failed
regression test verify\cell_widths.rgd failed
regression test verify\wa_table_thtd_rowspan.rgd failed
regression test verify\wa_table_tr_align.rgd failed
regression test verify\wf_table_index.rgd failed
regression test verify\nested2.rgd failed
regression test verify\bug123983.rgd failed
regression test verify\bug125543.rgd failed
regression test verify\bug138349-1.rgd failed
regression test verify\bug153785-1.rgd failed
regression test verify\bug153785-3.rgd failed
regression test verify\bug153785-4.rgd failed
regression test verify\bug165772-1.rgd failed
regression test verify\bug57378.rgd failed
regression test verify\bug57467.rgd failed
regression test verify\bug59280-1.rgd failed
regression test verify\bug66804.rgd failed
regression test verify\bug82401.rgd failed
regression test verify\splitCell-1.rgd failed
regression test verify\select_onchange.rgd failed


grep 'rgd failed' <false_positive.txt

regression test verify\test4.rgd failed
regression test verify\bug57467.rgd failed
regression test verify\select_init.rgd failed

Anything I can do to figure if those are real ones?

Comment 24

[Doron Rosenberg (IBM)]

Removing the nsTableRowGroupFrame changes didn't help, going to work more on
this tomorrow.

Comment 25

[Doron Rosenberg (IBM)]

Bernd says that
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/28811.html
has a big difference in the reflow tree, visually the only thing I can see is
that the last input is a bit more to the right than before the patch.

Comment 26

[Doron Rosenberg (IBM)]

I think I found a useable example:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/viewer_tests/test4.html

Go to the very end, the last 2 tables.

With the patch, the "after" case looses data - the C12 td is missing.

Similar behavior for the other before/after examples. - the collapsed areas
affect the uncollapsed parts.

Comment 27

[Doron Rosenberg (IBM)]

Another one:

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug106966.html

The third table, the first "contentwidth" gets cutoff at the yellow border,
while with a non-patched build I see text overflow.

Comment 28

[Doron Rosenberg (IBM)]

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/marvin/x_col_width_rel.xml

The 2nd <td>, with the new patch, the text is aligned to the middle, while
without its at the top (as expected).

Comment 29

[Bernd]

Robert I did run the rtest and get the same devastating result as doron. It
seems to me that the assumption, there is no rule currently is wrong. I don't
think that the patch is safe its just the opposite of it. If this should really
go on a branch the rtest failures needs to be either eliminated or explained in
a way that  shows that the failures are bogus and the patch will really improve
the picture, e.g. the failures are improvements in the rendering of the testcases.

Comment 30

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

I understand. This won't go on the branch anytime soon.

Comment 31

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Marking blocking1.7+, although this is at risk for being minused, since we're
hoping to ship early next week.

Comment 32

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

(In reply to comment #27)
> Another one:
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug106966.html
> 
> The third table, the first "contentwidth" gets cutoff at the yellow border,
> while with a non-patched build I see text overflow.

Actually in this testcase I'm seeing bad overflow rect calculation which makes
painting erratic in all of 1.7rc2, trunk-with-this-patch, and 2004032015. So
let's ignore this one...

Comment 33

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

(In reply to comment #26)
> I think I found a useable example:
>
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/viewer_tests/test4.html
> 
> Go to the very end, the last 2 tables.
> 
> With the patch, the "after" case looses data - the C12 td is missing.

It *should* be missing --- it's collapsed!

In 1.7rc2 and 2004032015, I get erratic behaviour: on an uncached load
(shift-reload) the collapsed columns are blank, rather than collapsed. When I
reload, the columns are collapsed to look like the trunk-with-patch. So I think
the patch is probably fixing a bug here.

> Similar behavior for the other before/after examples. - the collapsed areas
> affect the uncollapsed parts.

Not sure what you mean.

Comment 34

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

(In reply to comment #33)
> (In reply to comment #26)
> > Similar behavior for the other before/after examples. - the collapsed areas
> > affect the uncollapsed parts.
> 
> Not sure what you mean.

Well, I see the same issues in the other testcases as for the very last
testcase: on an initial load, *without* my patch the cells are not correctly
collapsed, *with* my patch they are.

Comment 35

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

(In reply to comment #28)
>
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/marvin/x_col_width_rel.xml
> 
> The 2nd <td>, with the new patch, the text is aligned to the middle, while
> without its at the top (as expected).

Actually the default vertical alignment for cells is 'middle' and the testcase
doesn't override it.
http://www.w3.org/TR/html401/struct/tables.html#adef-valign
So again I think the patch is actually fixing a bug.

(If I save that testcase as a local HTML file and open it in 1.7rc2 or
2004032015, then the right cell is middle aligned.)

Comment 36

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

(In reply to comment #25)
> Bernd says that
> http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/28811.html
> has a big difference in the reflow tree, visually the only thing I can see is
> that the last input is a bit more to the right than before the patch.

They look identical to me.

Comment 37

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

I have to go to bed. Someone needs to systematically look at these testcases,
identify the ones with visible differences, and determine whether the new
rendering is correct. If I have time tomorrow I'll set up my visual regression
tester to do the first two steps.

Comment 38

[Bernd]

Attachment: rtest log

Comment 39

[Bernd]

Attachment: the really interesting cases

Comment 40

[Bernd]

Comment on attachment 149589 [details] [diff] [review]
fix

I manually went true all the regression test failures. There are three classes
of failures:
1.) the frame state bits on text node change, this is responsible for the
majority of testcase failures. but doesnt seem critical to me as those are
internal bits. The changes are in the NS_FRAME_IMPL_RESERVED area.
2.) the second type are changes where table frames from not having overflow
children to be marked as having them. These are the majority of cases shown in 
the reduced list. But all those testcase look OK to me.
3.) Bbox errors
This are:
table/bugs/bug106966.html
table/bugs/bug44505.html
table/printing/bug123983.html
table/printing/bug165772-1.html
formctls/bugs/bug96506.html
I could not visually verify them.

So I r+ this, as I think this is correct and should work. However the large
number of changes in the regression output makes me  a little bit nerveous with
respect to a fast checkin into branch. I would not state that this is safe for
branch. This should bake a little bit on trunk before it goes into branch. We
might miss the 1.7 boat, but the main customer for this patch can modify its
source also later.

Comment 41

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

Bernd, did you use the full patch (changes to RowGroupFrame as well as
RowFrame)? Or just RowFrame?

Comment 42

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

Bernd's asleep, but since he r+ed the full patch, I'm going to assume he meant
the whole patch, so I'll check that in. If I'm wrong we can always back out the
RowGroupFrame chunk.

Comment 43

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

checked into trunk.

Comment 44

[Bernd]

Robert I did test the whole patch, I also did a test where I placed the the
mComputeMEW in the table cell reflow as I proposed on IRC, it has the same
number of rtest failures.

Comment 45

[Doron Rosenberg (IBM)]

since I'm home sick, I can't access my builds and all.  

I think that it would be ok to make this a 1.7.1 change to ensure sufficient
trunk baking.

mkaply, you ok with that?

Comment 46

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #40)
> 1.) the frame state bits on text node change, this is responsible for the
> majority of testcase failures. but doesnt seem critical to me as those are
> internal bits. The changes are in the NS_FRAME_IMPL_RESERVED area.

Should we fix nsTextFrame::GetDebugStateBits() to filter out those bits if they
don't actually indicate rtest issues?

Comment 47

[Mike Kaply [:mkaply]]

I got approval from drivers for 1.7. Cross your fingers.

Comment 48

[Doron Rosenberg (IBM)]

Crossing em!  Shouldn't this be marked fixed?

Comment 49

[José Jeria]

According to bug 246999, comment 8, this caused that regression.

Comment 50

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

this is fixed.

Comment 51

[Boris Zbarsky [:bzbarsky]]

It looks like this patch caused bug 262657