Bug 93903 - Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
Summary: Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
: 94126 94403 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-08-13 14:54 PDT by Julien Chaffraix
Modified: 2012-08-24 09:39 PDT (History)
7 users (show)

See Also:


Attachments
Reproduction - beware it will crash your browser (340 bytes, text/html)
2012-08-13 14:55 PDT, Julien Chaffraix
no flags Details
Proposed fix: Added parent NULL-checks to detect detach tree conditions. (4.55 KB, patch)
2012-08-13 15:24 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (757.83 KB, application/zip)
2012-08-13 19:26 PDT, WebKit Review Bot
no flags Details
Proposed fix 2. Not super happy about the fix but it's the safest change. (6.05 KB, patch)
2012-08-15 09:59 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Better work-around: use style information. (7.30 KB, patch)
2012-08-23 09:56 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-08-13 14:54:57 PDT
https://trac.webkit.org/changeset/124168 changed the code scrollbar creation code to run out of styleDidChange instead of layout.

This means that we can now be potentially detached (the first time styleDidChange is called) when we create our scrollbars, which the old code didn't expect.

This is causing crashes in some renderer assuming that they are inserted in the tree when we request layout information (like RenderTableCell::borderLeft).
Comment 1 Julien Chaffraix 2012-08-13 14:55:36 PDT
Created attachment 158114 [details]
Reproduction - beware it will crash your browser
Comment 2 Julien Chaffraix 2012-08-13 15:24:46 PDT
Created attachment 158124 [details]
Proposed fix: Added parent NULL-checks to detect detach tree conditions.
Comment 3 Tony Chang 2012-08-13 18:22:37 PDT
Comment on attachment 158124 [details]
Proposed fix: Added parent NULL-checks to detect detach tree conditions.

View in context: https://bugs.webkit.org/attachment.cgi?id=158124&action=review

> Source/WebCore/ChangeLog:9
> +        r124168 moved scrollbar creation from layout to styleDidChange. Because the first styleDidChange
> +        is called before we insert the new renderer into the tree, the code has to be made aware of detached

Do we get a second styleDidChange after the new renderer is attached?
Comment 4 WebKit Review Bot 2012-08-13 19:26:35 PDT
Comment on attachment 158124 [details]
Proposed fix: Added parent NULL-checks to detect detach tree conditions.

Attachment 158124 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13491393

New failing tests:
scrollbars/scrollbar-orientation.html
scrollbars/scrollbar-buttons.html
compositing/overflow/clip-content-under-overflow-controls.html
scrollbars/overflow-scrollbar-combinations.html
fast/events/touch/gesture/touch-gesture-scroll-sideways.html
Comment 5 WebKit Review Bot 2012-08-13 19:26:39 PDT
Created attachment 158193 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 6 Tony Chang 2012-08-13 20:38:16 PDT
Comment on attachment 158124 [details]
Proposed fix: Added parent NULL-checks to detect detach tree conditions.

Hmm, the cq failures seem to suggest that we don't get a second styleDidChange in all cases.
Comment 7 Julien Chaffraix 2012-08-14 13:21:00 PDT
(In reply to comment #6)
> (From update of attachment 158124 [details])
> Hmm, the cq failures seem to suggest that we don't get a second styleDidChange in all cases.

On some of the tests, there is a style recalc when closing the document which is done after dumping the pixels (pretty weird). As you have pointed out, the change is wrong as it assumes that a second styleDidChange will occur. Let me dig deeper on that.
Comment 8 Julien Chaffraix 2012-08-15 09:59:56 PDT
Created attachment 158584 [details]
Proposed fix 2. Not super happy about the fix but it's the safest change.
Comment 9 Julien Chaffraix 2012-08-15 11:13:24 PDT
*** Bug 94126 has been marked as a duplicate of this bug. ***
Comment 10 Ojan Vafai 2012-08-15 11:26:05 PDT
Comment on attachment 158584 [details]
Proposed fix 2. Not super happy about the fix but it's the safest change.

Couldn't the same bug happen in a table cell that is in a detached table row?
Comment 11 Julien Chaffraix 2012-08-15 11:40:12 PDT
(In reply to comment #10)
> (From update of attachment 158584 [details])
> Couldn't the same bug happen in a table cell that is in a detached table row?

That cannot happen as we ignore overflow: scroll / auto on table row and table section (see StyleResolver::adjustRenderStyle)
Comment 12 Tony Chang 2012-08-15 17:40:44 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 158584 [details] [details])
> > Couldn't the same bug happen in a table cell that is in a detached table row?
> 
> That cannot happen as we ignore overflow: scroll / auto on table row and table section (see StyleResolver::adjustRenderStyle)

I think Ojan means we have a detached row + cell (parent() is not null, but parent()->parent() is null).
Comment 13 Ojan Vafai 2012-08-15 18:17:43 PDT
(In reply to comment #12)
> I think Ojan means we have a detached row + cell (parent() is not null, but parent()->parent() is null).

Yes, that's what I meant.
Comment 14 Julien Chaffraix 2012-08-16 16:38:39 PDT
Comment on attachment 158584 [details]
Proposed fix 2. Not super happy about the fix but it's the safest change.

I will need to investigate and test the row case, clearing the review flag in the meantime.
Comment 15 Julien Chaffraix 2012-08-17 18:04:04 PDT
*** Bug 94403 has been marked as a duplicate of this bug. ***
Comment 16 Julien Chaffraix 2012-08-23 09:56:51 PDT
Created attachment 160189 [details]
Better work-around: use style information.
Comment 17 Tony Chang 2012-08-23 10:36:58 PDT
Comment on attachment 160189 [details]
Better work-around: use style information.

View in context: https://bugs.webkit.org/attachment.cgi?id=160189&action=review

> Source/WebCore/rendering/RenderScrollbarPart.cpp:96
> +    int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->style()->borderLeftWidth() - m_scrollbar->owningRenderer()->style()->borderRightWidth();

I think you also need to check m_scrollbar->owningRenderer()->style()->border{Left,Right}().nonZero() since the width can be non-zero, but if the style is none, it'll resolve as no border.  It should be easy to make a test case with this.
Comment 18 Julien Chaffraix 2012-08-23 11:09:26 PDT
Comment on attachment 160189 [details]
Better work-around: use style information.

View in context: https://bugs.webkit.org/attachment.cgi?id=160189&action=review

>> Source/WebCore/rendering/RenderScrollbarPart.cpp:96
>> +    int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->style()->borderLeftWidth() - m_scrollbar->owningRenderer()->style()->borderRightWidth();
> 
> I think you also need to check m_scrollbar->owningRenderer()->style()->border{Left,Right}().nonZero() since the width can be non-zero, but if the style is none, it'll resolve as no border.  It should be easy to make a test case with this.

I don't think so. First that's what RenderBoxModelObject::border* functions return. But also the code explicitly checks 'border-style' before returning the value:

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/style/BorderData.h#L63

Let me know if you want me to add a test covering that case.
Comment 19 Tony Chang 2012-08-23 11:28:11 PDT
Comment on attachment 160189 [details]
Better work-around: use style information.

Ah, you're right, BorderData does the check.
Comment 20 Tony Chang 2012-08-23 11:29:43 PDT
Oh, what about hidden borders? It looks like that will return 0 but take up space.
Comment 21 Julien Chaffraix 2012-08-23 14:44:37 PDT
(In reply to comment #20)
> Oh, what about hidden borders? It looks like that will return 0 but take up space.

I am assuming you are talking about 'border-style: hidden' borders here. Per CSS 2.1, those borders should be treated as 'none', except for table cell with collapsing borders (http://www.w3.org/TR/CSS2/box.html#border-style-properties). The new code will work properly for the first case and only one side of the second (the one having the 'hidden' border). The second case is explicitly mentioned as a case where the new code won't work properly but I am pretty sure that the old code wasn't working totally fine either due to reading layout information that are potentially dirty or zeroed during layout. See bug 94054 for example.
Comment 22 Tony Chang 2012-08-23 15:33:33 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Oh, what about hidden borders? It looks like that will return 0 but take up space.
> 
> I am assuming you are talking about 'border-style: hidden' borders here. Per CSS 2.1, those borders should be treated as 'none', except for table cell with collapsing borders (http://www.w3.org/TR/CSS2/box.html#border-style-properties). The new code will work properly for the first case and only one side of the second (the one having the 'hidden' border). The second case is explicitly mentioned as a case where the new code won't work properly but I am pretty sure that the old code wasn't working totally fine either due to reading layout information that are potentially dirty or zeroed during layout. See bug 94054 for example.

Thanks, sounds like this will work like before.  Works for me.
Comment 23 Julien Chaffraix 2012-08-24 09:30:44 PDT
Comment on attachment 160189 [details]
Better work-around: use style information.

Thanks Tony!
Comment 24 WebKit Review Bot 2012-08-24 09:39:20 PDT
Comment on attachment 160189 [details]
Better work-around: use style information.

Clearing flags on attachment: 160189

Committed r126591: <http://trac.webkit.org/changeset/126591>
Comment 25 WebKit Review Bot 2012-08-24 09:39:25 PDT
All reviewed patches have been landed.  Closing bug.