Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentVisibleSelectionNoCrash

Review-Url: https://codereview.chromium.org/2920583003
Cr-Commit-Position: refs/heads/master@{#477144}
Committed: https://chromium.googlesource.com/chromium/src/+/0cb66cf7b8cbc857c590ca5499f0ad6828684702

[Xiaocheng, 2017-06-01 00:40:50]

Description was changed from

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

BUG=725457
==========

to

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency.

This patch doesn't contain a regression test because it doesn't fix the root
cause. This patch is just a quick fix to be merged to M60.

BUG=725457
==========

[Xiaocheng, 2017-06-01 00:41:02]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-06-01 00:41:02]

PTAL.

[Xiaocheng, 2017-06-01 00:41:06]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 00:42:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-06-01 00:42:39]

Description was changed from

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency.

This patch doesn't contain a regression test because it doesn't fix the root
cause. This patch is just a quick fix to be merged to M60.

BUG=725457
==========

to

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency.

This patch doesn't contain a regression test because it doesn't fix the root
cause. This patch is just a quick fix to be merged to M60.

BUG=725457, 728441
==========

[yosin_UTC9, 2017-06-01 01:11:03]

On 2017/06/01 at 00:41:02, xiaochengh wrote:
> PTAL.

Could you wait next two weeks for collecting more samples?

[yosin_UTC9, 2017-06-01 01:13:12]

On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > PTAL.
> 
> Could you wait next two weeks for collecting more samples?

Culprit is:

PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
  ...
  if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
    if (anchor->CanParticipateInFlatTree() &&
        !FlatTreeTraversal::Parent(*anchor)) {
      // For Before/AfterAnchor, if |anchor| doesn't have parent in the flat
      // tree, there is no valid corresponding PositionInFlatTree.
      // Since this function is a primitive function, we do not adjust |pos|
      // to somewhere else in flat tree.
      // Reached by unit test
      // FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
      return PositionInFlatTree();
    }
  }
  // TODO(yosin): Once we have a test case for SLOT or active insertion point,
  // this function should handle it.
  return PositionInFlatTree(anchor, pos.AnchorType());
}

[commit-bot: I haz the power, 2017-06-01 03:00:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 03:00:31]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-06-02 00:35:51]

On 2017/06/01 at 01:13:12, yosin wrote:
> On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> > On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > > PTAL.
> > 
> > Could you wait next two weeks for collecting more samples?
> 
> Culprit is:
> 
> PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
>   ...
>   if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
>     if (anchor->CanParticipateInFlatTree() &&
>         !FlatTreeTraversal::Parent(*anchor)) {
>       // For Before/AfterAnchor, if |anchor| doesn't have parent in the flat
>       // tree, there is no valid corresponding PositionInFlatTree.
>       // Since this function is a primitive function, we do not adjust |pos|
>       // to somewhere else in flat tree.
>       // Reached by unit test
>       // FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
>       return PositionInFlatTree();
>     }
>   }
>   // TODO(yosin): Once we have a test case for SLOT or active insertion point,
>   // this function should handle it.
>   return PositionInFlatTree(anchor, pos.AnchorType());
> }

So this hack did cause inconsistency...

Should we do more hacking in SelectionEditor? In UpdateChachedVSInDOMTree, we
also check if the selection start and end can participate in flat tree, and use
them only if they can.

[yosin_UTC9, 2017-06-02 01:27:53]

On 2017/06/02 at 00:35:51, xiaochengh wrote:
> On 2017/06/01 at 01:13:12, yosin wrote:
> > On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> > > On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > > > PTAL.
> > > 
> > > Could you wait next two weeks for collecting more samples?
> > 
> > Culprit is:
> > 
> > PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
> >   ...
> >   if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
> >     if (anchor->CanParticipateInFlatTree() &&
> >         !FlatTreeTraversal::Parent(*anchor)) {
> >       // For Before/AfterAnchor, if |anchor| doesn't have parent in the flat
> >       // tree, there is no valid corresponding PositionInFlatTree.
> >       // Since this function is a primitive function, we do not adjust |pos|
> >       // to somewhere else in flat tree.
> >       // Reached by unit test
> >       // FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
> >       return PositionInFlatTree();
> >     }
> >   }
> >   // TODO(yosin): Once we have a test case for SLOT or active insertion
point,
> >   // this function should handle it.
> >   return PositionInFlatTree(anchor, pos.AnchorType());
> > }
> 
> So this hack did cause inconsistency...
> 
> Should we do more hacking in SelectionEditor? In UpdateChachedVSInDOMTree, we
also check if the selection start and end can participate in flat tree, and use
them only if they can.

Could you explain more about this "use them only if they can"?

toPositionInFlatTree() is used in
 - SelectionController
 - SelectionEditor

In SelectionController, toPositionInFlatTree() should be succeeded, because SC
works on "visible" positions.
In SelectionEditor, it should handle arbitrary cases. Since, JavaScript can
specify any position.

Here is proposal:
- Move toPostionInFlatTree() to SelectionController.cpp as file local function
and DCHECK(position is in flat tree).
- AdjustVSInDOMForVSInFlatTree() in SelectionEditor.cpp; adjustment for
"position isn't in flat tree"

The question is how do we map position can not be in flat tree.

[Xiaocheng, 2017-06-02 01:32:42]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-02 01:32:47]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-06-02 01:48:58]

On 2017/06/02 at 01:27:53, yosin wrote:
> On 2017/06/02 at 00:35:51, xiaochengh wrote:
> > On 2017/06/01 at 01:13:12, yosin wrote:
> > > On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> > > > On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > > > > PTAL.
> > > > 
> > > > Could you wait next two weeks for collecting more samples?
> > > 
> > > Culprit is:
> > > 
> > > PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
> > >   ...
> > >   if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
> > >     if (anchor->CanParticipateInFlatTree() &&
> > >         !FlatTreeTraversal::Parent(*anchor)) {
> > >       // For Before/AfterAnchor, if |anchor| doesn't have parent in the
flat
> > >       // tree, there is no valid corresponding PositionInFlatTree.
> > >       // Since this function is a primitive function, we do not adjust
|pos|
> > >       // to somewhere else in flat tree.
> > >       // Reached by unit test
> > >       // FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
> > >       return PositionInFlatTree();
> > >     }
> > >   }
> > >   // TODO(yosin): Once we have a test case for SLOT or active insertion
point,
> > >   // this function should handle it.
> > >   return PositionInFlatTree(anchor, pos.AnchorType());
> > > }
> > 
> > So this hack did cause inconsistency...
> > 
> > Should we do more hacking in SelectionEditor? In UpdateChachedVSInDOMTree,
we also check if the selection start and end can participate in flat tree, and
use them only if they can.
> 
> Could you explain more about this "use them only if they can"?
> 
> toPositionInFlatTree() is used in
>  - SelectionController
>  - SelectionEditor

We have more clients (via FromPositionInDOMTree):
- TextFinder::ScopeStringMatches
- FrameSelection::Contains

> 
> In SelectionController, toPositionInFlatTree() should be succeeded, because SC
works on "visible" positions.
> In SelectionEditor, it should handle arbitrary cases. Since, JavaScript can
specify any position.
> 
> Here is proposal:
> - Move toPostionInFlatTree() to SelectionController.cpp as file local function
and DCHECK(position is in flat tree).
> - AdjustVSInDOMForVSInFlatTree() in SelectionEditor.cpp; adjustment for
"position isn't in flat tree"
> 
> The question is how do we map position can not be in flat tree.

For SelectionEditor::UpdateVisibleSelectionInDOMTree, maybe we can do something
like:

Position start = ToPositionInFlatTree(selection_.Start()).IsNotNull() ?
selection_.Start() : Position();
Position end = ToPositionInFlatTree(selection_.End()).IsNotNull() ?
selection_.End() : Position()
cached_visible_selection_in_dom_tree = CreateVisibleSelection( ... start, end
...)

[yosin_UTC9, 2017-06-02 02:12:42]

On 2017/06/02 at 01:48:58, xiaochengh wrote:
> On 2017/06/02 at 01:27:53, yosin wrote:
> > On 2017/06/02 at 00:35:51, xiaochengh wrote:
> > > On 2017/06/01 at 01:13:12, yosin wrote:
> > > > On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> > > > > On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > > > > > PTAL.
> > > > > 
> > > > > Could you wait next two weeks for collecting more samples?
> > > > 
> > > > Culprit is:
> > > > 
> > > > PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
> > > >   ...
> > > >   if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
> > > >     if (anchor->CanParticipateInFlatTree() &&
> > > >         !FlatTreeTraversal::Parent(*anchor)) {
> > > >       // For Before/AfterAnchor, if |anchor| doesn't have parent in the
flat
> > > >       // tree, there is no valid corresponding PositionInFlatTree.
> > > >       // Since this function is a primitive function, we do not adjust
|pos|
> > > >       // to somewhere else in flat tree.
> > > >       // Reached by unit test
> > > >       // FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
> > > >       return PositionInFlatTree();
> > > >     }
> > > >   }
> > > >   // TODO(yosin): Once we have a test case for SLOT or active insertion
point,
> > > >   // this function should handle it.
> > > >   return PositionInFlatTree(anchor, pos.AnchorType());
> > > > }
> > > 
> > > So this hack did cause inconsistency...
> > > 
> > > Should we do more hacking in SelectionEditor? In UpdateChachedVSInDOMTree,
we also check if the selection start and end can participate in flat tree, and
use them only if they can.
> > 
> > Could you explain more about this "use them only if they can"?
> > 
> > toPositionInFlatTree() is used in
> >  - SelectionController
> >  - SelectionEditor
> 
> We have more clients (via FromPositionInDOMTree):
> - TextFinder::ScopeStringMatches
> - FrameSelection::Contains
> 
> > 
> > In SelectionController, toPositionInFlatTree() should be succeeded, because
SC works on "visible" positions.
> > In SelectionEditor, it should handle arbitrary cases. Since, JavaScript can
specify any position.
> > 
> > Here is proposal:
> > - Move toPostionInFlatTree() to SelectionController.cpp as file local
function and DCHECK(position is in flat tree).
> > - AdjustVSInDOMForVSInFlatTree() in SelectionEditor.cpp; adjustment for
"position isn't in flat tree"
> > 
> > The question is how do we map position can not be in flat tree.
> 
> For SelectionEditor::UpdateVisibleSelectionInDOMTree, maybe we can do
something like:
> 
> Position start = ToPositionInFlatTree(selection_.Start()).IsNotNull() ?
selection_.Start() : Position();
> Position end = ToPositionInFlatTree(selection_.End()).IsNotNull() ?
selection_.End() : Position()
> cached_visible_selection_in_dom_tree = CreateVisibleSelection( ... start, end
...)

I withdraw the proposal in #12.
We've gone wrong way from beginning...

When the position in DOM tree is not in flat tree, the position should not be
considered "visible" position.
Thus, both VisiblePosition and VisibleSelection should not hold such position.

It seems we could not handle "not in flat tree" case in
|CanonicalizePosition()|.
Functions called by |CanonicalizePosition()| return position satisfies
|IsVisuallyEquivalentCandidate()|

The root cause should be:
static bool IsVisuallyEquivalentCandidateAlgorithm(
    const PositionTemplate<Strategy>& position) {
  Node* const anchor_node = position.AnchorNode();
  if (!anchor_node)
    return false;

  LayoutObject* layout_object = anchor_node->GetLayoutObject();
  if (!layout_object)
    return false;
...
}

For {Before,After}Anchor, we can't catch children of shadow host.

The *simple* change is:

bool IsVisuallyEquivalentCandidate(const PositionInFlatTree& position) {
+ if (position is not in flat tree) return false;
  return IsVisuallyEquivalentCandidateAlgorithm<EditingInFlatTreeStrategy>(
      position);
}

We may also want to change |IsVisuallyEquivalentCandidate()| should use
container node to layout object instead of anchor node to handle
Before/AfterAnchor
correctly. However, we may have lots of layout test failure, and this doesn't
fix "position is not in flat tree" case, since shadow host has layout object.

[yosin_UTC9, 2017-06-02 05:07:37]

On 2017/06/02 at 02:12:42, yosin_UTC9 wrote:
> On 2017/06/02 at 01:48:58, xiaochengh wrote:
> > On 2017/06/02 at 01:27:53, yosin wrote:
> > > On 2017/06/02 at 00:35:51, xiaochengh wrote:
> > > > On 2017/06/01 at 01:13:12, yosin wrote:
> > > > > On 2017/06/01 at 01:11:03, yosin_UTC9 wrote:
> > > > > > On 2017/06/01 at 00:41:02, xiaochengh wrote:
> > > > > > > PTAL.
> > > > > > 
> > > > > > Could you wait next two weeks for collecting more samples?
> > > > > 
> > > > > Culprit is:
> > > > > 
> > > > > PositionInFlatTree ToPositionInFlatTree(const Position& pos) {
> > > > >   ...
> > > > >   if (pos.IsBeforeAnchor() || pos.IsAfterAnchor()) {
> > > > >     if (anchor->CanParticipateInFlatTree() &&
> > > > >         !FlatTreeTraversal::Parent(*anchor)) {
> > > > >       // For Before/AfterAnchor, if |anchor| doesn't have parent in
the flat
> > > > >       // tree, there is no valid corresponding PositionInFlatTree.
> > > > >       // Since this function is a primitive function, we do not adjust
|pos|
> > > > >       // to somewhere else in flat tree.
> > > > >       // Reached by unit test
> > > > >       //
FrameSelectionTest.SelectInvalidPositionInFlatTreeDoesntCrash.
> > > > >       return PositionInFlatTree();
> > > > >     }
> > > > >   }
> > > > >   // TODO(yosin): Once we have a test case for SLOT or active
insertion point,
> > > > >   // this function should handle it.
> > > > >   return PositionInFlatTree(anchor, pos.AnchorType());
> > > > > }
> > > > 
> > > > So this hack did cause inconsistency...
> > > > 
> > > > Should we do more hacking in SelectionEditor? In
UpdateChachedVSInDOMTree, we also check if the selection start and end can
participate in flat tree, and use them only if they can.
> > > 
> > > Could you explain more about this "use them only if they can"?
> > > 
> > > toPositionInFlatTree() is used in
> > >  - SelectionController
> > >  - SelectionEditor
> > 
> > We have more clients (via FromPositionInDOMTree):
> > - TextFinder::ScopeStringMatches
> > - FrameSelection::Contains
> > 
> > > 
> > > In SelectionController, toPositionInFlatTree() should be succeeded,
because SC works on "visible" positions.
> > > In SelectionEditor, it should handle arbitrary cases. Since, JavaScript
can specify any position.
> > > 
> > > Here is proposal:
> > > - Move toPostionInFlatTree() to SelectionController.cpp as file local
function and DCHECK(position is in flat tree).
> > > - AdjustVSInDOMForVSInFlatTree() in SelectionEditor.cpp; adjustment for
"position isn't in flat tree"
> > > 
> > > The question is how do we map position can not be in flat tree.
> > 
> > For SelectionEditor::UpdateVisibleSelectionInDOMTree, maybe we can do
something like:
> > 
> > Position start = ToPositionInFlatTree(selection_.Start()).IsNotNull() ?
selection_.Start() : Position();
> > Position end = ToPositionInFlatTree(selection_.End()).IsNotNull() ?
selection_.End() : Position()
> > cached_visible_selection_in_dom_tree = CreateVisibleSelection( ... start,
end ...)
> 
> I withdraw the proposal in #12.
> We've gone wrong way from beginning...
> 
> When the position in DOM tree is not in flat tree, the position should not be
considered "visible" position.
> Thus, both VisiblePosition and VisibleSelection should not hold such position.
> 
> It seems we could not handle "not in flat tree" case in
|CanonicalizePosition()|.
> Functions called by |CanonicalizePosition()| return position satisfies
|IsVisuallyEquivalentCandidate()|
> 
> The root cause should be:
> static bool IsVisuallyEquivalentCandidateAlgorithm(
>     const PositionTemplate<Strategy>& position) {
>   Node* const anchor_node = position.AnchorNode();
>   if (!anchor_node)
>     return false;
> 
>   LayoutObject* layout_object = anchor_node->GetLayoutObject();
>   if (!layout_object)
>     return false;
> ...
> }
> 
> For {Before,After}Anchor, we can't catch children of shadow host.
> 
> The *simple* change is:
> 
> bool IsVisuallyEquivalentCandidate(const PositionInFlatTree& position) {
> + if (position is not in flat tree) return false;
>   return IsVisuallyEquivalentCandidateAlgorithm<EditingInFlatTreeStrategy>(
>       position);
> }
> 
> We may also want to change |IsVisuallyEquivalentCandidate()| should use
> container node to layout object instead of anchor node to handle
Before/AfterAnchor
> correctly. However, we may have lots of layout test failure, and this doesn't
> fix "position is not in flat tree" case, since shadow host has layout object.

It seems I'm sleeping.

> bool IsVisuallyEquivalentCandidate(const Position& position) {
> + if (position is not in flat tree) return false;
>   return IsVisuallyEquivalentCandidateAlgorithm<EditingStrategy>(
>       position);
> }

[Xiaocheng, 2017-06-02 05:29:30]

Thanks for the idea. It indeed seems more reasonable to canonicalize such an
invalid position to null.

I'll think about it tmr.

[yosin_UTC9, 2017-06-02 08:59:11]

On 2017/06/02 at 05:29:30, xiaochengh wrote:
> Thanks for the idea. It indeed seems more reasonable to canonicalize such an
invalid position to null.
> 
> I'll think about it tmr.

It seems having DCHECK(CreateVisibleSelection(selection_).isNone()) <<
CreateVS(selection_)
after
 if (!cached_visible_selection_in_flat_tree_.IsNone())
    return;
in SelectionEditor::UpdateCachedVisibleSelectionInFlatTreeIfNeeded() 
is better.

When we hit this DCHECK, 

VS.base,extent,start,end = "\n"[0] after SVG.
Both "\n" and SVG have layout object

this.selection_.base_ = "\n"[0] in <text>; layout object is attached
this.selection_.extent_ = "\n"[1] in <text>
which is specified by svgvar00005.selectSubString(0,-1);

Strange thing is except for SVG, we expect "\n" text nodes has no layout object,
but it isn't. :-<

[yosin_UTC9, 2017-06-02 13:33:09]

On 2017/06/02 at 08:59:11, yosin_UTC9 wrote:
> On 2017/06/02 at 05:29:30, xiaochengh wrote:
> > Thanks for the idea. It indeed seems more reasonable to canonicalize such an
invalid position to null.
> > 
> > I'll think about it tmr.
> 
> It seems having DCHECK(CreateVisibleSelection(selection_).isNone()) <<
CreateVS(selection_)
> after
>  if (!cached_visible_selection_in_flat_tree_.IsNone())
>     return;
> in SelectionEditor::UpdateCachedVisibleSelectionInFlatTreeIfNeeded() 
> is better.
> 
> When we hit this DCHECK, 
> 
> VS.base,extent,start,end = "\n"[0] after SVG.
> Both "\n" and SVG have layout object
> 
> this.selection_.base_ = "\n"[0] in <text>; layout object is attached
> this.selection_.extent_ = "\n"[1] in <text>
> which is specified by svgvar00005.selectSubString(0,-1);
> 
> Strange thing is except for SVG, we expect "\n" text nodes has no layout
object, but it isn't. :-<

Yet, another idea.

bool FrameSelection::SelectionHasFocus() const {
  // TODO(editing-dev): Hoist UpdateStyleAndLayoutIgnorePendingStylesheets
  // to caller. See http://crbug.com/590369 for more details.
  GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
+ if (ComputeVisibleSelectionInFlatTree().isNone())
+   return false;
  const Node* current =
-      ComputeVisibleSelectionInDOMTree().Start().ComputeContainerNode();
+      ComputeVisibleSelectionInFlatTree().Start().ComputeContainerNode();
  if (!current)
    return false;

PROS: We compute only VSInFlatTree.
CONS: VSInDOM may have None or not-None for same value.

|current| may be different between VSInDOM and VSInFlat, but it is used
for walking ancestors up, this difference should not be an issue.

BTW, it seems we should check |focused_element| before UpdateStyleAndLayout().

In this way, we don't fix VSInDOM.isNone() != VSInFlat.IsNone().
To get samples, we can use DCHECK() mentioned in previous message #19.

[Xiaocheng, 2017-06-05 05:52:07]

Let me try to summarize.

We have two approaches:

1. Fix the root cause that, for a position that doesn't exist participate in
flat tree, it should be canonicalized to null. However, it seems quite hacky to
me to simply add a check of whether the position is in flat tree in
IsVisuallyEquivalentCandidate. I don't have a better idea to get a clean fix,
though.

2. As suggested in #20, set |current| to
VSInFlatTree.Start().ComputeContainerNode() as a workaround. The inconsistency
still remains there, so we still need to investigate it later.

I prefer 2 since we don't have a clean idea for 1 yet.

[yosin_UTC9, 2017-06-05 07:04:33]

On 2017/06/05 at 05:52:07, xiaochengh wrote:
> Let me try to summarize.
> 
> We have two approaches:
> 
> 1. Fix the root cause that, for a position that doesn't exist participate in
flat tree, it should be canonicalized to null. However, it seems quite hacky to
me to simply add a check of whether the position is in flat tree in
IsVisuallyEquivalentCandidate. I don't have a better idea to get a clean fix,
though.
> 
> 2. As suggested in #20, set |current| to
VSInFlatTree.Start().ComputeContainerNode() as a workaround. The inconsistency
still remains there, so we still need to investigate it later.
> 
> I prefer 2 since we don't have a clean idea for 1 yet.

Agree. Let's do option 2.

[Xiaocheng, 2017-06-05 18:41:48]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 18:42:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-06-05 18:47:59]

Description was changed from

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency.

This patch doesn't contain a regression test because it doesn't fix the root
cause. This patch is just a quick fix to be merged to M60.

BUG=725457, 728441
==========

to

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentSelectionNoCrash
==========

[Xiaocheng, 2017-06-05 18:48:10]

Description was changed from

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentSelectionNoCrash
==========

to

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentVisibleSelectionNoCrash
==========

[Xiaocheng, 2017-06-05 19:36:58]

Updated. PTAL.

[commit-bot: I haz the power, 2017-06-05 20:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 20:43:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Xiaocheng, 2017-06-05 20:45:52]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 20:46:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 22:50:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 22:50:48]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-06-06 01:11:57]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-06-06 01:11:57]

lgtm

[commit-bot: I haz the power, 2017-06-06 01:12:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 01:17:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496711517333060,
"parent_rev": "9ec4445dc173de920663a997a77966e04567e829", "commit_rev":
"0cb66cf7b8cbc857c590ca5499f0ad6828684702"}

[commit-bot: I haz the power, 2017-06-06 01:18:02]

Description was changed from

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentVisibleSelectionNoCrash
==========

to

==========
Stop crashing when VisibleSelections in DOM and Flat trees are inconsistent

There are some cases where FrameSelection::ComputeVisibleSelectionInDOMTree()
and FrameSelection::ComputeVisibleSelectionInFlatTree() return inconsistent
results, with one being null and another being non-null. This is unexpected
and causes crashes in some cases.

This patch stops the crash in FrameSelection::SelectionHasFocus() due to this
inconsistency. It doesn't fix the root cause, though, as the inconsistency is
still there. It is made small to be merged to M60.

BUG=725457
TEST=FrameSelectionTest.InconsistentVisibleSelectionNoCrash

Review-Url: https://codereview.chromium.org/2920583003
Cr-Commit-Position: refs/heads/master@{#477144}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cb66cf7b8cbc857c590ca5499f0...
==========

[commit-bot: I haz the power, 2017-06-06 01:18:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0cb66cf7b8cbc857c590ca5499f0...