Skip to content

view.TreeWalker may try to create positions "after root" in some specific scenarios #18744

@scofalik

Description

@scofalik

📝 Provide detailed reproduction steps (if any)

We were reported following editor crash that happened in some scenarios after the user clicked inside the editor.

Image

Looking at this stack trace, the error happens during view.Range#getTrimmed(), most probably fired for a selection range of selection created from DOM when handing selection change event:

const newViewSelection = this.domConverter.domSelectionToView( domSelection );

Regardless of the steps to reproduce this error, it seems that maybe in some cases view.Range#getTrimmed() can "step out of range" and lead to TreeWalker creating an incorrect view position (after an item which has no parent).

Looking further at the stack trace, we see that it crashes in view.Position#_createAfter(), which is called in view.TreeWalker#_formatReturnValue(). The only place where it could crash is here noted with /* CRASH */:

	private _formatReturnValue(
		type: ViewTreeWalkerValueType,
		item: ViewItem,
		previousPosition: ViewPosition,
		nextPosition: ViewPosition,
		length?: number
	): IteratorYieldResult<ViewTreeWalkerValue> {
		// Text is a specific parent, because contains string instead of children.
		// Walker doesn't enter to the Text except situations when walker is iterating over every single character,
		// or the bound starts/ends inside the Text. So when the position is at the beginning or at the end of the Text
		// we move it just before or just after Text.
		if ( item.is( 'view:$textProxy' ) ) {
			// Position is at the end of Text.
			if ( item.offsetInText + item.data.length == item.textNode.data.length ) {
				if ( this.direction == 'forward' && !( this.boundaries && this.boundaries.end.isEqual( this.position ) ) ) {
/* CRASH */				nextPosition = ViewPosition._createAfter( item.textNode );
					// When we change nextPosition of returned value we need also update walker current position.
					this._position = nextPosition;
				} else {
					previousPosition = ViewPosition._createAfter( item.textNode );
				}
			}

This is weird, because it means that a view text node did not have a parent, which most probably means this is a removed text node, and maybe the original view range to-be-trimmed also was only on that text node.

Regardless why such view selection was build from the DOM selection (and whether it is a bug or not), we may want to secure view.Range#getTrimmed() or TreeWalker#skip() or any of these method from this stack trace, so that they don't allow for this to happen.

Metadata

Metadata

Assignees

Labels

package:enginesupport:2An issue reported by a commercially licensed client.type:bugThis issue reports a buggy (incorrect) behavior.

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions