[Crash][Editing] VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries() crashes in some Shadow DOM case

• Component: HTML Editing
• Status: RESOLVED FIXED

[Shinya Kawanaka]

HTML 
<div contenteditable>
    <div>BEFORE HOST</div>
    <div id="host" contenteditable="false">     a     </div>
</div>

Shadow DOM for host
<div id="shadow-host">SHADOW (BEFORE)<shadow></shadow>SHADOW (AFTER)</div>

Nested Shadow DOM for shadow-host
<div contenteditable>NESTED BEFORE<shadow></shadow>NESTED AFTER</div>


Select from 'a' to left, then ASSERTION triggered.

SHOULD NEVER BE REACHED
VisibleSelection.cpp(592) : adjustSelectionToAvoidCrossingEditingBoundaries()

[Shinya Kawanaka]

Created attachment 146263 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 146263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146263&action=review

> Source/WebCore/editing/VisibleSelection.cpp:496
> +        m_extent = adjustPositionForEnd(m_start.anchorNode()->treeScope(), m_end, m_start.containerNode());

Why are we using anchorNode's treeScope and then passing containerNode!?
anchorNode != containerNode. Can we just pass m_start.containerNode() here and obtain the tree scope inside the function?

[Shinya Kawanaka]

Created attachment 146775 [details]
Patch

[Shinya Kawanaka]

> Why are we using anchorNode's treeScope and then passing containerNode!?
> anchorNode != containerNode. Can we just pass m_start.containerNode() here and obtain the tree scope inside the function?

yeah... it was strange... I've update the patch. anchorNode()->treeScope() and containerNode()->treeScope() should be the same basically.

[Ryosuke Niwa]

Comment on attachment 146775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146775&action=review

> Source/WebCore/editing/VisibleSelection.cpp:501
> -        m_extent = adjustPositionBefore(m_start.anchorNode()->treeScope(), m_end);
> +        m_extent = adjustPositionForEnd(m_start.containerNode()->treeScope(), m_end, m_start.containerNode());

Again, why do we need to call treeScope here? You can just call it inside the function, right?

> Source/WebCore/editing/VisibleSelection.cpp:504
> -        m_extent = adjustPositionAfter(m_end.anchorNode()->treeScope(), m_start);
> +        m_extent = adjustPositionForStart(m_end.containerNode()->treeScope(), m_start, m_end.containerNode());

Ditto.

> LayoutTests/editing/shadow/breaking-editing-boundaries.html:10
> +<div contenteditable>
> +    BEFORE<div id="host" contenteditable="false">DRAG FROM<span id="target"> </span>HERE</div>    
> +</div>

Please add a test description.

[Shinya Kawanaka]

(In reply to comment #5)
> (From update of attachment 146775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146775&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:501
> > -        m_extent = adjustPositionBefore(m_start.anchorNode()->treeScope(), m_end);
> > +        m_extent = adjustPositionForEnd(m_start.containerNode()->treeScope(), m_end, m_start.containerNode());
> 
> Again, why do we need to call treeScope here? You can just call it inside the function, right?

Ah, OK. I didn't understand what you said...

[Shinya Kawanaka]

Created attachment 147466 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 147466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review

> Source/WebCore/editing/VisibleSelection.cpp:468
> +    if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) {        

What if this position was an offset in the anchor node? Why is it okay to move it before or after the node?

> Source/WebCore/editing/VisibleSelection.cpp:471
> +        if (ancestor->contains(startContainerNode))
> +            return positionAfterNode(ancestor);
> +        return positionBeforeNode(ancestor);

Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring.

[Shinya Kawanaka]

(In reply to comment #8)
> (From update of attachment 147466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:468
> > +    if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) {        
> 
> What if this position was an offset in the anchor node? Why is it okay to move it before or after the node?
> 

Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here. I should have asserted. (I'll update the patch soon)
So treeScope->ancestorInThisScope(currentPosition.containerNode()) will never return currentPosition.containerNode() itself here.

> > Source/WebCore/editing/VisibleSelection.cpp:471
> > +        if (ancestor->contains(startContainerNode))
> > +            return positionAfterNode(ancestor);
> > +        return positionBeforeNode(ancestor);
> 
> Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring.

Let's think:  '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C".

When selecting from A (including) to B (including), comparePosition() will judge as B is always before to A. So m_start will be B, and m_end will be A.  A and B have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(A).

Node that we cannot always move A to positionBeforeNode(A). When selecting from D (including) to B (including), we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust B to positionAfterNode(A).

[Shinya Kawanaka]

Created attachment 147497 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 147466 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147466&action=review
> > 
> > > Source/WebCore/editing/VisibleSelection.cpp:468
> > > +    if (Node* ancestor = treeScope->ancestorInThisScope(currentPosition.anchorNode())) {        
> > 
> > What if this position was an offset in the anchor node? Why is it okay to move it before or after the node?
> > 
> 
> Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here.

Could be, but not always.

> > > Source/WebCore/editing/VisibleSelection.cpp:471
> > > +        if (ancestor->contains(startContainerNode))
> > > +            return positionAfterNode(ancestor);
> > > +        return positionBeforeNode(ancestor);
> > 
> > Why do we need to modify position when the ancestor is in the same tree scope? That sounds red-herring.
> 
> Let's think:  '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C".
> 
> When selecting from A (including) to B (including), comparePosition() will judge as B is always before to A. So m_start will be B, and m_end will be A.

You can't say m_start is B. You can't talk about a position being at some node. A position is a point in the DOM tree consisting of anchor type, anchor, and offset.

There are 5 anchor types, and anchor and offset mean different things in each type:
PositionIsOffsetInAnchor - (anchor, offset) specifies the point in DOM. (div, 2) for example means that the position is before the 3rd children of the div (i.e. there are 2 children before this position).
PositionIsBeforeAnchor - The position is before the anchor; offset is ignored. i.e. if we have <div><em>a</em>b</div> and the anchor is at "b", then the position is after the em element and before "b". This is the same position as (PositionIsOffsetInAnchor, div, 1)
PositionIsAfterAnchor - The position is after the anchor. With the same example, the position is after "b" and is identical to (PositionIsOffsetInAnchor, div, 2)
PositionIsBeforeChildren - The position is the first point in the anchor. If the anchor is at the em element above, then the position is before "a" is identical to (PositionIsOffsetInAnchor, em, 0)
PositionIsAfterChildren - The position is the last point in the anchor. If the anchor is at the em element above, then the position is after "a" is identical to (PositionIsOffsetInAnchor, em, 1)

> A and B have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(A).
> 
> Node that we cannot always move A to positionBeforeNode(A). When selecting from D (including) to B (including), we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust B to positionAfterNode(A).

Because of this, what you're talking here is ambiguous. For example, the position immediately before the letter "B" can be expressed as (PositionIsOffsetInAnchor, shadow root, 0), (PositionIsBeforeChildren, shadow root), (PositionIsBeforeAnchor, "B", 0), or (PositionIsOffsetInAnchor, "B", 0).

[Shinya Kawanaka]

Comment on attachment 147497 [details]
Patch

Oops, I've uploaded a wrong patch...

[Ryosuke Niwa]

> the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move B to just after A (positionAfterNode(A)).

Here, for example, we need to first identify which of 6 possible positions we're talking about offset 0, offset 1, before, after positions anchored at the text node "B" and beforechildren, offset 0 positions anchored at the shadow root.

[Shinya Kawanaka]

> > Actually currentPosition.anchorNode()->treeScope() will not be equal to startContainerNode->treeScope() here.
> 
> Could be, but not always.

Since adjustPositionForEnd and adjustPositionForStart are used in adjustSelectionToAvoidCrossingShadowBoundaries() and it checks the the equality of treeScopes of m_start->anchorNode() and m_end->anchorNode()... if Position is valid, what kind of pattern makes them different?

> Because of this, what you're talking here is ambiguous. For example, the position immediately before the letter "B" can be expressed as (PositionIsOffsetInAnchor, shadow root, 0), (PositionIsBeforeChildren, shadow root), (PositionIsBeforeAnchor, "B", 0), or (PositionIsOffsetInAnchor, "B", 0).

OK. Let me try to make this strict... (in another reply)

[Build Bot]

Comment on attachment 147497 [details]
Patch

Attachment 147497 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12958134

[Shinya Kawanaka]

Let's think:  '<div id="host">A<div>D' and host having a ShadowRoot whose innerHTML is "B<content></content>C".

When selecting from p1 (PositionIsOffsetAnchor, div, 1) to p2 (PositionIsOffsetInAnchor, shadowRoot, 0), comparePosition() will judge as p2 is always before to p1. So m_start will be p2, and m_end will be p1. The anchor nodes of p1 and p2 have the different tree scope, we have to adjust them. However, the current implementation on of adjustSelectionToAvoidCrossingShadowBoundaries() always move p2 to just after A (positionAfterNode(div), i.e. (PositionIsAfterAnchor, div)). This breaks the assumption m_start is before m_end. So in that case, we have to adjust A to positionBeforeNode(div) i.e. (PositionIsBeforeAnchor, div).

Node that we cannot always move p1 to positionBeforeNode(div). When selecting from p3 (PositionIsOffsetAnchor, D's parent, 2),  to p2, we don't want to select A. It's like if <div id="host"></div> is <input>, we don't want to select <input>. So in this case we want to adjust p2 to positionAfterNode(div).

[Shinya Kawanaka]

Created attachment 147502 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 147502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147502&action=review

> Source/WebCore/ChangeLog:11
> +        For example, let's think '<div id="host">A</div>D' and host has a Shadow DOM whose innerHTML is

Nit: s/let's think/consider/

> Source/WebCore/ChangeLog:13
> +        'B<content></content>C'. When selecting from just after A (p1 = (PositionIsOffsetInAnchor, div, 1)) to
> +        B (p2 = (PositionIsOffsetInAnchor, shadowRoot, 0)), since comparePosition always judge as p2 is

How does (PositionIsOffsetInAnchor, div, 1) make sense? When we have a shadow DOM on #host, #host should be treated as an atomic node.
Selection should never be inside #host. Or is this a legacy editing offset?

[Shinya Kawanaka]

> > Source/WebCore/ChangeLog:13
> > +        'B<content></content>C'. When selecting from just after A (p1 = (PositionIsOffsetInAnchor, div, 1)) to
> > +        B (p2 = (PositionIsOffsetInAnchor, shadowRoot, 0)), since comparePosition always judge as p2 is
> 
> How does (PositionIsOffsetInAnchor, div, 1) make sense? When we have a shadow DOM on #host, #host should be treated as an atomic node.
> Selection should never be inside #host. Or is this a legacy editing offset?

Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host.
So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries().

[Ryosuke Niwa]

(In reply to comment #19)
> Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host.
> So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries().

Even before adjusting it, we should never have that position. Isn't it a legacy editing position? (i.e. m_isLegacyEditingPosition is true). If not, we need to figure out where this position is created and fix there as well.

[Shinya Kawanaka]

(In reply to comment #20)
> (In reply to comment #19)
> > Yeah, Selection should never be inside #host. But before adjusting, selection could be inside the host.
> > So I wrote as (PositionIsOffsetInAnchor, div, 1). It's a position before calling adjustSelectionToAvoidCrossingShadowBoundaries().
> 
> Even before adjusting it, we should never have that position. Isn't it a legacy editing position? (i.e. m_isLegacyEditingPosition is true). If not, we need to figure out where this position is created and fix there as well.

Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true)

[Ryosuke Niwa]

(In reply to comment #21)
> Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true)

Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host.

[Shinya Kawanaka]

(In reply to comment #22)
> (In reply to comment #21)
> > Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true)
> 
> Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host.

Really...?
OK. I apologize I used a reduced example. Let me try to explain using the real example.

# Before that I upload a new Patch...

[Shinya Kawanaka]

Created attachment 147726 [details]
Patch

[Shinya Kawanaka]

Created attachment 147727 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Sorry for late response. Yes. It's a legacy editing position. (m_isLegacyEditingPosition is true)
> > 
> > Okay, that makes sense. In that case, (host, 0) and (host, 1) should be treated like before and after the host.
> 
> Really...?

Yes. See https://rniwa.com/2011-06-26/position-and-anchor-types/  In legacy editing positions, when the anchor node is "atomic", offset 0 and offset 1 mean before and after the anchor.

[Shinya Kawanaka]

In the testcase, after performing eventSender.mouseMoveTo(nestedShadowHost.offsetLeft + 10, nestedShadowHost.offsetTop + nestedShadowHost.offsetHeight / 2),
VisibleSelection::adjustSelectionToAvoidCrossingShadowBoundaries() will be executed a few times.
In one of them, "if (m_baseIsFirst) { ... } " is executed.

In this moment,

m_baseIsFirst = false

m_start = {
    m_anchorNode = ...snip.../SPAN[@id="shadow-host"]/#shadow-root[0]/SPAN/text()
    m_offset = 1,
    m_anchorType = 0,
    m_isLegacyEditingPosition = true
}

m_end = {
    m_anchorNode = ...snip.../SPAN[@id="host" and position() = 0]/text()
    m_offset = 13,
    m_anchorType = 0,
    m_isLegacyEditingPosition = true
}


This does not seem that we should treat such positons as before the host or after the host.

I'm sorry for my explanation ambiguity...

[Shinya Kawanaka]

I'm very sorry that my previous explanation was not correct or strict.

[Ryosuke Niwa]

Comment on attachment 147727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147727&action=review

> LayoutTests/editing/shadow/breaking-editing-boundaries.html:12
> +    BEFORE<span id="host" contenteditable="false">DRAG FROM HERE<span id="target"> </span>FOO BAR</span>

Nit: ALL CAPS DON'T READ WELL. Please use normal capitalization.

[Shinya Kawanaka]

Created attachment 148141 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148141 [details]
Patch for landing

Rejecting attachment 148141 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12970785

[Shinya Kawanaka]

Created attachment 148184 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148184 [details]
Patch for landing

Clearing flags on attachment: 148184

Committed r120666: <http://trac.webkit.org/changeset/120666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.