Bug 54053

Created attachment 81728 [details] demo

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

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.

(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.

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

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

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 :(

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.

Created attachment 81767 [details] fixes the bug

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.

<rdar://problem/8977569>

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.

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.

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