REGRESSION (r95852?): Disappearing Border on bugs.webkit.org attachments <table>

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Joseph Pecoraro]

The border on the attachments table rows is randomly appearing / disappearing
due to hover and style changes.

To reproduce wave your mouse over and outside of the <table> on
a bugzilla bug with "PATCH" attachments. For example:
<http://webkit.org/b/69152> Web Inspector: rgb() with percentages shows wrong hex/hsl values

See attached screenshots.

[Joseph Pecoraro]

Created attachment 109536 [details]
[IMAGE] Example Issue - Missing Border

[Joseph Pecoraro]

Regressed to somewhere between r95778 and r95880:
http://trac.webkit.org/log/trunk/?rev=95880&stop_rev=95778

One commit that jumps out at me is r95852. So I'm adding Julien and Hyatt:
<http://webkit.org/b/64546> Redrawing dirty parts of a large table is very slow

Does this sound like it could have caused this regression?

[Dave Hyatt]

Sounds likely yes.

[Julien Chaffraix]

I agree, r95852 could totally have such side effect. CC'ing Konstantin.

[Konstantin Shcheglov]

I can reproduce this with collapsing border caching and can not without.
So, it seems that regression was really caused by this change.
I will continue investigation.

[Konstantin Shcheglov]

Created attachment 109679 [details]
Test case

Somehow having TD DIV DIV in one cell and :hover style in this TR triggers this problem.
Initially remembered cached border is:

border=0x7fb336afeb68 style=8 nonZero=1 width=1

When it is not displayed, I see:
border=0x7fb336afeb68 style=0 nonZero=0 width=379
border=0x7fb336afeb68 style=0 nonZero=0 width=3

I see between these variants strange changes in borders, here is only leftWidth.

RenderStyle::setBorderLeftWidth border=0x7fb336afea08 3 -> 0
RenderStyle::setBorderLeftWidth border=0x7fb336afea08 0 -> 1
RenderStyle::setBorderLeftWidth border=0x7fb336afe488 3 -> 0
RenderStyle::setBorderLeftWidth border=0x7fb336afe488 0 -> 1

[Konstantin Shcheglov]

Looks like memory management problem.

RenderTable::recalcCollapsedBorders() remembers Vector of CollapsedBorderValue, which holds _pointer_ on BorderValue.

However sometimes Element::recalcStyle(StyleChange change) replaces full RenderStyle, and this causes that remembered BorderValue pointer starts pointing nowhere, and sometimes its style changed to 0, and then border is not painted.

BorderValue is remembered in RenderStyle by value, so we can not use RefPtr. So, I'm going to remember BorderValue in CollapsedBorderValue also by value.

[Konstantin Shcheglov]

Created attachment 109802 [details]
Remember BorderValue in CollapsedBorderValue by value.

[Darin Adler]

Comment on attachment 109802 [details]
Remember BorderValue in CollapsedBorderValue by value.

This change doesn’t look right. Either it fixes a bug, so it needs a test case, or it doesn’t, so it needs an explanation for the rationale of the changes other than "fixes a detail".

This makes CollapsedBorderValue objects larger, so there needs to be some benefit. From reading the comments in the bug report it seems that this *does* fix a bug and so needs a test case to show what it fixes.

[Konstantin Shcheglov]

(In reply to comment #9)
> (From update of attachment 109802 [details])
> This change doesn’t look right. Either it fixes a bug, so it needs a test case, or it doesn’t, so it needs an explanation for the rationale of the changes other than "fixes a detail".
> 
> This makes CollapsedBorderValue objects larger, so there needs to be some benefit. From reading the comments in the bug report it seems that this *does* fix a bug and so needs a test case to show what it fixes.

1. Actual count of CollapsedBorderValue objects is small even for large tables, because only unique objects are remembered.

2. I think that my comment #7 has enough description why we need to store BorderValue as value. Is it not good enough? Is having description in bug comments not enough? I don't know what is policy about ChangeLog, so may be I should also include same description into ChangeLog file?

3. As you can see, this is memory problem, which happens _sometimes_, depending on whether we are "lucky" that m_style flag filled with 0, or not. So, it would be not easy to make reproducable test. Again, I'm open to recommendations here. If you know that in cases like this it is recommended to write at least flaky test, I will do this (hopefully eventSender.mouseMoveTo() works). I just want to describe reason why it was not done, and I'd like to have confirmation.

[Konstantin Shcheglov]

Created attachment 109829 [details]
Remember BorderValue in CollapsedBorderValue by value. Includes also test.

[Dave Hyatt]

Comment on attachment 109829 [details]
Remember BorderValue in CollapsedBorderValue by value. Includes also test.

r=me

[WebKit Review Bot]

Comment on attachment 109829 [details]
Remember BorderValue in CollapsedBorderValue by value. Includes also test.

Clearing flags on attachment: 109829

Committed r96822: <http://trac.webkit.org/changeset/96822>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Thanks for the quick fix!