Bug 69296

Summary: REGRESSION (r95852?): Disappearing Border on bugs.webkit.org attachments <table>
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt, jchaffraix, joepeck, scheglov, webkit.review.bot
Priority: P1 Keywords: Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[IMAGE] Example Issue - Missing Border
none
Test case
none
Remember BorderValue in CollapsedBorderValue by value.
none
Remember BorderValue in CollapsedBorderValue by value. Includes also test. none

Description Joseph Pecoraro 2011-10-03 15:03:56 PDT
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.
Comment 1 Joseph Pecoraro 2011-10-03 15:04:25 PDT
Created attachment 109536 [details]
[IMAGE] Example Issue - Missing Border
Comment 2 Joseph Pecoraro 2011-10-03 15:15:15 PDT
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?
Comment 3 Dave Hyatt 2011-10-03 22:58:56 PDT
Sounds likely yes.
Comment 4 Julien Chaffraix 2011-10-04 10:29:17 PDT
I agree, r95852 could totally have such side effect. CC'ing Konstantin.
Comment 5 Konstantin Shcheglov 2011-10-04 10:53:51 PDT
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.
Comment 6 Konstantin Shcheglov 2011-10-04 13:53:54 PDT
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
Comment 7 Konstantin Shcheglov 2011-10-05 08:16:21 PDT
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.
Comment 8 Konstantin Shcheglov 2011-10-05 08:40:08 PDT
Created attachment 109802 [details]
Remember BorderValue in CollapsedBorderValue by value.
Comment 9 Darin Adler 2011-10-05 09:34:09 PDT
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.
Comment 10 Konstantin Shcheglov 2011-10-05 09:46:56 PDT
(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.
Comment 11 Konstantin Shcheglov 2011-10-05 11:41:36 PDT
Created attachment 109829 [details]
Remember BorderValue in CollapsedBorderValue by value. Includes also test.
Comment 12 Dave Hyatt 2011-10-06 08:53:45 PDT
Comment on attachment 109829 [details]
Remember BorderValue in CollapsedBorderValue by value. Includes also test.

r=me
Comment 13 WebKit Review Bot 2011-10-06 09:57:31 PDT
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>
Comment 14 WebKit Review Bot 2011-10-06 09:57:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Joseph Pecoraro 2011-10-06 10:13:17 PDT
Thanks for the quick fix!