Fix use-after-free bug in long press selection

Fix use-after-free bug in long press text selection

The crash occurs because longpress may cause script to run (blur, focus event
handles) which  may detach the frame. To prevent crashing we protect FrameView
while handling the longpress event and also guard against |frame->settings()|
being NULL which happens when frame detaches.


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

[majidvp, 2015-08-28 21:06:13]

majidvp@chromium.org changed reviewers:
+ rbyers@chromium.org

[majidvp, 2015-08-28 21:09:12]

On 2015/08/28 21:06:13, majidvp wrote:

FYI, I ensured that the test reproduces the crash before applying the fix. See:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[Rick Byers, 2015-08-29 20:54:00]

LGTM

[yosin_UTC9, 2015-08-31 02:11:12]

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

[yosin_UTC9, 2015-08-31 02:11:13]

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash-expected.txt
(right):

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash-expected.txt:1:
Test passes if it does not crash.
nit: Can we use testharness.js to avoid expectation text,
which has async_test(), EventWatcher, etc?

Since this expected result doesn't have useful information.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html
(right):

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:21:
iframe1.addEventListener('load', finish, false);
nit: We don't need to have |false|.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:34:
setTimeout(function(){ testRunner.notifyDone(); }, 0);
nit: insert an space after |()|.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:37:
document.addEventListener("DOMContentLoaded", crash, false);
nit: We don't need to have |false|.

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2789: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
Does this change relate to crbug/519905?
It is better to have another CL for this guard.

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2812: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
Does this change relate to crbug/519905?
It is better to have another CL for this guard.

[majidvp, 2015-08-31 18:06:25]

yosin@: PTAL. I updated the test to use testharness.js.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash-expected.txt
(right):

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash-expected.txt:1:
Test passes if it does not crash.
On 2015/08/31 02:11:12, Yosi_UTC9 wrote:
> nit: Can we use testharness.js to avoid expectation text,
> which has async_test(), EventWatcher, etc?
> 
> Since this expected result doesn't have useful information.

Done!

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html
(right):

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:21:
iframe1.addEventListener('load', finish, false);
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> nit: We don't need to have |false|.

Acknowledged.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:21:
iframe1.addEventListener('load', finish, false);
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> nit: We don't need to have |false|.

Done.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:34:
setTimeout(function(){ testRunner.notifyDone(); }, 0);
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> nit: insert an space after |()|.

Done.

https://codereview.chromium.org/1315983004/diff/60001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:37:
document.addEventListener("DOMContentLoaded", crash, false);
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> nit: We don't need to have |false|.

Done.

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2789: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> Does this change relate to crbug/519905?
> It is better to have another CL for this guard.

It is necessary to fix the bug.

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2812: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> Does this change relate to crbug/519905?
> It is better to have another CL for this guard.

Ditto.

[yosin_UTC9, 2015-09-01 01:33:29]

lgtm w/ nits

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2789: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
On 2015/08/31 18:06:25, majidvp wrote:
> On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> > Does this change relate to crbug/519905?
> > It is better to have another CL for this guard.
> 
> It is necessary to fix the bug.

Please this describe this in description explicitly.
Current description doesn't tell us relationship between
this change and |m_frame->view()| protection.

BTW, when |m_frame->settings()| is |nullptr|, |m_frame| is
detached, e.g. after |Frame::detach()|. So, we may not want
to process an event for detached frame. If we check detached
frame by another way, we don't need to check null for settings.
Anyway, this is another topic.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html
(right):

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:2:

nit: We don't need to have an extra blank line.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:6:
"use strict";
nit: We don't need to have "use strict", because this crash occurs regardless
strict mode or not.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:25:
if (window.eventSender)
nit: It is better to check |window.eventSender| at top of this function.

[majidvp, 2015-09-01 15:33:53]

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1315983004/diff/60001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2789: if (m_frame->settings() &&
m_frame->settings()->showContextMenuOnMouseUp())
On 2015/09/01 01:33:28, Yosi_UTC9 wrote:
> On 2015/08/31 18:06:25, majidvp wrote:
> > On 2015/08/31 02:11:13, Yosi_UTC9 wrote:
> > > Does this change relate to crbug/519905?
> > > It is better to have another CL for this guard.
> > 
> > It is necessary to fix the bug.
> 
> Please this describe this in description explicitly.
> Current description doesn't tell us relationship between
> this change and |m_frame->view()| protection.
> 
> BTW, when |m_frame->settings()| is |nullptr|, |m_frame| is
> detached, e.g. after |Frame::detach()|. So, we may not want
> to process an event for detached frame. If we check detached
> frame by another way, we don't need to check null for settings.
> Anyway, this is another topic.

Updated the description to make it clear. 

The test does detach the frame in the process of handling the longpress
selection. I agree that an alternative solution is to stop handling the 
event when the frame gets detached. That requires checking
for detached frame after any call that may run script in this case
|FrameSelection::selectFrameElementInParentIfFullySelected()|.

I choose the current solution mainly because it is similar to other places
in EventHandler where we protect frameview to avoid its destruction 
while event is being processed.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
File
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html
(right):

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:2:

On 2015/09/01 01:33:29, Yosi_UTC9 wrote:
> nit: We don't need to have an extra blank line.

Done.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:6:
"use strict";
On 2015/09/01 01:33:29, Yosi_UTC9 wrote:
> nit: We don't need to have "use strict", because this crash occurs regardless
> strict mode or not.

That is correct. Given it is irrelevant I prefer to keep the strict mode. I
couldn't find any recommendation for or against it in style guide.

https://codereview.chromium.org/1315983004/diff/80001/LayoutTests/editing/sel...
LayoutTests/editing/selection/longpress-selection-in-iframe-removed-crash.html:25:
if (window.eventSender)
On 2015/09/01 01:33:29, Yosi_UTC9 wrote:
> nit: It is better to check |window.eventSender| at top of this function.

Done.

[majidvp, 2015-09-01 17:34:07]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2015-09-01 17:34:08]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1315983004/#ps100001
(title: "address feedback")

[commit-bot: I haz the power, 2015-09-01 17:34:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1315983004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1315983004/100001

[commit-bot: I haz the power, 2015-09-01 18:42:09]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201568