Bug 67669 – Null ptr crash in RenderScrollbar::updateScrollbarParts

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Abhishek Arya]

http://code.google.com/p/chromium/issues/detail?id=95552

Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000018 )

0x584150d3	 [chrome.dll	 - renderobject.h:960]	WebCore::RenderObject::setChildNeedsLayout(bool,bool)
0x5843873f	 [chrome.dll	 - renderscrollbar.cpp:198]	WebCore::RenderScrollbar::updateScrollbarParts(bool)
0x58438470	 [chrome.dll	 - renderscrollbar.cpp:93]	WebCore::RenderScrollbar::setEnabled(bool)
0x58579e8a	 [chrome.dll	 - scrollview.cpp:554]	WebCore::ScrollView::updateScrollbars(WebCore::IntSize const &)
0x58579633	 [chrome.dll	 - scrollview.cpp:296]	WebCore::ScrollView::setContentsSize(WebCore::IntSize const &)
0x584a4ea9	 [chrome.dll	 - frameview.cpp:489]	WebCore::FrameView::setContentsSize(WebCore::IntSize const &)
0x584a4f77	 [chrome.dll	 - frameview.cpp:515]	WebCore::FrameView::adjustViewSize()
0x584a5899	 [chrome.dll	 - frameview.cpp:1029]	WebCore::FrameView::layout(bool)
0x5861f718	 [chrome.dll	 - document.cpp:1614]	WebCore::Document::updateLayout()
0x5861f78c	 [chrome.dll	 - document.cpp:1645]	WebCore::Document::updateLayoutIgnorePendingStylesheets()
0x58627013	 [chrome.dll	 - element.cpp:402]	WebCore::Element::offsetParent()
0x586c00a3	 [chrome.dll	 - v8element.cpp:100]	WebCore::ElementInternal::offsetParentAttrGetter

Basically, owning renderer can be cleared in ClearOwningRenderer. we need a null check here.  unfortunately top crasher, but no repro.

[Abhishek Arya]

Created attachment 106481 [details]
Patch

[James Robinson]

From that stack, it looks like this scrollbar is associated directly with a ScrollView instead of a scrollable RenderObject, so I'm kind of surprised that this null check fixes it since clearOwningRenderer() doesn't touch the m_owningFrame ptr, it only touches the m_owner pointer.

Can you take a look at the crashing URLs and see if they involve custom styled scrollbars?

[Abhishek Arya]

from scrollview.h
    // Overridden by FrameView to create custom CSS scrollbars if applicable.
    virtual PassRefPtr<Scrollbar> createScrollbar(ScrollbarOrientation);

(In reply to comment #2)
> From that stack, it looks like this scrollbar is associated directly with a ScrollView instead of a scrollable RenderObject, so I'm kind of surprised that this null check fixes it since clearOwningRenderer() doesn't touch the m_owningFrame ptr, it only touches the m_owner pointer.
> 
> Can you take a look at the crashing URLs and see if they involve custom styled scrollbars?

The crashing url is not important, but the chrome extension is :) - https://chrome.google.com/webstore/detail/ojmmnceaidnmminjjffpndcbdibelgam [http://crash/reportdetail?reportid=0278250f44f77565]. Yeah, it creates nice custom scrollbars.

[James Robinson]

Comment on attachment 106481 [details]
Patch

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

So I think this is at least not harmful, so R=me.  I really don't understand how this could help with the crash that you cite, however.

> Source/WebCore/ChangeLog:7
> +
> +        Reviewed by NOBODY (OOPS!).
> +

describe the change and why you haven't added tests

> Source/WebCore/rendering/RenderScrollbar.cpp:199
> +        if (owningRenderer())
> +            owningRenderer()->setChildNeedsLayout(true);

this should be written more like
if (RenderBox* box = owningRenderer())
    box->setChildNeedsLayout(true)

this is more common in webkit and more efficient in the case where owningRenderer() is expensive

[Abhishek Arya]

Created attachment 106603 [details]
Patch

[James Robinson]

Comment on attachment 106603 [details]
Patch

OK. keep an eye on the crash reports, please

[Abhishek Arya]

(In reply to comment #6)
> (From update of attachment 106603 [details])
> OK. keep an eye on the crash reports, please

Sure I will. Thanks for the review.

[Abhishek Arya]

Committed r94690: <http://trac.webkit.org/changeset/94690>

[Abhishek Arya]

This top crasher is now gone in 14.0.835.159.