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

Joseph Pecoraro
Reported 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.
Attachments
[IMAGE] Example Issue - Missing Border (59.26 KB, image/png)
2011-10-03 15:04 PDT, Joseph Pecoraro
no flags
Test case (755 bytes, text/html)
2011-10-04 13:53 PDT, Konstantin Shcheglov
no flags
Remember BorderValue in CollapsedBorderValue by value. (21.32 KB, patch)
2011-10-05 08:40 PDT, Konstantin Shcheglov
no flags
Remember BorderValue in CollapsedBorderValue by value. Includes also test. (26.34 KB, patch)
2011-10-05 11:41 PDT, Konstantin Shcheglov
no flags
Joseph Pecoraro
Comment 1 2011-10-03 15:04:25 PDT
Created attachment 109536 [details] [IMAGE] Example Issue - Missing Border
Joseph Pecoraro
Comment 2 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?
Dave Hyatt
Comment 3 2011-10-03 22:58:56 PDT
Sounds likely yes.
Julien Chaffraix
Comment 4 2011-10-04 10:29:17 PDT
I agree, r95852 could totally have such side effect. CC'ing Konstantin.
Konstantin Shcheglov
Comment 5 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.
Konstantin Shcheglov
Comment 6 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
Konstantin Shcheglov
Comment 7 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.
Konstantin Shcheglov
Comment 8 2011-10-05 08:40:08 PDT
Created attachment 109802 [details] Remember BorderValue in CollapsedBorderValue by value.
Darin Adler
Comment 9 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.
Konstantin Shcheglov
Comment 10 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.
Konstantin Shcheglov
Comment 11 2011-10-05 11:41:36 PDT
Created attachment 109829 [details] Remember BorderValue in CollapsedBorderValue by value. Includes also test.
Dave Hyatt
Comment 12 2011-10-06 08:53:45 PDT
Comment on attachment 109829 [details] Remember BorderValue in CollapsedBorderValue by value. Includes also test. r=me
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2011-10-06 09:57:36 PDT
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 15 2011-10-06 10:13:17 PDT
Thanks for the quick fix!
Note You need to log in before you can comment on or make changes to this bug.