Diff
Modified: trunk/Source/WebCore/ChangeLog (222737 => 222738)
--- trunk/Source/WebCore/ChangeLog 2017-10-02 20:00:03 UTC (rev 222737)
+++ trunk/Source/WebCore/ChangeLog 2017-10-02 20:06:06 UTC (rev 222738)
@@ -1,3 +1,40 @@
+2017-10-02 Zalan Bujtas <[email protected]>
+
+ SelectionRangeData should not hold raw RenderObject pointers
+ https://bugs.webkit.org/show_bug.cgi?id=177677
+ <rdar://problem/34763060>
+
+ Reviewed by Sam Weinig.
+
+ SelectionRangeData::Context start and end renderers' lifetime is not strictly tied
+ to the lifetime of SelectionRangeData.
+
+ Covered by existing tests.
+
+ * editing/FrameSelection.cpp:
+ (WebCore::FrameSelection::updateAppearance):
+ * platform/DragImage.cpp:
+ (WebCore::createDragImageForRange):
+ * rendering/SelectionRangeData.cpp:
+ (WebCore::isValidRendererForSelection):
+ (WebCore::collect):
+ (WebCore::SelectionRangeData::set):
+ (WebCore::SelectionRangeData::clear):
+ (WebCore::SelectionRangeData::repaint const):
+ (WebCore::SelectionRangeData::collectBounds const):
+ (WebCore::SelectionRangeData::apply):
+ * rendering/SelectionRangeData.h:
+ (WebCore::SelectionRangeData::Context::Context):
+ (WebCore::SelectionRangeData::Context::start const):
+ (WebCore::SelectionRangeData::Context::end const):
+ (WebCore::SelectionRangeData::Context::startPosition const):
+ (WebCore::SelectionRangeData::Context::endPosition const):
+ (WebCore::SelectionRangeData::Context::operator== const):
+ (WebCore::SelectionRangeData::start const):
+ (WebCore::SelectionRangeData::end const):
+ (WebCore::SelectionRangeData::startPosition const):
+ (WebCore::SelectionRangeData::endPosition const):
+
2017-10-02 Olivier Blin <[email protected]>
[WPE] Remove GnuTLS dependency
Modified: trunk/Source/WebCore/editing/FrameSelection.cpp (222737 => 222738)
--- trunk/Source/WebCore/editing/FrameSelection.cpp 2017-10-02 20:00:03 UTC (rev 222737)
+++ trunk/Source/WebCore/editing/FrameSelection.cpp 2017-10-02 20:06:06 UTC (rev 222738)
@@ -2114,7 +2114,7 @@
RenderObject* endRenderer = endPos.deprecatedNode()->renderer();
int endOffset = endPos.deprecatedEditingOffset();
ASSERT(startOffset >= 0 && endOffset >= 0);
- view->selection().set(SelectionRangeData::Context(startRenderer, endRenderer, startOffset, endOffset));
+ view->selection().set({ startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset) });
}
}
Modified: trunk/Source/WebCore/platform/DragImage.cpp (222737 => 222738)
--- trunk/Source/WebCore/platform/DragImage.cpp 2017-10-02 20:00:03 UTC (rev 222737)
+++ trunk/Source/WebCore/platform/DragImage.cpp 2017-10-02 20:06:06 UTC (rev 222738)
@@ -185,7 +185,7 @@
int startOffset = start.deprecatedEditingOffset();
int endOffset = end.deprecatedEditingOffset();
ASSERT(startOffset >= 0 && endOffset >= 0);
- view->selection().set(SelectionRangeData::Context(startRenderer, endRenderer, startOffset, endOffset), SelectionRangeData::RepaintMode::Nothing);
+ view->selection().set({ startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset) }, SelectionRangeData::RepaintMode::Nothing);
// We capture using snapshotFrameRect() because we fake up the selection using
// FrameView but snapshotSelection() uses the selection from the Frame itself.
return createDragImageFromSnapshot(snapshotFrameRect(frame, view->selection().boundsClippedToVisibleContent(), options), nullptr);
Modified: trunk/Source/WebCore/rendering/SelectionRangeData.cpp (222737 => 222738)
--- trunk/Source/WebCore/rendering/SelectionRangeData.cpp 2017-10-02 20:00:03 UTC (rev 222737)
+++ trunk/Source/WebCore/rendering/SelectionRangeData.cpp 2017-10-02 20:06:06 UTC (rev 222738)
@@ -102,7 +102,7 @@
static bool isValidRendererForSelection(const RenderObject& renderer, const SelectionRangeData::Context& selection)
{
- return (renderer.canBeSelectionLeaf() || &renderer == selection.start || &renderer == selection.end)
+ return (renderer.canBeSelectionLeaf() || &renderer == selection.start() || &renderer == selection.end())
&& renderer.selectionState() != RenderObject::SelectionNone
&& renderer.containingBlock();
}
@@ -115,14 +115,14 @@
static SelectionData collect(const SelectionRangeData::Context& selection, bool repaintDifference)
{
- SelectionData oldSelectionData { selection.startPosition, selection.endPosition, { }, { } };
+ SelectionData oldSelectionData { selection.startPosition(), selection.endPosition(), { }, { } };
// Blocks contain selected objects and fill gaps between them, either on the left, right, or in between lines and blocks.
// In order to get the repaint rect right, we have to examine left, middle, and right rects individually, since otherwise
// the union of those rects might remain the same even when changes have occurred.
- RenderObject* start = selection.start;
+ auto* start = selection.start();
RenderObject* stop = nullptr;
- if (selection.end)
- stop = rendererAfterPosition(*selection.end, selection.endPosition.value());
+ if (selection.end())
+ stop = rendererAfterPosition(*selection.end(), selection.endPosition().value());
SelectionIterator selectionIterator(start);
while (start && start != stop) {
if (isValidRendererForSelection(*start, selection)) {
@@ -154,7 +154,7 @@
{
// Make sure both our start and end objects are defined.
// Check www.msnbc.com and try clicking around to find the case where this happened.
- if ((selection.start && !selection.end) || (selection.end && !selection.start))
+ if ((selection.start() && !selection.end()) || (selection.end() && !selection.start()))
return;
// Just return if the selection hasn't changed.
auto isCaret = m_renderView.frame().selection().isCaret();
@@ -172,7 +172,7 @@
void SelectionRangeData::clear()
{
m_renderView.layer()->repaintBlockSelectionGaps();
- set(SelectionRangeData::Context(), SelectionRangeData::RepaintMode::NewMinusOld);
+ set({ }, SelectionRangeData::RepaintMode::NewMinusOld);
}
void SelectionRangeData::repaint() const
@@ -179,11 +179,11 @@
{
HashSet<RenderBlock*> processedBlocks;
RenderObject* end = nullptr;
- if (m_selectionContext.end)
- end = rendererAfterPosition(*m_selectionContext.end, m_selectionContext.endPosition.value());
- SelectionIterator selectionIterator(m_selectionContext.start);
+ if (m_selectionContext.end())
+ end = rendererAfterPosition(*m_selectionContext.end(), m_selectionContext.endPosition().value());
+ SelectionIterator selectionIterator(m_selectionContext.start());
for (auto* renderer = selectionIterator.current(); renderer && renderer != end; renderer = selectionIterator.next()) {
- if (!renderer->canBeSelectionLeaf() && renderer != m_selectionContext.start && renderer != m_selectionContext.end)
+ if (!renderer->canBeSelectionLeaf() && renderer != m_selectionContext.start() && renderer != m_selectionContext.end())
continue;
if (renderer->selectionState() == RenderObject::SelectionNone)
continue;
@@ -200,13 +200,13 @@
IntRect SelectionRangeData::collectBounds(ClipToVisibleContent clipToVisibleContent) const
{
SelectionData::RendererMap renderers;
- RenderObject* start = m_selectionContext.start;
+ auto* start = m_selectionContext.start();
RenderObject* stop = nullptr;
- if (m_selectionContext.end)
- stop = rendererAfterPosition(*m_selectionContext.end, m_selectionContext.endPosition.value());
+ if (m_selectionContext.end())
+ stop = rendererAfterPosition(*m_selectionContext.end(), m_selectionContext.endPosition().value());
SelectionIterator selectionIterator(start);
while (start && start != stop) {
- if ((start->canBeSelectionLeaf() || start == m_selectionContext.start || start == m_selectionContext.end)
+ if ((start->canBeSelectionLeaf() || start == m_selectionContext.start() || start == m_selectionContext.end())
&& start->selectionState() != RenderObject::SelectionNone) {
// Blocks are responsible for painting line gaps and margin gaps. They must be examined as well.
renderers.set(start, std::make_unique<RenderSelectionInfo>(*start, clipToVisibleContent == ClipToVisibleContent::Yes));
@@ -243,24 +243,24 @@
for (auto* renderer : oldSelectionData.renderers.keys())
renderer->setSelectionStateIfNeeded(RenderObject::SelectionNone);
m_selectionContext = newSelection;
+ auto* selectionStart = m_selectionContext.start();
// Update the selection status of all objects between selectionStart and selectionEnd
- if (m_selectionContext.start && m_selectionContext.start == m_selectionContext.end)
- m_selectionContext.start->setSelectionStateIfNeeded(RenderObject::SelectionBoth);
+ if (selectionStart && selectionStart == m_selectionContext.end())
+ selectionStart->setSelectionStateIfNeeded(RenderObject::SelectionBoth);
else {
- if (m_selectionContext.start)
- m_selectionContext.start->setSelectionStateIfNeeded(RenderObject::SelectionStart);
- if (m_selectionContext.end)
- m_selectionContext.end->setSelectionStateIfNeeded(RenderObject::SelectionEnd);
+ if (selectionStart)
+ selectionStart->setSelectionStateIfNeeded(RenderObject::SelectionStart);
+ if (auto* end = m_selectionContext.end())
+ end->setSelectionStateIfNeeded(RenderObject::SelectionEnd);
}
- auto* selectionStart = m_selectionContext.start;
- auto* selectionDataEnd = m_selectionContext.end;
RenderObject* selectionEnd = nullptr;
+ auto* selectionDataEnd = m_selectionContext.end();
if (selectionDataEnd)
- selectionEnd = rendererAfterPosition(*selectionDataEnd, m_selectionContext.endPosition.value());
+ selectionEnd = rendererAfterPosition(*selectionDataEnd, m_selectionContext.endPosition().value());
SelectionIterator selectionIterator(selectionStart);
for (auto* currentRenderer = selectionStart; currentRenderer && currentRenderer != selectionEnd; currentRenderer = selectionIterator.next()) {
- if (currentRenderer == m_selectionContext.start || currentRenderer == m_selectionContext.end)
+ if (currentRenderer == selectionStart || currentRenderer == m_selectionContext.end())
continue;
if (!currentRenderer->canBeSelectionLeaf())
continue;
@@ -308,8 +308,8 @@
auto* newInfo = newSelectedRenderers.get(renderer);
auto* oldInfo = selectedRendererInfo.value.get();
if (!newInfo || oldInfo->rect() != newInfo->rect() || oldInfo->state() != newInfo->state()
- || (m_selectionContext.start == renderer && oldSelectionData.startPosition != m_selectionContext.startPosition)
- || (m_selectionContext.end == renderer && oldSelectionData.endPosition != m_selectionContext.endPosition)) {
+ || (m_selectionContext.start() == renderer && oldSelectionData.startPosition != m_selectionContext.startPosition())
+ || (m_selectionContext.end() == renderer && oldSelectionData.endPosition != m_selectionContext.endPosition())) {
oldInfo->repaint();
if (newInfo) {
newInfo->repaint();
Modified: trunk/Source/WebCore/rendering/SelectionRangeData.h (222737 => 222738)
--- trunk/Source/WebCore/rendering/SelectionRangeData.h 2017-10-02 20:00:03 UTC (rev 222737)
+++ trunk/Source/WebCore/rendering/SelectionRangeData.h 2017-10-02 20:06:06 UTC (rev 222738)
@@ -44,26 +44,32 @@
public:
SelectionRangeData(RenderView&);
- struct Context {
- Context()
- { }
+ class Context {
+ public:
+ Context() = default;
+ Context(RenderObject* start, RenderObject* end, unsigned startOffset, unsigned endOffset)
+ : m_start(start ? makeWeakPtr(*start) : nullptr)
+ , m_end(end ? makeWeakPtr(*end) : nullptr)
+ , m_startPosition(startOffset)
+ , m_endPosition(endOffset)
+ {
+ }
- Context(RenderObject* startRenderer, RenderObject* endRenderer, unsigned startOffset, unsigned endOffset)
- : start(startRenderer)
- , end(endRenderer)
- , startPosition(startOffset)
- , endPosition(endOffset)
- { }
+ RenderObject* start() const { return m_start.get(); }
+ RenderObject* end() const { return m_end.get(); }
+ std::optional<unsigned> startPosition() const { return m_startPosition; }
+ std::optional<unsigned> endPosition() const { return m_endPosition; }
- RenderObject* start { nullptr };
- RenderObject* end { nullptr };
- std::optional<unsigned> startPosition;
- std::optional<unsigned> endPosition;
-
bool operator==(const Context& other) const
{
- return start == other.start && end == other.end && startPosition == other.startPosition && endPosition == other.endPosition;
+ return m_start == other.m_start && m_end == other.m_end && m_startPosition == other.m_startPosition && m_endPosition == other.m_endPosition;
}
+
+ private:
+ WeakPtr<RenderObject> m_start;
+ WeakPtr<RenderObject> m_end;
+ std::optional<unsigned> m_startPosition;
+ std::optional<unsigned> m_endPosition;
};
enum class RepaintMode { NewXOROld, NewMinusOld, Nothing };
@@ -70,10 +76,10 @@
void set(const Context&, RepaintMode = RepaintMode::NewXOROld);
const Context& get() const { return m_selectionContext; }
- RenderObject* start() const { return m_selectionContext.start; }
- RenderObject* end() const { return m_selectionContext.end; }
- unsigned startPosition() const { ASSERT(m_selectionContext.startPosition); return m_selectionContext.startPosition.value(); }
- unsigned endPosition() const { ASSERT(m_selectionContext.endPosition); return m_selectionContext.endPosition.value(); }
+ RenderObject* start() const { return m_selectionContext.start(); }
+ RenderObject* end() const { return m_selectionContext.end(); }
+ unsigned startPosition() const { ASSERT(m_selectionContext.startPosition()); return m_selectionContext.startPosition().value(); }
+ unsigned endPosition() const { ASSERT(m_selectionContext.endPosition()); return m_selectionContext.endPosition().value(); }
void clear();
IntRect bounds() const { return collectBounds(ClipToVisibleContent::No); }
