third_party/WebKit/Source/core/editing/SelectionController.cpp - Issue 2537243002: Adding |Node::layoutObject()| null check to |setCaretAtHitTestResult()|

Side by Side Diff: third_party/WebKit/Source/core/editing/SelectionController.cpp

Issue 2537243002: Adding |Node::layoutObject()| null check to |setCaretAtHitTestResult()| (Closed)

Patch Set: Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights	2 * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights

3 * reserved.	3 * reserved.

4 * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)	4 * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)

5 * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)	5 * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)

6 * Copyright (C) 2015 Google Inc. All rights reserved.	6 * Copyright (C) 2015 Google Inc. All rights reserved.

7 *	7 *

8 * Redistribution and use in source and binary forms, with or without	8 * Redistribution and use in source and binary forms, with or without

9 * modification, are permitted provided that the following conditions	9 * modification, are permitted provided that the following conditions

10 * are met:	10 * are met:

(...skipping 634 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
645	645

646 const FrameSelection::SetSelectionOptions options =	646 const FrameSelection::SetSelectionOptions options =

647 FrameSelection::CloseTyping \| FrameSelection::ClearTypingStyle;	647 FrameSelection::CloseTyping \| FrameSelection::ClearTypingStyle;

648 selection().setSelection(newSelection, options, CursorAlignOnScroll::IfNeeded,	648 selection().setSelection(newSelection, options, CursorAlignOnScroll::IfNeeded,

649 granularity);	649 granularity);

650 }	650 }

651	651

652 bool SelectionController::setCaretAtHitTestResult(	652 bool SelectionController::setCaretAtHitTestResult(

653 const HitTestResult& hitTestResult) {	653 const HitTestResult& hitTestResult) {

654 Node* innerNode = hitTestResult.innerNode();	654 Node* innerNode = hitTestResult.innerNode();

655 if (!innerNode)	655 if (!innerNode \|\| !innerNode->layoutObject())
	yosin_UTC9 2016/11/30 02:01:48 It is strange that HitTestResult picks up "display 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/12/01 01:25:34 I didn't think it was possible for the HitTestResu Show quoted text 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. Show quoted text > 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. Show quoted text > 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? Show quoted text > 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 We need to find where we call HitTestResult::setIn Show quoted text 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...
656 return false;	656 return false;

657	657

658 const VisiblePositionInFlatTree& visibleHitPos =	658 const VisiblePositionInFlatTree& visibleHitPos =

659 visiblePositionOfHitTestResult(hitTestResult);	659 visiblePositionOfHitTestResult(hitTestResult);

660 const VisiblePositionInFlatTree& visiblePos =	660 const VisiblePositionInFlatTree& visiblePos =

661 visibleHitPos.isNull()	661 visibleHitPos.isNull()

662 ? createVisiblePosition(	662 ? createVisiblePosition(

663 PositionInFlatTree::firstPositionInOrBeforeNode(innerNode))	663 PositionInFlatTree::firstPositionInOrBeforeNode(innerNode))

664 : visibleHitPos;	664 : visibleHitPos;

665	665

(...skipping 357 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1023 return event.event().altKey() && event.isOverLink();	1023 return event.event().altKey() && event.isOverLink();

1024 }	1024 }

1025	1025

1026 bool isExtendingSelection(const MouseEventWithHitTestResults& event) {	1026 bool isExtendingSelection(const MouseEventWithHitTestResults& event) {

1027 bool isMouseDownOnLinkOrImage =	1027 bool isMouseDownOnLinkOrImage =

1028 event.isOverLink() \|\| event.hitTestResult().image();	1028 event.isOverLink() \|\| event.hitTestResult().image();

1029 return event.event().shiftKey() && !isMouseDownOnLinkOrImage;	1029 return event.event().shiftKey() && !isMouseDownOnLinkOrImage;

1030 }	1030 }

1031	1031

1032 } // namespace blink	1032 } // namespace blink

OLD	NEW

« no previous file with comments | « no previous file | no next file » | no next file with comments »