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.
Created attachment 109536 [details] [IMAGE] Example Issue - Missing Border
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?
Sounds likely yes.
I agree, r95852 could totally have such side effect. CC'ing Konstantin.
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.
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
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.
Created attachment 109802 [details] Remember BorderValue in CollapsedBorderValue by value.
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.
(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.
Created attachment 109829 [details] Remember BorderValue in CollapsedBorderValue by value. Includes also test.
Comment on attachment 109829 [details] Remember BorderValue in CollapsedBorderValue by value. Includes also test. r=me
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>
All reviewed patches have been landed. Closing bug.
Thanks for the quick fix!