Adding |Node::layoutObject()| null check to |setCaretAtHitTestResult()|

Adding null check of |layoutObject()| before calling |visiblePositionOfHitTestResult()|

BUG=667567
Committed: https://crrev.com/4807e64756edb7f9f8febaf2ae1ce45124b4c75e
Cr-Commit-Position: refs/heads/master@{#439073}

[amaralp, 2016-11-29 20:04:51]

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

[commit-bot: I haz the power, 2016-11-29 20:05:24]

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, 2016-11-29 22:42:53]

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

[commit-bot: I haz the power, 2016-11-29 22:42:54]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-11-29 23:04:22]

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

[amaralp, 2016-11-29 23:06:02]

Description was changed from

==========
Adding |Node::layoutObject()| null check

Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567
==========

to

==========
Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567
==========

[amaralp, 2016-11-29 23:06:20]

PTAL

[yosin_UTC9, 2016-11-30 02:01:48]

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionController.cpp:655: if
(!innerNode || !innerNode->layoutObject())
It is strange that HitTestResult picks up "display:none" node.
Also, I'm not sure why |innerNode| is null. I think it should be
DCHECK(innerNode); DCHECK(inner->layoutObject());

Could you add a test case for this situation?

Call sites of setCaretAtHitTestResult() should make sure provide sane
|HitTestResult|.

[amaralp, 2016-11-30 22:12:40]

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

[commit-bot: I haz the power, 2016-11-30 22:12:57]

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, 2016-11-30 22:15:28]

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

[commit-bot: I haz the power, 2016-11-30 22:15:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[amaralp, 2016-11-30 22:29:03]

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

[commit-bot: I haz the power, 2016-11-30 22:29:33]

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, 2016-12-01 01:21:46]

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

[commit-bot: I haz the power, 2016-12-01 01:21:46]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-12-01 01:25:34]

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionController.cpp:655: if
(!innerNode || !innerNode->layoutObject())
On 2016/11/30 at 02:01:48, Yosi_UTC9 wrote:
> It is strange that HitTestResult picks up "display:none" node.
I didn't think it was possible for the HitTestResult to have the |LayoutObject|
be null. It looks like all the other call sites of
|visiblePositionOfHitTestResult()| do this check so I am thinking that this is
probably what is causing the segmentation faults.

> Also, I'm not sure why |innerNode| is null. I think it should be
DCHECK(innerNode); DCHECK(inner->layoutObject());
> 
Maybe these DCHECKS should be added to |visiblePositionOfHitTestResult()| since
that is what is probably causing the crashes.

> Could you add a test case for this situation?
>  
How would I test this? Long-pressing based on the id of a display:none element?

> Call sites of setCaretAtHitTestResult() should make sure provide sane
|HitTestResult|.
I moved the check to the call sites although it looks like
|selectClosestWordFromHitTestResult()| and
|selectClosestMisspellingFromHitTestResult()| do the check themselves.

[yosin_UTC9, 2016-12-01 01:51:09]

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2537243002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/SelectionController.cpp:655: if
(!innerNode || !innerNode->layoutObject())
On 2016/12/01 at 01:25:34, amaralp wrote:
> On 2016/11/30 at 02:01:48, Yosi_UTC9 wrote:
> > It is strange that HitTestResult picks up "display:none" node.
> I didn't think it was possible for the HitTestResult to have the
|LayoutObject| be null. It looks like all the other call sites of
|visiblePositionOfHitTestResult()| do this check so I am thinking that this is
probably what is causing the segmentation faults.
> 
> > Also, I'm not sure why |innerNode| is null. I think it should be
DCHECK(innerNode); DCHECK(inner->layoutObject());
> > 
> Maybe these DCHECKS should be added to |visiblePositionOfHitTestResult()|
since that is what is probably causing the crashes.
> 
> > Could you add a test case for this situation?
> >  
> How would I test this? Long-pressing based on the id of a display:none
element?
We need to find where we call HitTestResult::setInnerNode() with
node->layoutObject() == nullptr. Then we can assemble such HTML and simulate
events in gTest.

Below is short investigation:

Insane HitTestReulst is calculated in GestureManager::handleGestureLongPress()
with

  IntPoint hitTestPoint =
      m_frame->view()->rootFrameToContents(gestureEvent.position());
  HitTestResult hitTestResult =
      m_frame->eventHandler().hitTestResultAtPoint(hitTestPoint);

Expect for <body style="display:none">, all touch event should have visible
node.
So, suspicious is rootFrameToContents() returns unexpected point then
hitTestResultAtPoint() returns insane HitTestResult.


In EventHandler::hitTestResultAtPoint(), following code fragment can return
insane HitTestResult:

  // LayoutView::hitTest causes a layout, and we don't want to hit that until
  // the first layout because until then, there is nothing shown on the screen -
  // the user can't have intentionally clicked on something belonging to this
  // page.  Furthermore, mousemove events before the first layout should not
  // lead to a premature layout() happening, which could show a flash of white.
  // See also the similar code in Document::performMouseEventHitTest.
  if (m_frame->contentLayoutItem().isNull() || !m_frame->view() ||
      !m_frame->view()->didFirstLayout())
    return result; **** innerNode == nullptr

  m_frame->contentLayoutItem().hitTest(result);


But this isn't innerNode()->layoutObject() == nullptr case.

Fumm...

https://codereview.chromium.org/2537243002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2537243002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/SelectionController.cpp:892: if
(!innerNode || !innerNode->layoutObject())
Could you hoist call site of this function or father?

Member functions of SelectionController should take sane HitTestResult, e.g.
innerNode() != nullptr && innerNode()->layoutObject() != nullptr

[amaralp, 2016-12-02 01:18:46]

> Expect for <body style="display:none">, all touch event should have visible
node.

I think there is a potential race-condition where a |Node| is long-pressed and
so we get it when we do the hit test in
|GestureManager::handleGestureLongPress()|. Then that Node's style is updated to
"display:none" and the call to
|Document::updateStyleAndLayoutTree()| in
SelectionController::handleGestureLongPress()| causes the node to detach from
the layout tree and set its |LayoutObject| to null.

The fix would be to do the hit test after
|Document::updateStyleAndLayoutTree()|.

Do you think this is a plausible explanation?

[yosin_UTC9, 2016-12-02 01:38:44]

On 2016/12/02 at 01:18:46, amaralp wrote:
> > Expect for <body style="display:none">, all touch event should have visible
node.
> 
> I think there is a potential race-condition where a |Node| is long-pressed and
so we get it when we do the hit test in
> |GestureManager::handleGestureLongPress()|. Then that Node's style is updated
to "display:none" and the call to
> |Document::updateStyleAndLayoutTree()| in
SelectionController::handleGestureLongPress()| causes the node to detach from
> the layout tree and set its |LayoutObject| to null.
> 
> The fix would be to do the hit test after
|Document::updateStyleAndLayoutTree()|.
> 
> Do you think this is a plausible explanation?

No. Let's identify which function make layout tree dirty. To change style of
node, we may dispatch event then JS event
handler does something.

Since The |MouseManager::handleDragDropIfPossible()| is the function called
between hitTest and SelectionController::handleGestureLongPress().
|MouseManager::handleDragDropIfPossible()| may update layout tree. However, I
could not locate dispatch event yet.

  HitTestResult hitTestResult =
      m_frame->eventHandler().hitTestResultAtPoint(hitTestPoint);

  if (!hitTestContainsLinks &&
      m_mouseEventManager->handleDragDropIfPossible(targetedEvent)) {
    m_longTapShouldInvokeContextMenu = true;
    return WebInputEventResult::HandledSystem;
  }


  if (m_selectionController->handleGestureLongPress(gestureEvent,
                                                    hitTestResult)) {



Could you utilize |ScriptForbiddenScope| where we call JS event handler during
processing gesture long press?

[amaralp, 2016-12-06 02:46:57]

On 2016/12/02 at 01:38:44, yosin wrote:
> On 2016/12/02 at 01:18:46, amaralp wrote:
> > > Expect for <body style="display:none">, all touch event should have
visible node.
> > 
> > I think there is a potential race-condition where a |Node| is long-pressed
and so we get it when we do the hit test in
> > |GestureManager::handleGestureLongPress()|. Then that Node's style is
updated to "display:none" and the call to
> > |Document::updateStyleAndLayoutTree()| in
SelectionController::handleGestureLongPress()| causes the node to detach from
> > the layout tree and set its |LayoutObject| to null.
> > 
> > The fix would be to do the hit test after
|Document::updateStyleAndLayoutTree()|.
> > 
> > Do you think this is a plausible explanation?
> 
> No. Let's identify which function make layout tree dirty. To change style of
node, we may dispatch event then JS event
> handler does something.
> 
> Since The |MouseManager::handleDragDropIfPossible()| is the function called
between hitTest and SelectionController::handleGestureLongPress().
> |MouseManager::handleDragDropIfPossible()| may update layout tree. However, I
could not locate dispatch event yet.
> 
>   HitTestResult hitTestResult =
>       m_frame->eventHandler().hitTestResultAtPoint(hitTestPoint);
> 
>   if (!hitTestContainsLinks &&
>       m_mouseEventManager->handleDragDropIfPossible(targetedEvent)) {
>     m_longTapShouldInvokeContextMenu = true;
>     return WebInputEventResult::HandledSystem;
>   }
> 
> 
>   if (m_selectionController->handleGestureLongPress(gestureEvent,
>                                                     hitTestResult)) {
> 
> 
> 
> Could you utilize |ScriptForbiddenScope| where we call JS event handler during
processing gesture long press?

I took a closer look at |handleDragDropIfPossible()| and from I can tell if it
dispatches a javascript event then
it returns |true| meaning that |SelectionController.handleGestureLongPress()|
never gets called.

This means that the |HitTestResult| had a null |LayoutObject| from when it was
created. I'll continue to investigate
scenarios where such a |HitTestResult| could be created.

[amaralp, 2016-12-07 22:12:06]

On 2016/12/06 at 02:46:57, amaralp wrote:
> On 2016/12/02 at 01:38:44, yosin wrote:
> > On 2016/12/02 at 01:18:46, amaralp wrote:
> > > > Expect for <body style="display:none">, all touch event should have
visible node.
> > > 
> > > I think there is a potential race-condition where a |Node| is long-pressed
and so we get it when we do the hit test in
> > > |GestureManager::handleGestureLongPress()|. Then that Node's style is
updated to "display:none" and the call to
> > > |Document::updateStyleAndLayoutTree()| in
SelectionController::handleGestureLongPress()| causes the node to detach from
> > > the layout tree and set its |LayoutObject| to null.
> > > 
> > > The fix would be to do the hit test after
|Document::updateStyleAndLayoutTree()|.
> > > 
> > > Do you think this is a plausible explanation?
> > 
> > No. Let's identify which function make layout tree dirty. To change style of
node, we may dispatch event then JS event
> > handler does something.
> > 
> > Since The |MouseManager::handleDragDropIfPossible()| is the function called
between hitTest and SelectionController::handleGestureLongPress().
> > |MouseManager::handleDragDropIfPossible()| may update layout tree. However,
I could not locate dispatch event yet.
> > 
> >   HitTestResult hitTestResult =
> >       m_frame->eventHandler().hitTestResultAtPoint(hitTestPoint);
> > 
> >   if (!hitTestContainsLinks &&
> >       m_mouseEventManager->handleDragDropIfPossible(targetedEvent)) {
> >     m_longTapShouldInvokeContextMenu = true;
> >     return WebInputEventResult::HandledSystem;
> >   }
> > 
> > 
> >   if (m_selectionController->handleGestureLongPress(gestureEvent,
> >                                                     hitTestResult)) {
> > 
> > 
> > 
> > Could you utilize |ScriptForbiddenScope| where we call JS event handler
during processing gesture long press?
> 
> I took a closer look at |handleDragDropIfPossible()| and from I can tell if it
dispatches a javascript event then
> it returns |true| meaning that |SelectionController.handleGestureLongPress()|
never gets called.
> 
> This means that the |HitTestResult| had a null |LayoutObject| from when it was
created. I'll continue to investigate
> scenarios where such a |HitTestResult| could be created.

As you had suggested going to "data:text/html,<html
style="display:none"></html>" and longpressing reproduces the crash.
I traced the problem to |LayoutView.updateHitTestResult()|. I think we'd need a
null check for this scenario.

[amaralp, 2016-12-08 23:40:05]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org, yukawa@chromium.org

[amaralp, 2016-12-08 23:51:06]

amaralp@chromium.org changed reviewers:
+ yutak@chromium.org
- yukawa@chromium.org

[amaralp, 2016-12-08 23:51:07]

Adding yukawa@ since yosin@ is OOO.

Here is a quick recap:

This CL is to fix the regression (crbug.com/667567) introduced by CL
(crrev.com/2480353006). I found a way to reproduce the crash. On an Android
device running M57 go to "data:text/html,<html style="display:none"></html>" and
long press. This should cause chrome to crash. This causes a nullptr exception
because the document node doesn't have a LayoutObject since it is
"display:none". My solution to this is adding a null check.

[yosin_UTC9, 2016-12-09 02:03:01]

On 2016/12/08 at 23:51:07, amaralp wrote:
> Adding yukawa@ since yosin@ is OOO.
> 
> Here is a quick recap:
> 
> This CL is to fix the regression (crbug.com/667567) introduced by CL
(crrev.com/2480353006). I found a way to reproduce the crash. On an Android
device running M57 go to "data:text/html,<html style="display:none"></html>" and
long press. This should cause chrome to crash. This causes a nullptr exception
because the document node doesn't have a LayoutObject since it is
"display:none". My solution to this is adding a null check.

Thanks for investigating root cause!

It seems touch event is dispatched for document not yet layout.
Repro case |<html style="display:none"></html>| is unusual and web author
doesn't write so.
One possibility is user tap on IFRAME being loaded.

Please do null check as much as bottom of call stack.

It seems EventHandler::handleGestureEvent() calls
GestureManager::handleGestureEventInFrame()
calls null frame case. Do we really need to call GM::handleGestureEventInFrame()
without local frame?
See the end of EventHandler::handleGestureEvent()

WebInputEventResult EventHandler::handleGestureEvent(
    const GestureEventWithHitTestResults& targetedEvent) {
  TRACE_EVENT0("input", "EventHandler::handleGestureEvent");

  ...

  // Route to the correct frame.
  if (LocalFrame* innerFrame = targetedEvent.hitTestResult().innerNodeFrame())
    return innerFrame->eventHandler().handleGestureEventInFrame(targetedEvent);

  // No hit test result, handle in root instance. Perhaps we should just return
  // false instead?
  return m_gestureManager->handleGestureEventInFrame(targetedEvent);
}

[amaralp, 2016-12-09 03:03:18]

On 2016/12/09 at 02:03:01, yosin wrote:
> Thanks for investigating root cause!
> 
> It seems touch event is dispatched for document not yet layout.
> Repro case |<html style="display:none"></html>| is unusual and web author
doesn't write so.
> One possibility is user tap on IFRAME being loaded.
> 
> Please do null check as much as bottom of call stack.
> 
Pulled to GestureManager.

> It seems EventHandler::handleGestureEvent() calls
GestureManager::handleGestureEventInFrame()
> calls null frame case. Do we really need to call
GM::handleGestureEventInFrame() without local frame?
> See the end of EventHandler::handleGestureEvent()
> 
> WebInputEventResult EventHandler::handleGestureEvent(
>     const GestureEventWithHitTestResults& targetedEvent) {
>   TRACE_EVENT0("input", "EventHandler::handleGestureEvent");
> 
>   ...
> 
>   // Route to the correct frame.
>   if (LocalFrame* innerFrame = targetedEvent.hitTestResult().innerNodeFrame())
>     return
innerFrame->eventHandler().handleGestureEventInFrame(targetedEvent);
> 
>   // No hit test result, handle in root instance. Perhaps we should just
return
>   // false instead?
>   return m_gestureManager->handleGestureEventInFrame(targetedEvent);
> }

The callstacks from clusterfuzz
(https://cluster-fuzz.appspot.com/v2/testcase-detail/4784817788157952) and the
chrome crash report:

https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%...

seem to indicate that the if statement at the end of |EH::handleGestureEvent()|
was true and so we actually did have a local frame.

[amaralp, 2016-12-14 23:33:04]

amaralp@chromium.org changed reviewers:
+ bokan@chromium.org

[amaralp, 2016-12-14 23:36:09]

Adding bokan@ for core/input OWNERS.

yutak@, do you think you could review this? yosin@ is OOO until the beginning of
January and
this is a M57 blocker.

[bokan, 2016-12-15 20:56:04]

lgtm but please wait for yutak@'s thumbs up.

[Yuta Kitamura, 2016-12-16 06:01:11]

LGTM

[amaralp, 2016-12-16 07:34:10]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2016-12-16 07:34:23]

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, 2016-12-16 09:17:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481873650550500,
"parent_rev": "d3149dd85e8d4cf6acd5357d0e9aa5940d268a7a", "commit_rev":
"4bae92a3db37327214fb863c11cb5c6549b96408"}

[commit-bot: I haz the power, 2016-12-16 09:17:45]

Description was changed from

==========
Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567
==========

to

==========
Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567

Review-Url: https://codereview.chromium.org/2537243002
==========

[commit-bot: I haz the power, 2016-12-16 09:17:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-16 09:19:43]

Description was changed from

==========
Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567

Review-Url: https://codereview.chromium.org/2537243002
==========

to

==========
Adding null check of |layoutObject()| before calling
|visiblePositionOfHitTestResult()|

BUG=667567

Committed: https://crrev.com/4807e64756edb7f9f8febaf2ae1ce45124b4c75e
Cr-Commit-Position: refs/heads/master@{#439073}
==========

[commit-bot: I haz the power, 2016-12-16 09:19:44]

Patchset 4 (id:??) landed as
https://crrev.com/4807e64756edb7f9f8febaf2ae1ce45124b4c75e
Cr-Commit-Position: refs/heads/master@{#439073}