Commit ab769069 authored by inferno@chromium.org's avatar inferno@chromium.org

Crash due to owning renderer not removed from custom scrollbar.

https://bugs.webkit.org/show_bug.cgi?id=80610

Reviewed by Eric Seidel.

Source/WebCore:

Test: scrollbars/scrollbar-owning-renderer-crash.html

Changed RenderScrollbar to keep pointer to owning node, instead of the
renderer. Renderer can get destroyed without informing the scrollbar, causing
crashes later. Remove code from r94107 since it is not needed anymore and saves
times when RenderBox is getting destroyed.

* page/FrameView.cpp:
(WebCore::FrameView::createScrollbar): pass renderer's node.
* page/FrameView.h:
* rendering/RenderBox.cpp:
(WebCore::RenderBox::willBeDestroyed): no longer need this. came originally from r94107.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::createScrollbar): pass renderer's node.
(WebCore::RenderLayer::destroyScrollbar): no longer need to clear owning renderer.
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::createScrollbar): pass renderer's node.
* rendering/RenderMenuList.cpp:
(WebCore::RenderMenuList::createScrollbar): pass renderer's node.
* rendering/RenderScrollbar.cpp:
(WebCore::RenderScrollbar::createCustomScrollbar): Store owner node instead of renderer.
(WebCore::RenderScrollbar::RenderScrollbar): Store owner node instead of renderer.
(WebCore::RenderScrollbar::owningRenderer): calculate owning renderer from owner node.
* rendering/RenderScrollbar.h:
(RenderScrollbar):
* rendering/RenderTextControlSingleLine.cpp:
(WebCore::RenderTextControlSingleLine::createScrollbar): pass renderer's node.

LayoutTests:

* scrollbars/scrollbar-owning-renderer-crash-expected.txt: Added.
* scrollbars/scrollbar-owning-renderer-crash.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@116476 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent b53499d5
2012-05-08 Abhishek Arya <inferno@chromium.org>
Crash due to owning renderer not removed from custom scrollbar.
https://bugs.webkit.org/show_bug.cgi?id=80610
Reviewed by Eric Seidel.
* scrollbars/scrollbar-owning-renderer-crash-expected.txt: Added.
* scrollbars/scrollbar-owning-renderer-crash.html: Added.
2012-05-08 Kangil Han <kangil.han@samsung.com>
[EFL][DRT] Implement TextInputController.
......
WebKit bug 80610 - Crash due to owning renderer not removed from custom scrollbar.
PASS. WebKit didn't crash.
<html id="html1">
<head id="head1">
<style>
::-webkit-scrollbar { height: 50000; }
</style>
<script>
if (window.layoutTestController) {
layoutTestController.dumpAsText();
layoutTestController.waitUntilDone();
}
function runTest() {
child = document.getElementById('head1');
child.parentNode.removeChild(child);
document.body.offsetTop;
child = document.getElementById('html1');
child.parentNode.removeChild(child);
document.open();
document.write("WebKit bug 80610 - Crash due to owning renderer not removed from custom scrollbar. <br />");
document.write("PASS. WebKit didn't crash.");
document.close();
}
function finish() {
if (window.eventSender)
eventSender.mouseMoveTo(100, 100);
if (window.layoutTestController)
layoutTestController.notifyDone();
}
setTimeout("if (window.eventSender) eventSender.mouseMoveTo(0, 0);", 0);
setTimeout("runTest();", 10);
setTimeout("finish();", 11);
</script>
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
</head>
</html>
\ No newline at end of file
2012-05-08 Abhishek Arya <inferno@chromium.org>
Crash due to owning renderer not removed from custom scrollbar.
https://bugs.webkit.org/show_bug.cgi?id=80610
Reviewed by Eric Seidel.
Test: scrollbars/scrollbar-owning-renderer-crash.html
Changed RenderScrollbar to keep pointer to owning node, instead of the
renderer. Renderer can get destroyed without informing the scrollbar, causing
crashes later. Remove code from r94107 since it is not needed anymore and saves
times when RenderBox is getting destroyed.
* page/FrameView.cpp:
(WebCore::FrameView::createScrollbar): pass renderer's node.
* page/FrameView.h:
* rendering/RenderBox.cpp:
(WebCore::RenderBox::willBeDestroyed): no longer need this. came originally from r94107.
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::createScrollbar): pass renderer's node.
(WebCore::RenderLayer::destroyScrollbar): no longer need to clear owning renderer.
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::createScrollbar): pass renderer's node.
* rendering/RenderMenuList.cpp:
(WebCore::RenderMenuList::createScrollbar): pass renderer's node.
* rendering/RenderScrollbar.cpp:
(WebCore::RenderScrollbar::createCustomScrollbar): Store owner node instead of renderer.
(WebCore::RenderScrollbar::RenderScrollbar): Store owner node instead of renderer.
(WebCore::RenderScrollbar::owningRenderer): calculate owning renderer from owner node.
* rendering/RenderScrollbar.h:
(RenderScrollbar):
* rendering/RenderTextControlSingleLine.cpp:
(WebCore::RenderTextControlSingleLine::createScrollbar): pass renderer's node.
2012-05-08 Jon Lee <jonlee@apple.com>
Safari warns that it needs to resend the form in an iFrame when going back
......@@ -482,12 +482,12 @@ PassRefPtr<Scrollbar> FrameView::createScrollbar(ScrollbarOrientation orientatio
// Try the <body> element first as a scrollbar source.
Element* body = doc ? doc->body() : 0;
if (body && body->renderer() && body->renderer()->style()->hasPseudoStyle(SCROLLBAR))
return RenderScrollbar::createCustomScrollbar(this, orientation, body->renderer()->enclosingBox());
return RenderScrollbar::createCustomScrollbar(this, orientation, body);
// If the <body> didn't have a custom style, then the root element might.
Element* docElement = doc ? doc->documentElement() : 0;
if (docElement && docElement->renderer() && docElement->renderer()->style()->hasPseudoStyle(SCROLLBAR))
return RenderScrollbar::createCustomScrollbar(this, orientation, docElement->renderBox());
return RenderScrollbar::createCustomScrollbar(this, orientation, docElement);
// If we have an owning iframe/frame element, then it can set the custom scrollbar also.
RenderPart* frameRenderer = m_frame->ownerRenderer();
......@@ -2913,23 +2913,6 @@ bool FrameView::hasCustomScrollbars() const
return false;
}
void FrameView::clearOwningRendererForCustomScrollbars(RenderBox* box)
{
const HashSet<RefPtr<Widget> >* viewChildren = children();
HashSet<RefPtr<Widget> >::const_iterator end = viewChildren->end();
for (HashSet<RefPtr<Widget> >::const_iterator current = viewChildren->begin(); current != end; ++current) {
Widget* widget = current->get();
if (widget->isScrollbar()) {
Scrollbar* scrollbar = static_cast<Scrollbar*>(widget);
if (scrollbar->isCustomScrollbar()) {
RenderScrollbar* customScrollbar = toRenderScrollbar(scrollbar);
if (customScrollbar->owningRenderer() == box)
customScrollbar->clearOwningRenderer();
}
}
}
}
FrameView* FrameView::parentFrameView() const
{
if (Widget* parentView = parent()) {
......
......@@ -318,8 +318,6 @@ public:
void setAnimatorsAreActive();
RenderBox* embeddedContentBox() const;
void clearOwningRendererForCustomScrollbars(RenderBox*);
void setTracksRepaints(bool);
bool isTrackingRepaints() const { return m_isTrackingRepaints; }
......
......@@ -256,11 +256,6 @@ void RenderBox::willBeDestroyed()
if (styleToUse) {
if (RenderView* view = this->view()) {
if (FrameView* frameView = view->frameView()) {
// If this renderer is owning renderer for the FrameView's custom scrollbars,
// we need to clear it from the scrollbar. See webkit bug 64737.
if (styleToUse->hasPseudoStyle(SCROLLBAR))
frameView->clearOwningRendererForCustomScrollbars(this);
if (styleToUse->position() == FixedPosition)
frameView->removeFixedObject();
}
......
......@@ -2194,7 +2194,7 @@ PassRefPtr<Scrollbar> RenderLayer::createScrollbar(ScrollbarOrientation orientat
RenderObject* actualRenderer = renderer()->node() ? renderer()->node()->shadowAncestorNode()->renderer() : renderer();
bool hasCustomScrollbarStyle = actualRenderer->isBox() && actualRenderer->style()->hasPseudoStyle(SCROLLBAR);
if (hasCustomScrollbarStyle)
widget = RenderScrollbar::createCustomScrollbar(this, orientation, toRenderBox(actualRenderer));
widget = RenderScrollbar::createCustomScrollbar(this, orientation, actualRenderer->node());
else {
widget = Scrollbar::createNativeScrollbar(this, orientation, RegularScrollbar);
if (orientation == HorizontalScrollbar)
......@@ -2210,14 +2210,10 @@ void RenderLayer::destroyScrollbar(ScrollbarOrientation orientation)
{
RefPtr<Scrollbar>& scrollbar = orientation == HorizontalScrollbar ? m_hBar : m_vBar;
if (scrollbar) {
if (scrollbar->isCustomScrollbar())
toRenderScrollbar(scrollbar.get())->clearOwningRenderer();
else {
if (orientation == HorizontalScrollbar)
willRemoveHorizontalScrollbar(scrollbar.get());
else
willRemoveVerticalScrollbar(scrollbar.get());
}
if (orientation == HorizontalScrollbar)
willRemoveHorizontalScrollbar(scrollbar.get());
else
willRemoveVerticalScrollbar(scrollbar.get());
scrollbar->removeFromParent();
scrollbar->disconnectFromScrollableArea();
......
......@@ -831,7 +831,7 @@ PassRefPtr<Scrollbar> RenderListBox::createScrollbar()
RefPtr<Scrollbar> widget;
bool hasCustomScrollbarStyle = style()->hasPseudoStyle(SCROLLBAR);
if (hasCustomScrollbarStyle)
widget = RenderScrollbar::createCustomScrollbar(this, VerticalScrollbar, this);
widget = RenderScrollbar::createCustomScrollbar(this, VerticalScrollbar, this->node());
else {
widget = Scrollbar::createNativeScrollbar(this, VerticalScrollbar, theme()->scrollbarControlSizeForPart(ListboxPart));
didAddVerticalScrollbar(widget.get());
......
......@@ -495,7 +495,7 @@ PassRefPtr<Scrollbar> RenderMenuList::createScrollbar(ScrollableArea* scrollable
RefPtr<Scrollbar> widget;
bool hasCustomScrollbarStyle = style()->hasPseudoStyle(SCROLLBAR);
if (hasCustomScrollbarStyle)
widget = RenderScrollbar::createCustomScrollbar(scrollableArea, orientation, this);
widget = RenderScrollbar::createCustomScrollbar(scrollableArea, orientation, this->node());
else
widget = Scrollbar::createNativeScrollbar(scrollableArea, orientation, controlSize);
return widget.release();
......
......@@ -34,16 +34,18 @@
namespace WebCore {
PassRefPtr<Scrollbar> RenderScrollbar::createCustomScrollbar(ScrollableArea* scrollableArea, ScrollbarOrientation orientation, RenderBox* renderer, Frame* owningFrame)
PassRefPtr<Scrollbar> RenderScrollbar::createCustomScrollbar(ScrollableArea* scrollableArea, ScrollbarOrientation orientation, Node* ownerNode, Frame* owningFrame)
{
return adoptRef(new RenderScrollbar(scrollableArea, orientation, renderer, owningFrame));
return adoptRef(new RenderScrollbar(scrollableArea, orientation, ownerNode, owningFrame));
}
RenderScrollbar::RenderScrollbar(ScrollableArea* scrollableArea, ScrollbarOrientation orientation, RenderBox* renderer, Frame* owningFrame)
RenderScrollbar::RenderScrollbar(ScrollableArea* scrollableArea, ScrollbarOrientation orientation, Node* ownerNode, Frame* owningFrame)
: Scrollbar(scrollableArea, orientation, RegularScrollbar, RenderScrollbarTheme::renderScrollbarTheme())
, m_owner(renderer)
, m_owner(ownerNode)
, m_owningFrame(owningFrame)
{
ASSERT(ownerNode || owningFrame);
// FIXME: We need to do this because RenderScrollbar::styleChanged is called as soon as the scrollbar is created.
// Update the scrollbar size.
......@@ -81,7 +83,7 @@ RenderBox* RenderScrollbar::owningRenderer() const
RenderBox* currentRenderer = m_owningFrame->ownerRenderer();
return currentRenderer;
}
return m_owner;
return m_owner && m_owner->renderer() ? m_owner->renderer()->enclosingBox() : 0;
}
void RenderScrollbar::setParent(ScrollView* parent)
......
......@@ -26,6 +26,7 @@
#ifndef RenderScrollbar_h
#define RenderScrollbar_h
#include "Node.h"
#include "RenderStyleConstants.h"
#include "Scrollbar.h"
#include <wtf/HashMap.h>
......@@ -39,18 +40,17 @@ class RenderStyle;
class RenderScrollbar : public Scrollbar {
protected:
RenderScrollbar(ScrollableArea*, ScrollbarOrientation, RenderBox*, Frame*);
RenderScrollbar(ScrollableArea*, ScrollbarOrientation, Node*, Frame*);
public:
friend class Scrollbar;
static PassRefPtr<Scrollbar> createCustomScrollbar(ScrollableArea*, ScrollbarOrientation, RenderBox*, Frame* owningFrame = 0);
static PassRefPtr<Scrollbar> createCustomScrollbar(ScrollableArea*, ScrollbarOrientation, Node*, Frame* owningFrame = 0);
virtual ~RenderScrollbar();
static ScrollbarPart partForStyleResolve();
static RenderScrollbar* scrollbarForStyleResolve();
RenderBox* owningRenderer() const;
void clearOwningRenderer() { m_owner = 0; }
void paintPart(GraphicsContext*, ScrollbarPart, const IntRect&);
......@@ -80,7 +80,12 @@ private:
PassRefPtr<RenderStyle> getScrollbarPseudoStyle(ScrollbarPart, PseudoId);
void updateScrollbarPart(ScrollbarPart, bool destroy = false);
RenderBox* m_owner;
// This Scrollbar(Widget) may outlive the DOM which created it (during tear down),
// so we keep a reference to the Node which caused this custom scrollbar creation.
// This will not create a reference cycle as the Widget tree is owned by our containing
// FrameView which this Node pointer can in no way keep alive. See webkit bug 80610.
RefPtr<Node> m_owner;
Frame* m_owningFrame;
HashMap<unsigned, RenderScrollbarPart*> m_parts;
};
......
......@@ -750,7 +750,7 @@ PassRefPtr<Scrollbar> RenderTextControlSingleLine::createScrollbar(ScrollableAre
RefPtr<Scrollbar> widget;
bool hasCustomScrollbarStyle = style()->hasPseudoStyle(SCROLLBAR);
if (hasCustomScrollbarStyle)
widget = RenderScrollbar::createCustomScrollbar(scrollableArea, orientation, this);
widget = RenderScrollbar::createCustomScrollbar(scrollableArea, orientation, this->node());
else
widget = Scrollbar::createNativeScrollbar(scrollableArea, orientation, controlSize);
return widget.release();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment