Avoid using cross RenderView selection rendering

Avoid using cross RenderView selection rendering

This patch makes sure we pass |RenderObject| belong to RenderView in
|RenderView::setSelection|, which takes two |RenderObject|s for start and end of
selection, in |FrameSeleciton::updateAppearance|.

The bug is caused by |VisibleSelection::base| and |VisibleSelection::start|
are in different document, |base| points to IFRAME and |start| points |TextNode|
in IFRAME. This causes |RenderView|, which holds |RenderObject|s of selection
start points and end points, have dangling |RenderObject|'s. Because,
|RenderView| doesn't know destructed |RenderObject| belongs to another
|RenderView|.

BUG=356690
TEST=LayoutTests/undo/execCommand/crash-redo-with-iframes.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171440

[yosin_UTC9, 2014-04-11 08:40:20]

Could you review this patch?

This is short term solution for fixing heap-use-after-free. For long term, we
should fix |VisibleSelection::validate()| to base/start and extent/end in same
document like |adjustSelectionToAvoidCrossingShadowBoundaries|. However, I'm not
sure what is correct behavior on this case, and take long time to implement.
And, it isn't easy to make extent/end point different documents.

Thanks in advance.

P.S.
Mac bot failure doesn't relate to this patch.

[Yuta Kitamura, 2014-04-11 08:59:22]

https://codereview.chromium.org/234463003/diff/1/Source/core/editing/FrameSel...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/234463003/diff/1/Source/core/editing/FrameSel...
Source/core/editing/FrameSelection.cpp:232: if (s.start().anchorNode()) {
As far as I know, the relationship among base, extent, start and end is either:
  - (base, extent) = (start, end), or
  - (base, extent) = (end, start)

So, I think this change just flips the coin of crashing condition upside down;
it may prevent some cases from crashing, but will create another chance of
crashes caused by different test cases.

I'm not sure what's the right move here, but it may be worth trying to check
the documents of both ends of |s| and early-exit if they do not match
(assuming the early exit won't cause a severe regression).

Or, VisibleSelection may validate the two ends and nullify itself if
they belong to different documents.

[yosin_UTC9, 2014-04-14 05:03:01]

PTAL

As discussed off-line, this is minimal short term solution for  fixing
heap-use-after-free.

[Yuta Kitamura, 2014-04-14 06:23:39]

LGTM

https://codereview.chromium.org/234463003/diff/40001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/234463003/diff/40001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1589: if (startRenderer->view() == view
&& endRenderer->view() == view)
This line would probably deserve some comments because it's not very obvious
why we need these checks.

[yosin_UTC9, 2014-04-14 08:03:56]

Thanks!
Committing...

https://codereview.chromium.org/234463003/diff/40001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/234463003/diff/40001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1589: if (startRenderer->view() == view
&& endRenderer->view() == view)
On 2014/04/14 06:23:40, Yuta Kitamura wrote:
> This line would probably deserve some comments because it's not very obvious
> why we need these checks.

Done.

[yosin_UTC9, 2014-04-14 08:04:01]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-04-14 08:04:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yosin@chromium.org/234463003/40001

[commit-bot: I haz the power, 2014-04-14 08:24:17]

Change committed as 171440