Reset SelectionState w/o propagation when clear old SeletionState.

We clear old selected but not selected LayoutObject's SelectionState
to kNone using LayoutObject::SetSelectionStateIfNeeded().
However the function calls virtual LayoutObject::SetSelectionState(),
which change containing blocks' SelectionState.
This causes inconsistency that marked not kNone LayoutObject can
accidentally marked kNone.

This patch changes it to call non virtual
LayoutObject::SetSelectionState() and assign kNone SelectionState
to only target old selected LayoutObjects.

Bug: 739062

Change-Id: I9ca736de43a2a982b1e29a1607325a1b77e3c35f
Reviewed-on: https://chromium-review.googlesource.com/748841
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513096}

third_party/WebKit/Source/core/editing/LayoutSelection.cpp

@@ -258,7 +258,7 @@ static void SetShouldInvalidateSelection(
   // above.
   for (LayoutObject* layout_object : old_invalidation_set) {
     const SelectionState old_state = layout_object->GetSelectionState();
-    layout_object->SetSelectionStateIfNeeded(SelectionState::kNone);
+    layout_object->LayoutObject::SetSelectionState(SelectionState::kNone);
     if (layout_object->GetSelectionState() == old_state)
       continue;
     SetShouldInvalidateIfNeeds(layout_object);
@@ -593,43 +593,15 @@ void LayoutSelection::Commit() {
   DCHECK(frame_selection_->GetDocument().GetLayoutView()->GetFrameView());
   SetShouldInvalidateSelection(new_range, paint_range_);
   paint_range_ = new_paint_range;
-  // TODO(yoichio): Remove this if state.
-  // This SelectionState reassignment is ad-hoc patch for
-  // prohibiting use-after-free(crbug.com/752715).
-  // LayoutText::setSelectionState(state) propergates |state| to ancestor
-  // LayoutObjects, which can accidentally change start/end LayoutObject state
-  // then LayoutObject::IsSelectionBorder() returns false although we should
-  // clear selection at LayoutObject::WillBeRemoved().
-  // We should make LayoutObject::setSelectionState() trivial and remove
-  // such propagation or at least do it in LayoutSelection.
-  if ((paint_range_.StartLayoutObject()->GetSelectionState() !=
-           SelectionState::kStart &&
-       paint_range_.StartLayoutObject()->GetSelectionState() !=
-           SelectionState::kStartAndEnd) ||
-      (paint_range_.EndLayoutObject()->GetSelectionState() !=
-           SelectionState::kEnd &&
-       paint_range_.EndLayoutObject()->GetSelectionState() !=
-           SelectionState::kStartAndEnd)) {
-    if (paint_range_.StartLayoutObject() == paint_range_.EndLayoutObject()) {
-      paint_range_.StartLayoutObject()->SetSelectionStateIfNeeded(
-          SelectionState::kStartAndEnd);
-    } else {
-      paint_range_.StartLayoutObject()->SetSelectionStateIfNeeded(
-          SelectionState::kStart);
-      paint_range_.EndLayoutObject()->SetSelectionStateIfNeeded(
-          SelectionState::kEnd);
-    }
+  if (paint_range_.StartLayoutObject() == paint_range_.EndLayoutObject()) {
+    DCHECK_EQ(paint_range_.StartLayoutObject()->GetSelectionState(),
+              SelectionState::kStartAndEnd);
+    return;
   }
-  // TODO(yoichio): If start == end, they should be kStartAndEnd.
-  // If not, start.SelectionState == kStart and vice versa.
-  DCHECK(paint_range_.StartLayoutObject()->GetSelectionState() ==
-             SelectionState::kStart ||
-         paint_range_.StartLayoutObject()->GetSelectionState() ==
-             SelectionState::kStartAndEnd);
-  DCHECK(paint_range_.EndLayoutObject()->GetSelectionState() ==
-             SelectionState::kEnd ||
-         paint_range_.EndLayoutObject()->GetSelectionState() ==
-             SelectionState::kStartAndEnd);
+  DCHECK_EQ(paint_range_.StartLayoutObject()->GetSelectionState(),
+            SelectionState::kStart);
+  DCHECK_EQ(paint_range_.EndLayoutObject()->GetSelectionState(),
+            SelectionState::kEnd);
 }
 
 void LayoutSelection::OnDocumentShutdown() {

third_party/WebKit/Source/core/editing/LayoutSelectionTest.cpp

@@ -266,10 +266,10 @@ TEST_F(LayoutSelectionTest,
           .Build());
   // This commit should not crash.
   Selection().CommitAppearanceIfNeeded();
-  TEST_NEXT(IsLayoutBlock, kNone, NotInvalidate);
+  TEST_NEXT(IsLayoutBlock, kStartAndEnd, NotInvalidate);
   TEST_NEXT(IsLayoutBlockFlow, kStartAndEnd, NotInvalidate);
   TEST_NEXT("div1", kStartAndEnd, ShouldInvalidate);
-  TEST_NEXT(IsLayoutBlockFlow, kNone, NotInvalidate);
+  TEST_NEXT(IsLayoutBlockFlow, kStartAndEnd, NotInvalidate);
   TEST_NEXT("foo", kNone, NotInvalidate);
   TEST_NEXT(IsLayoutInline, kNone, NotInvalidate);
   TEST_NEXT("bar", kNone, ShouldInvalidate);
@@ -364,8 +364,8 @@ TEST_F(LayoutSelectionTest, FirstLetterUpdateSeletion) {
                                .SetBaseAndExtent({baz, 2}, {baz, 3})
                                .Build());
   Selection().CommitAppearanceIfNeeded();
-  TEST_NEXT(IsLayoutBlock, kNone, NotInvalidate);
-  TEST_NEXT(IsLayoutBlock, kNone, NotInvalidate);
+  TEST_NEXT(IsLayoutBlock, kStartAndEnd, NotInvalidate);
+  TEST_NEXT(IsLayoutBlock, kStart, NotInvalidate);
   TEST_NEXT("foo", kNone, ShouldInvalidate);
   // TODO(yoichio): Invalidating next LayoutBlock is flaky but it doesn't
   // matter in wild because we don't paint selection gap. I will update