Commit 11875df0 authored by eric@webkit.org's avatar eric@webkit.org

Speculative fix

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@44617 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 8a10bb96
2009-06-03 Eric Seidel <eric@webkit.org>
Reviewed by Darin Adler.
Crash at Node::nodeIndex()
https://bugs.webkit.org/show_bug.cgi?id=26044
This crash seems to be caused by calling nodeIndex() on
and invalid Node pointer. As far as I can tell, the most likely
explanation is that the Range holding the node is actually
deleted already (thus the memory is junk). Looking at the code
Range changes m_start.container() in its destructor to tell if
the range is detached or not. If somehow m_start.container() was
cleared, the range will never be detached and the Document will end
up with pointers to deleted ranges. I don't know how to reproduce this
any reproduction I could think of would hit ASSERTS in Debug mode.
One reproduction I tried, did not crash in Release mode, even though it
did in Debug mode.
I'm making a speculative fix by always detaching the Range from the document
during ~Range. This is safer, and all we lose is an ASSERT to prevent double-detach.
Another solution would be to instead store an m_attached bool on the Range object, or
expose a Document::isRangeAttached(range) call. Either of those solutions
could affect performance, so I went with this solution for now.
I also removed the RangeBoundaryPoint default constructor
as it is not used anywhere (and is more likely to cause confusion).
Since I can't reproduce the crash, no test.
* dom/Document.cpp:
(WebCore::Document::detachRange):
* dom/Range.cpp:
(WebCore::Range::~Range):
(WebCore::Range::detach):
* dom/RangeBoundaryPoint.h:
(WebCore::RangeBoundaryPoint::RangeBoundaryPoint):
RangeBoundaryPoints always have an m_containerNode, ASSERT that.
(WebCore::RangeBoundaryPoint::set):
ASSERT(container) to match all the other functions.
All callers already ASSERT(container). Since we only
have reports if this crash from Release mode, I can only
assume that those ASSERTs would have been hit in a Debug reproduction.
2009-06-11 Shinichiro Hamaji <hamaji@chromium.org> 2009-06-11 Shinichiro Hamaji <hamaji@chromium.org>
Reviewed by Adam Barth. Reviewed by Adam Barth.
...@@ -4287,7 +4287,8 @@ void Document::attachRange(Range* range) ...@@ -4287,7 +4287,8 @@ void Document::attachRange(Range* range)
void Document::detachRange(Range* range) void Document::detachRange(Range* range)
{ {
ASSERT(m_ranges.contains(range)); // We don't ASSERT m_ranges.contains(range) to allow us to call this
// unconditionally to fix: https://bugs.webkit.org/show_bug.cgi?id=26044
m_ranges.remove(range); m_ranges.remove(range);
} }
......
...@@ -95,8 +95,8 @@ PassRefPtr<Range> Range::create(PassRefPtr<Document> ownerDocument, const Positi ...@@ -95,8 +95,8 @@ PassRefPtr<Range> Range::create(PassRefPtr<Document> ownerDocument, const Positi
Range::~Range() Range::~Range()
{ {
if (m_start.container()) // Always detach (even if we've already detached) to fix https://bugs.webkit.org/show_bug.cgi?id=26044
m_ownerDocument->detachRange(this); m_ownerDocument->detachRange(this);
#ifndef NDEBUG #ifndef NDEBUG
rangeCounter.decrement(); rangeCounter.decrement();
...@@ -379,7 +379,6 @@ Range::CompareResults Range::compareNode(Node* refNode, ExceptionCode& ec) ...@@ -379,7 +379,6 @@ Range::CompareResults Range::compareNode(Node* refNode, ExceptionCode& ec)
} }
} }
short Range::compareBoundaryPoints(CompareHow how, const Range* sourceRange, ExceptionCode& ec) const short Range::compareBoundaryPoints(CompareHow how, const Range* sourceRange, ExceptionCode& ec) const
{ {
if (!m_start.container()) { if (!m_start.container()) {
...@@ -1070,6 +1069,7 @@ PassRefPtr<DocumentFragment> Range::createContextualFragment(const String& marku ...@@ -1070,6 +1069,7 @@ PassRefPtr<DocumentFragment> Range::createContextualFragment(const String& marku
void Range::detach(ExceptionCode& ec) void Range::detach(ExceptionCode& ec)
{ {
// Check first to see if we've already detached:
if (!m_start.container()) { if (!m_start.container()) {
ec = INVALID_STATE_ERR; ec = INVALID_STATE_ERR;
return; return;
......
...@@ -33,7 +33,6 @@ namespace WebCore { ...@@ -33,7 +33,6 @@ namespace WebCore {
class RangeBoundaryPoint { class RangeBoundaryPoint {
public: public:
RangeBoundaryPoint();
explicit RangeBoundaryPoint(PassRefPtr<Node> container); explicit RangeBoundaryPoint(PassRefPtr<Node> container);
const Position toPosition() const; const Position toPosition() const;
...@@ -63,17 +62,12 @@ private: ...@@ -63,17 +62,12 @@ private:
Node* m_childBeforeBoundary; Node* m_childBeforeBoundary;
}; };
inline RangeBoundaryPoint::RangeBoundaryPoint()
: m_offsetInContainer(0)
, m_childBeforeBoundary(0)
{
}
inline RangeBoundaryPoint::RangeBoundaryPoint(PassRefPtr<Node> container) inline RangeBoundaryPoint::RangeBoundaryPoint(PassRefPtr<Node> container)
: m_containerNode(container) : m_containerNode(container)
, m_offsetInContainer(0) , m_offsetInContainer(0)
, m_childBeforeBoundary(0) , m_childBeforeBoundary(0)
{ {
ASSERT(m_containerNode);
} }
inline Node* RangeBoundaryPoint::container() const inline Node* RangeBoundaryPoint::container() const
...@@ -116,6 +110,7 @@ inline void RangeBoundaryPoint::clear() ...@@ -116,6 +110,7 @@ inline void RangeBoundaryPoint::clear()
inline void RangeBoundaryPoint::set(PassRefPtr<Node> container, int offset, Node* childBefore) inline void RangeBoundaryPoint::set(PassRefPtr<Node> container, int offset, Node* childBefore)
{ {
ASSERT(container);
ASSERT(offset >= 0); ASSERT(offset >= 0);
ASSERT(childBefore == (offset ? container->childNode(offset - 1) : 0)); ASSERT(childBefore == (offset ? container->childNode(offset - 1) : 0));
m_containerNode = container; m_containerNode = container;
......
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