Bug 206314 – Use Visible Position to calculate Positions for highlights

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Use Visible Position to calculate Positions for highlights

[Megan Gardner]

Created attachment 387845 [details]
Patch

[Ryosuke Niwa]

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.

[Megan Gardner]

Created attachment 387873 [details]
Patch

[Megan Gardner]

Created attachment 387882 [details]
Patch

[Ryosuke Niwa]

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().

[Megan Gardner]

Created attachment 387888 [details]
Patch

[Ryosuke Niwa]

Looks like CSS highlight tests are failing?

[Megan Gardner]

Created attachment 388005 [details]
Patch

[Ryosuke Niwa]

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?

[Megan Gardner]

Created attachment 388101 [details]
Patch

[Ryosuke Niwa]

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.

[Wenson Hsieh]

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"

[Megan Gardner]

Created attachment 388121 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 388121 [details]
Patch for landing

Clearing flags on attachment: 388121

Committed r254785: <https://trac.webkit.org/changeset/254785>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58703201>

[Megan Gardner]

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