Harden FrameSelection methods against JS running inside recalcStyle

Harden FrameSelection methods against JS running inside recalcStyle

RenderView::selectionBounds and repaintSelection both call into
updateStyleIfNeeded which is not safe since the JS that can run as
the result of the recalcStyle can free the RenderView. Instead
FrameSelection should be doing this and adding the needed RefPtrs
to ensure it doesn't use free'd FrameViews or RenderViews.

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

[esprehn, 2013-09-27 22:44:53]

FrameSelection is scary and full of unsafe code. I don't have good test cases
for this, but I've seen stacks down here from ClusterFuzz before.

[abarth-chromium, 2013-09-27 22:55:00]

https://codereview.chromium.org/25004008/diff/1/Source/core/editing/FrameSele...
File Source/core/editing/FrameSelection.cpp (left):

https://codereview.chromium.org/25004008/diff/1/Source/core/editing/FrameSele...
Source/core/editing/FrameSelection.cpp:1558: if (RenderView* view =
m_frame->document()->renderView())
Should we bail out at some point if the document leaves the frame?

[esprehn, 2013-09-27 22:58:47]

On 2013/09/27 22:55:00, abarth wrote:
>
https://codereview.chromium.org/25004008/diff/1/Source/core/editing/FrameSele...
> File Source/core/editing/FrameSelection.cpp (left):
> 
>
https://codereview.chromium.org/25004008/diff/1/Source/core/editing/FrameSele...
> Source/core/editing/FrameSelection.cpp:1558: if (RenderView* view =
> m_frame->document()->renderView())
> Should we bail out at some point if the document leaves the frame?

Probably, but I don't really understand what all this code is doing. Why does
FrameSelection have any interaction with caps lock...

I can make this change but I was trying to avoid changing behavior.

[eseidel, 2013-09-27 23:04:26]

https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
File Source/core/rendering/RenderView.cpp (left):

https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
Source/core/rendering/RenderView.cpp:631: document().updateStyleIfNeeded();
Should these be ASSERT(!document->needsStylin())

[esprehn, 2013-09-27 23:14:59]

On 2013/09/27 23:04:26, eseidel wrote:
>
https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
> File Source/core/rendering/RenderView.cpp (left):
> 
>
https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
> Source/core/rendering/RenderView.cpp:631: document().updateStyleIfNeeded();
> Should these be ASSERT(!document->needsStylin())

I could, but it'd be trivial to hit those asserts since I can run script inside
::recalcStyle that makes the style invalid again after returning from
updateStyleIfNeeded(). See also the <use> hack inside Document::recalcStyle.

[esprehn, 2013-10-04 04:44:05]

On 2013/09/27 23:14:59, esprehn wrote:
> On 2013/09/27 23:04:26, eseidel wrote:
> >
>
https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
> > File Source/core/rendering/RenderView.cpp (left):
> > 
> >
>
https://codereview.chromium.org/25004008/diff/1/Source/core/rendering/RenderV...
> > Source/core/rendering/RenderView.cpp:631: document().updateStyleIfNeeded();
> > Should these be ASSERT(!document->needsStylin())
> 
> I could, but it'd be trivial to hit those asserts since I can run script
inside
> ::recalcStyle that makes the style invalid again after returning from
> updateStyleIfNeeded(). See also the <use> hack inside Document::recalcStyle.

Can you look at this again? This has been a source of a bunch of cluserfuzz
nagging over the past 6 months, I'd like to land it and see. :)

[eseidel, 2013-10-04 14:54:07]

lgtm

I feel like were just juggling here.

[commit-bot: I haz the power, 2013-10-04 14:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/25004008/1

[commit-bot: I haz the power, 2013-10-04 15:20:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-14 10:06:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/25004008/19001

[commit-bot: I haz the power, 2013-10-14 11:34:54]

Change committed as 159575