WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69296
REGRESSION (
r95852
?): Disappearing Border on bugs.webkit.org attachments <table>
https://bugs.webkit.org/show_bug.cgi?id=69296
Summary
REGRESSION (r95852?): Disappearing Border on bugs.webkit.org attachments <table>
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
Details
Test case
(755 bytes, text/html)
2011-10-04 13:53 PDT
,
Konstantin Shcheglov
no flags
Details
Remember BorderValue in CollapsedBorderValue by value.
(21.32 KB, patch)
2011-10-05 08:40 PDT
,
Konstantin Shcheglov
no flags
Details
Formatted Diff
Diff
Remember BorderValue in CollapsedBorderValue by value. Includes also test.
(26.34 KB, patch)
2011-10-05 11:41 PDT
,
Konstantin Shcheglov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug