Bug 54053 – REGRESSION(r76107): Crash in VisibleSelection::toNormalizedRange

• Component: Forms
• Status: RESOLVED FIXED

[Ryosuke Niwa]

#0	0x1018deb28 in WebCore::RenderBlock::positionForBox at RenderBlock.cpp:3965
#1	0x1018def8b in WebCore::RenderBlock::positionForPointWithInlineChildren at RenderBlock.cpp:4053
#2	0x1018df1d9 in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4092
#3	0x1018de993 in WebCore::positionForPointRespectingEditingBoundaries at RenderBlock.cpp:3992
#4	0x1018df24f in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4097
#5	0x101277b76 in WebCore::EventHandler::handleMousePressEventSingleClick at EventHandler.cpp:374
#6	0x10127a5f5 in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:480
#7	0x10127dd48 in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:1402
#8	0x1012827e4 in WebCore::EventHandler::mouseDown at EventHandlerMac.mm:506

In PositionForBox, box->renderer->node() is returning a stale pointer.

http://crbug.com/71264

[Ryosuke Niwa]

Created attachment 81728 [details]
demo

[Ryosuke Niwa]

It seems like inline boxes under INPUT's renderer is giving us a bad pointer.

[Ryosuke Niwa]

Here's more detailed information.

We're inside positionForPointWithInlineChildren where we have:
    if (lastRootBoxWithChildren) {
        // We hit this case for Mac behavior when the Y coordinate is below the last box.
        ASSERT(moveCaretToBoundary);
        return VisiblePosition(positionForBox(lastRootBoxWithChildren->lastLeafChild(), false), DOWNSTREAM); // < this is where we're now.
    }

stack trace:
#0	0x1018def66 in WebCore::RenderBlock::positionForPointWithInlineChildren at RenderBlock.cpp:4053
#1	0x1018df1d9 in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4092
#2	0x1018de993 in WebCore::positionForPointRespectingEditingBoundaries at RenderBlock.cpp:3992
#3	0x1018df24f in WebCore::RenderBlock::positionForPoint at RenderBlock.cpp:4097
#4	0x101277b76 in WebCore::EventHandler::handleMousePressEventSingleClick at EventHandler.cpp:374
#5	0x10127a5f5 in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:480
#6	0x10127dd48 in WebCore::EventHandler::handleMousePressEvent at EventHandler.cpp:1402
#7	0x1012827e4 in WebCore::EventHandler::mouseDown at EventHandlerMac.mm:506

(gdb) p showRenderTree(this)
RenderView 0x107051858                 	#document	0x107882000
  RenderBlock 0x107051c68              	HTML	0x107089a90
    RenderBody 0x107076798             	BODY	0x10704e1b0
      RenderBlock 0x107083c18          	P	0x107068580
        RenderText 0x107076888         	#text	0x10704d030 "This tests clicking a region immediately after file input field does not crash WebKit.\nThis manually test, click on the black region below."
      RenderBlock (positioned) 0x107076dd8	DIV	0x1070769d0 STYLE=width: 200px; height: 200px; position: absolute; top: 20px; left: 0px; background: black;
*       RenderFileUploadControl 0x105b3a708	INPUT	0x10704d240 STYLE=width: 100px; height: 20px; display: block; padding: 10px; background: white;
          RenderButton 0x1070a8c18     	INPUT	0x105b39810
            RenderBlock (anonymous) 0x1070a8e18
              RenderText 0x1070a8d08
$3 = void
(gdb) p showTree(this->node())
BODY	0x10704e1b0
	#text	0x107092800 "\n"
	P	0x107068580
		#text	0x10704d030 "This tests clicking a region immediately after file input field does not crash WebKit.\nThis manually test, click on the black region below."
	#text	0x1070aba80 "\n"
	DIV	0x1070769d0 STYLE=width: 200px; height: 200px; position: absolute; top: 20px; left: 0px; background: black;
		#text	0x10704d600 "\n"
*		INPUT	0x10704d240 STYLE=width: 100px; height: 20px; display: block; padding: 10px; background: white;
		#text	0x1070a9010 "\n"
	#text	0x10704d1c0 "\n"
	SCRIPT	0x1070a9070
		#text	0x1070a93c0 "\n\nif (window.layoutTestController && window.eventSender) {\n    layoutTestController.dumpAsText();\n\n    eventSender.mouseMoveTo(180, 50);\n    eventSender.leapForward(200);\n    eventSender.mouseDown();\n    eventSender.leapForward(200);\n    eventSender.mouseUp();\n    eventSender.leapForward(200);\n    document.writeln('PASS');\n}\n\n"
$4 = void
(gdb) p lastRootBoxWithChildren->renderer()
$5 = (const 'WebCore::RenderObject' *) 0x105b3a708
(gdb) p lastRootBoxWithChildren->lastLeafChild()
$6 = (class WebCore::InlineBox *) 0x1070a0ab8
(gdb) p lastRootBoxWithChildren->lastLeafChild()->renderer()
$7 = (const 'WebCore::RenderObject' *) 0x1070a8c18
(gdb) p lastRootBoxWithChildren->lastLeafChild()->renderer()->node()
$8 = (class WebCore::Node *) 0x105b39810
(gdb) p showTree(lastRootBoxWithChildren->lastLeafChild()->renderer()->node())
+-荈 (0x105b39810)

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
0x000000010139901a in WTF::RefPtr<WebCore::HistoryItem>::operator-> (this=0x0) at RefPtr.h:66
66	        ALWAYS_INLINE T* operator->() const { return m_ptr; }
The program being debugged was signaled while in a function called from GDB.
GDB remains in the frame where the signal was received.
To change this behavior use "set unwindonsignal on"
Evaluation of the expression containing the function (showTree) will be abandoned.

In summary, RenderFileUploadControl's node() is returning a bad pointer.

[Ryosuke Niwa]

(In reply to comment #3)
> In summary, RenderFileUploadControl's node() is returning a bad pointer.

Oops, this isn't quite right.  It's the shadowInputElement (m_button) under RenderFileUploadControl that's returning a bogus pointer.

[Ryosuke Niwa]

This is a regression.  The crash reproduces on r77919 but does not reproduce on r73316.

[Ryosuke Niwa]

r75294: ok
r75891: ok
r76498: bad!
r76640: bad!
r77034: bad!
r77282: bad!

[Ryosuke Niwa]

Ah, it seems like this isn't really a security bug.  showTree can print the tree if I explicitly cast the pointer into Node*.  It seems like this is a bug in GDB :(

[Ryosuke Niwa]

Ah, this crash itself is a regression from r76107 but the problem is inherent to the situation.  In this case, the hit test code returns a position inside a button control inside an input[type=file]; i.e. [input, 0].  Now, this isn't a valid DOM position because input can't have children so we used to hit an assertion in Range.  Now r76107 changed the code to always use the parent anchored position but this poses a problem because input doesn't have any parent (it's the only element in the shadow DOM)!

I think we need to make it so that shadow DOM has a document-like root node.

[Ryosuke Niwa]

Created attachment 81767 [details]
fixes the bug

[Ryosuke Niwa]

Comment on attachment 81767 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=81767&action=review

> Source/WebCore/rendering/RenderFileUploadControl.h:45
> +    // Shouldn't hit-test inside file upload control.

I'm going to remove this useless comment before I land if this patched was r+ed.

[Lucas Forschler]

<rdar://problem/8977569>

[Darin Adler]

Comment on attachment 81767 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=81767&action=review

> Source/WebCore/rendering/RenderFileUploadControl.h:46
> +    VisiblePosition positionForPoint(const IntPoint&);

Better to make this function private. Also, we normally specify virtual explicitly when overriding a virtual function.

[Ryosuke Niwa]

Thanks for the review as always :)

(In reply to comment #12)
> (From update of attachment 81767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81767&action=review
> 
> > Source/WebCore/rendering/RenderFileUploadControl.h:46
> > +    VisiblePosition positionForPoint(const IntPoint&);
> 
> Better to make this function private. Also, we normally specify virtual explicitly when overriding a virtual function.

Will do.

[Ryosuke Niwa]

Committed r78168: <http://trac.webkit.org/changeset/78168>