reset VisibleSelection at the Undo command.

Source/core/editing/Editor.cpp

Index: Source/core/editing/Editor.cpp
diff --git a/Source/core/editing/Editor.cpp b/Source/core/editing/Editor.cpp
index e2b1faa4d1c1eab468eb37be8e8b268ae69f51ec..c40754f96ce3206ad7e3dcb1e48c8a70f26843ec 100644
--- a/Source/core/editing/Editor.cpp
+++ b/Source/core/editing/Editor.cpp
@@ -710,6 +710,22 @@ void Editor::appliedEditing(PassRefPtrWillBeRawPtr<CompositeEditCommand> cmd)
     respondToChangedContents(newSelection);
 }
 
+static bool isValidPosition(const Position& position)
+{
+    if (!position.inDocument())
+        return false;
+
+    if (position.anchorType() != Position::PositionIsOffsetInAnchor)
+        return true;
+
+    if (position.offsetInContainerNode() < 0)
+        return false;
+
+    const unsigned offset = static_cast<unsigned>(position.offsetInContainerNode());
+    const unsigned nodeLength = Range::lengthOfContentsInNode(position.anchorNode());
+    return offset <= nodeLength;
+}
+
 void Editor::unappliedEditing(PassRefPtrWillBeRawPtr<EditCommandComposition> cmd)
 {
     EventQueueScope scope;
@@ -718,6 +734,8 @@ void Editor::unappliedEditing(PassRefPtrWillBeRawPtr<EditCommandComposition> cmd
     dispatchEditableContentChangedEvents(cmd->startingRootEditableElement(), cmd->endingRootEditableElement());
 
     VisibleSelection newSelection(cmd->startingSelection());
+    if (!isValidPosition(newSelection.base()) || !isValidPosition(newSelection.extent()) || !isValidPosition(newSelection.start()) || !isValidPosition(newSelection.end()))
+        newSelection = VisibleSelection(newSelection.base(), newSelection.extent(), newSelection.affinity(), newSelection.isDirectional());

[yosin_UTC9, 2014/06/04 01:17:30]

Q: Why do we create |VisibleSelection| from invalid positions?

[yoichio, 2014/06/04 02:01:46]

It's OK to create null VisibleSelection if there are invalid positions.
However, some of them have valid information like anchor Node and the
VisibleSelection constructor creates new valid VisibleSelection even from the
invalid Positions through the canonicalization.
I think it's better to recycle VisibleSelection from the information than to
reset VisibleSelection.

[yosin_UTC9, 2014/06/04 02:20:57]

I think it is better to reset rather than set unexpected value.
yutak@, How do you think?

[Yuta Kitamura, 2014/06/05 08:37:55]

First of all, in my view this line is very hard to understand, since it seems to
create another VisibleSelection with the exact same set of arguments. It's
totally
not obvious that it resets out-of-index positions.

My take is that VisibleSelection should be responsible for making sure it is in
a valid state. In that light, it should be VisibleSelection's copy constructor
to check the validity of Positions, not Editor.

If changing VisibleSelection's copy constructor is not feasible (because it
slows
down the performance or breaks compatibility), then VisibleSelection should have
a member function that corrects the positions, and lines 736-738 should be
replaced with something like:

    VisibleSelection newSelection(
        cmd->startingSelection().ensureValidPositions());

[yoichio, 2014/06/06 04:49:21]

VisibleSelection is a DISALLOW_ALLOCATION class. It means that its instances are
always only on stack and you must copy an object if you need another variable of
that.
Thus to insert ::valid() to the copy constructor may make performance down.
Additionally, VisibleSelection::valid() change a VisibleSelection which has
'valid' Positions because of the  valid() behavior to the notorious base,
extend, start and end relationship.

As conclusion, I add VisibleSelection::validatePositionsIfNeeded to validate
each position if its position is invalid.

     changeSelectionAfterCommand(newSelection, FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle);
 
     m_lastEditCommand = nullptr;