Bug 206314

Created attachment 387845 [details] Patch

Comment on attachment 387845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387845&action=review > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:85 > + auto visibleSelection = VisibleSelection(Range::create(*rangeGroup->m_document.get(), data->range)); It makes no sense to create a Range just so that we can create VisibleSelection. r- because of this. Once again, Range is a very expensive object to create so we should avoid making one. Just add a new constructor of VisibleSelection which takes StaticRange instead. Also, what guarantees that boundary points of StaticRange still point to nodes in the same document at this point? > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:49 > + HighlightRangeData(StaticRange&& range) Nit: Please define this before all data members and place a blank line between member functions & member variables to visually separate them. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:71 > + Nit: whitespace. > Source/WebCore/editing/TextManipulationController.cpp:-209 > - This seems like an unrelated change. Please revert. > Source/WebCore/rendering/InlineTextBox.cpp:1055 > + Ditto. Revert. > Source/WebCore/rendering/InlineTextBox.cpp:1060 > + highlightData.setContext({startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)}); Ditto. > Source/WebCore/rendering/SelectionRangeData.cpp:170 > - if (&renderer == m_selectionContext.start() || renderer.isDescendantOf(m_selectionContext.start())) { > + if (&renderer == m_selectionContext.start() /*|| renderer.isDescendantOf(m_selectionContext.start())*/) { What's happening with this? We don't commit commented out code like this. > Source/WebCore/rendering/SelectionRangeData.cpp:176 > - if (&renderer == m_selectionContext.end() || renderer.isDescendantOf(m_selectionContext.end())) > + if (&renderer == m_selectionContext.end() /*|| renderer.isDescendantOf(m_selectionContext.end())*/) Ditto.

Created attachment 387873 [details] Patch

Created attachment 387882 [details] Patch

Comment on attachment 387882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387882&action=review Okay, the new patch looks better but there is one security bug in this patch so we need to fix that before I can r+ it. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:80 > + Nit: whitespace. > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:84 > + if (!data || !areNodesConnectedInSameTreeScope(data->range->startContainer(), data->range->endContainer())) > + return; Hm... areNodesConnectedInSameTreeScope returns true when both nodes are disconnected. We want to check that. So we need to exit early when !data->range->startContainer().isConnected(). In fact, it might be simpler to just check that &data->range->startContainer().treeScope() == &data->range->endContainer().treeScope() > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:86 > + auto visibleSelection = VisibleSelection(data->range); Why not just: VisibleSelection visibleSelection(data->range); > Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:88 > + data->startPosition = visibleSelection.visibleStart().deepEquivalent(); > + data->endPosition = visibleSelection.visibleEnd().deepEquivalent(); Let's not safe to access data here. Construction of VisibleSelection and calls to visibleStart or visibleEnd can easy trigger a sync layout, execute arbitrary JS and delete this highlight group. You should either check nullity of weakData after each call, or make a copy of HighightRangeData at the beginning. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:47 > + static Ref<HighlightRangeData> create(Ref<StaticRange>&& range) Nit: Need a blank line between the constructor & this static member function. > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:71 > + Vector<Ref<HighlightRangeData>> m_rangesData; // TODO: use a HashSet instead of a Vector <rdar://problem/57760614> Nit: Use FIXME instead. > Source/WebCore/dom/Range.cpp:48 > +#include "StaticRange.h" Why do we need to include StaticRange.h here? > Source/WebCore/dom/Range.h:47 > +class StaticRange; Ditto. > Source/WebCore/editing/VisibleSelection.cpp:106 > + validate(); We should assert that the caller has checked both start & end belong to the same document here. i.e. check that &staticRange.startContainer()->treeScope() == &staticRange.endContainer()->treeScope().

Created attachment 387888 [details] Patch

Looks like CSS highlight tests are failing?

Created attachment 388005 [details] Patch

Comment on attachment 388005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388005&action=review > Source/WebCore/dom/Document.cpp:2747 > + for (auto& highlight : m_highlightMap->map()) { > + for (auto& rangeData : highlight.value->rangesData()) { These iterations aren't safe because you're constructing VisibleSelection inside. Constructing VisibleSelection can result in layout to be updated and execute arbitrary JS code. We probably have to make a copy of highlight groups to a Vector instead. For each highlight group, we also need to make a Vector of WeakPtr to range data or something. > Source/WebCore/dom/Document.cpp:2748 > + if ((rangeData->startPosition.isNotNull() && rangeData->endPosition.isNotNull()) || (&rangeData->range->startContainer()->treeScope() != &rangeData->range->endContainer()->treeScope())) Hm... instead of using positions being null as a way of detecting positions not being set, can we use Optional like Optional<pair<Position, Position>> or add a dedicated boolean flag?

Created attachment 388101 [details] Patch

Comment on attachment 388101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388101&action=review > Source/WebCore/dom/Document.cpp:2751 > + if ((!rangeData->startPosition.hasValue() || !rangeData->endPosition.hasValue()) > + && (&rangeData->range->startContainer()->treeScope() == &rangeData->range->endContainer()->treeScope())) > + rangesData.append(makeWeakPtr(rangeData.ptr())); This is a complicated expression. Can we continue early instead? i.e. if (!rangeData->startPosition || !rangeData->endPosition) continue; if (rangeData->range->startContainer()->treeScope() != &rangeData->range->endContainer()->treeScope()) continue; rangesData.append(makeWeakPtr(rangeData.ptr())); > Source/WebCore/dom/Document.cpp:2765 > + if (weakRangeData.get()) { Continue when !weakRangeData instead? > Source/WebCore/dom/Document.cpp:2766 > + if (!rangeData->startPosition.hasValue()) I don't think we need to check this since it's unlikely that Page::updateRendering would have ran by updating the layout. Even if did, there is no harm in re-setting to the new position value. > Source/WebCore/dom/Document.cpp:2767 > + rangeData->startPosition = visibleSelection.visibleStart().deepEquivalent(); Need to do this instead: rangeData->startPosition = startPosition > Source/WebCore/dom/Document.cpp:2768 > + if (!rangeData->endPosition.hasValue()) Ditto. > Source/WebCore/dom/Document.cpp:2769 > + rangeData->endPosition = visibleSelection.visibleEnd().deepEquivalent(); Ditto. > Source/WebCore/rendering/InlineTextBox.cpp:1047 > + if (rangeData->startPosition.hasValue() && rangeData->endPosition.hasValue()) { Nit: It's more succinct to say: rangeData->startPosition && rangeData->endPosition.

Comment on attachment 388101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388101&action=review > Source/WebCore/Modules/highlight/HighlightRangeGroup.h:38 > +class Position; Nit - you don't need the forward declaration here if you also #include "Position.h"

Created attachment 388121 [details] Patch for landing

Comment on attachment 388121 [details] Patch for landing Clearing flags on attachment: 388121 Committed r254785: <https://trac.webkit.org/changeset/254785>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58703201>

*** Bug 205529 has been marked as a duplicate of this bug. ***