Assertion failure in RenderBlock::positionForPointWithInlineChildren when running fast/inline/relative-positioned-overflow.html

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

Debug builds on Windows are hitting an assertion in RenderBlock::positionForPointWithInlineChildren when running fast/inline/relative-positioned-overflow.html

To reproduce:

1. run-webkit-tests fast/inline/relative-positioned-overflow.html

You'll hit this assertion:

        ASSERT(!useWindowsBehavior);

<http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderBlock.cpp?rev=48679#L3437>

Here's the backtrace:

>	WebKit_debug.dll!WebCore::RenderBlock::positionForPointWithInlineChildren(const WebCore::IntPoint & pointInContents={...})  Line 3437 + 0x24 bytes	C++
 	WebKit_debug.dll!WebCore::RenderBlock::positionForPoint(const WebCore::IntPoint & point={...})  Line 3470 + 0x10 bytes	C++
 	WebKit_debug.dll!WebCore::RenderInline::positionForPoint(const WebCore::IntPoint & point={...})  Line 503 + 0x1a bytes	C++
 	WebKit_debug.dll!WebCore::EventHandler::handleMousePressEventSingleClick(const WebCore::MouseEventWithHitTestResults & event={...})  Line 323 + 0x3a bytes	C++
 	WebKit_debug.dll!WebCore::EventHandler::handleMousePressEvent(const WebCore::MouseEventWithHitTestResults & event={...})  Line 418 + 0xc bytes	C++
 	WebKit_debug.dll!WebCore::EventHandler::handleMousePressEvent(const WebCore::PlatformMouseEvent & mouseEvent={...})  Line 1265 + 0xf bytes	C++
 	WebKit_debug.dll!WebView::handleMouseEvent(unsigned int message=513, unsigned int wParam=0, long lParam=3276850)  Line 1302 + 0x1d bytes	C++
 	WebKit_debug.dll!WebViewWndProc(HWND__ * hWnd=0x0005058e, unsigned int message=513, unsigned int wParam=0, long lParam=3276850)  Line 1908 + 0x14 bytes	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x28 bytes	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7 bytes	
 	user32.dll!_DispatchMessageWorker@8()  + 0xdc bytes	
 	user32.dll!_DispatchMessageW@4()  + 0xf bytes	
 	DumpRenderTree_debug.exe!dispatchMessage(const tagMSG * msg=0x0012e158)  Line 128 + 0xc bytes	C++
 	DumpRenderTree_debug.exe!mouseDownCallback(const OpaqueJSContext * context=0x06400098, OpaqueJSValue * function=0x06843100, OpaqueJSValue * thisObject=0x06842b80, unsigned int argumentCount=0, const OpaqueJSValue * const * arguments=0x0012e1f8, const OpaqueJSValue * * exception=0x0012e1dc)  Line 179 + 0x9 bytes	C++
 	JavaScriptCore_debug.dll!JSC::JSCallbackFunction::call(JSC::ExecState * exec=0x06400098, JSC::JSObject * functionObject=0x06843100, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 65 + 0x27 bytes	C++
 	JavaScriptCore_debug.dll!cti_op_call_NotJSFunction(void * * args=0x0012e350)  Line 1607 + 0x31 bytes	C++
 	JavaScriptCore_debug.dll!@cti_op_convert_this@4()  + 0x10f bytes	C++
 	JavaScriptCore_debug.dll!JSC::JITCode::execute(JSC::RegisterFile * registerFile=0x04f31eac, JSC::ExecState * callFrame=0x06400048, JSC::JSGlobalData * globalData=0x04f16608, JSC::JSValue * exception=0x0012e4a0)  Line 79 + 0x24 bytes	C++
 	JavaScriptCore_debug.dll!JSC::Interpreter::execute(JSC::ProgramExecutable * program=0x04f62dc8, JSC::ExecState * callFrame=0x04f323e0, JSC::ScopeChainNode * scopeChain=0x04f32598, JSC::JSObject * thisObj=0x06840000, JSC::JSValue * exception=0x0012e4a0)  Line 658 + 0x31 bytes	C++
 	JavaScriptCore_debug.dll!JSC::evaluate(JSC::ExecState * exec=0x04f323e0, JSC::ScopeChain & scopeChain={...}, const JSC::SourceCode & source={...}, JSC::JSValue thisValue={...})  Line 62	C++
 	WebKit_debug.dll!WebCore::ScriptController::evaluate(const WebCore::ScriptSourceCode & sourceCode={...})  Line 110 + 0x34 bytes	C++
 	WebKit_debug.dll!WebCore::FrameLoader::executeScript(const WebCore::ScriptSourceCode & sourceCode={...})  Line 807	C++
 	WebKit_debug.dll!WebCore::HTMLTokenizer::scriptExecution(const WebCore::ScriptSourceCode & sourceCode={...}, WebCore::HTMLTokenizer::State state={...})  Line 563 + 0x27 bytes	C++
 	WebKit_debug.dll!WebCore::HTMLTokenizer::scriptHandler(WebCore::HTMLTokenizer::State state={...})  Line 505 + 0x88 bytes	C++
 	WebKit_debug.dll!WebCore::HTMLTokenizer::parseNonHTMLText(WebCore::SegmentedString & src={...}, WebCore::HTMLTokenizer::State state={...})  Line 352 + 0x10 bytes	C++
 	WebKit_debug.dll!WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString & src={...}, WebCore::HTMLTokenizer::State state={...})  Line 1522 + 0x17 bytes	C++
 	WebKit_debug.dll!WebCore::HTMLTokenizer::write(const WebCore::SegmentedString & str={...}, bool appendData=true)  Line 1756 + 0x1c bytes	C++
 	WebKit_debug.dll!WebCore::FrameLoader::write(const char * str=0x00000000, int len=0, bool flush=true)  Line 1054 + 0x27 bytes	C++
 	WebKit_debug.dll!WebCore::FrameLoader::endIfNotLoadingMainResource()  Line 1090	C++
 	WebKit_debug.dll!WebCore::FrameLoader::end()  Line 1076	C++
 	WebKit_debug.dll!WebCore::DocumentLoader::finishedLoading()  Line 332	C++
 	WebKit_debug.dll!WebCore::FrameLoader::finishedLoading()  Line 3264	C++
 	WebKit_debug.dll!WebCore::MainResourceLoader::didFinishLoading()  Line 376	C++
 	WebKit_debug.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x04e9eba0)  Line 403 + 0xf bytes	C++
 	WebKit_debug.dll!WebCore::didFinishLoading(_CFURLConnection * conn=0x04eff160, const void * clientInfo=0x04e9eba0)  Line 228 + 0x1e bytes	C++

[Adam Roben (:aroben)]

This test seems to pass in Release builds.

[Adam Roben (:aroben)]

<rdar://problem/7267988>

[Eric Seidel (no email)]

Was this a regression?  Do we know yet from which revision?

[Shinichiro Hamaji]

Created attachment 43914 [details]
Patch v1

[Shinichiro Hamaji]

Created attachment 43915 [details]
test case

[Shinichiro Hamaji]

Created attachment 43916 [details]
Patch v2

[Shinichiro Hamaji]

Comment on attachment 43914 [details]
Patch v1

This was a bit older patch.

[Shinichiro Hamaji]

This issue happens when an empty inline element (which has size because of paddings) is clicked. In this case, the root inline box has no leaf children and closestBox can be NULL even if we are using windows behavior.

We may need to use lastChild instead of lastLeafChild when lastLeafChild is NULL so that the mouse press starts from the empty inline element correctly. See the attached test case, which is the same HTML as the layout test I added, to see how dragging from an empty inline element is handled now.

Chromium has the same issue: http://code.google.com/p/chromium/issues/detail?id=23751

Comment on attachment 43916 [details]
Patch v2

Thanks for working on this bug!

> +    eventSender.mouseMoveTo(x, y + 50);
> +    eventSender.mouseDown();
> +    eventSender.mouseMoveTo(x, y + 40);
> +    eventSender.mouseMoveTo(x, y + 30);
> +    eventSender.mouseMoveTo(x, y + 20);
> +    eventSender.mouseMoveTo(x, y + 10);
> +    eventSender.mouseMoveTo(x, y);
> +    eventSender.mouseUp();

Is there a reason to visit all the intermediate positions? I think you need just two mouseMoveTo()s, and even that is only because the selection start position is determined not on mouse down but on the first mouse move after that (which is arguably a bug). I think mouseMoveTo(x, y + 50); mouseDown(); mouseMoveTo(x, y + 50); mouseMoveTo(x, y); mouseUp(); will work.

I don’t understand why this fix is correct. Just using a non-leaf child (which is not even necessarily a left in the line box tree, it may have nested inline boxes) in this case seems inconsistent. What if there is a line in the middle that has no leaf boxes. What if the first line that has children has no leaf boxes? It seems like we may end up calling positionForBox(null, …) in such cases because we consider lines that have no leaf boxes and then call {first,last,closest}LeafChild on them. Perhaps the right thing to do is in the initial vertical scan loop, change
        if (!root->firstChild())
            continue;
to
        if (!root->firstLeafChild())
            continue;

so that only lines with leaf child boxes are considered. Did you try that?

(In reply to comment #9)
> is not even necessarily a left in the line box tree, it may have nested inline

typo: s/left/leaf/

[Shinichiro Hamaji]

Created attachment 43926 [details]
Patch v3

[Shinichiro Hamaji]

Thanks for your quick review!

> Is there a reason to visit all the intermediate positions? I think you need
> just two mouseMoveTo()s, and even that is only because the selection start
> position is determined not on mouse down but on the first mouse move after that
> (which is arguably a bug). I think mouseMoveTo(x, y + 50); mouseDown();
> mouseMoveTo(x, y + 50); mouseMoveTo(x, y); mouseUp(); will work.

Ah, it works. As this was my first patch around selection and inline
flows, I've just emulated other tests in the same directory after I noticed
mouseMoveTo(x, y+50); mouseDown(); mouseMoveTo(x, y); mouseUp();
doesn't work due to the bug you mentioned.

> I don’t understand why this fix is correct. Just using a non-leaf child (which
> is not even necessarily a left in the line box tree, it may have nested inline
> boxes) in this case seems inconsistent. What if there is a line in the middle
> that has no leaf boxes. What if the first line that has children has no leaf
> boxes? It seems like we may end up calling positionForBox(null, …) in such
> cases because we consider lines that have no leaf boxes and then call
> {first,last,closest}LeafChild on them. Perhaps the right thing to do is in the
> initial vertical scan loop, change
>         if (!root->firstChild())
>             continue;
> to
>         if (!root->firstLeafChild())
>             continue;
> 
> so that only lines with leaf child boxes are considered. Did you try that?

Yeah, you are right. I studied the code of inline box for a
while... it was insufficient :( My understanding about the structure of
inline boxes were completely wrong. Now I think I have a better
understanding thanks to your comment. Thanks!

I'll commit this patch tomorrow after I re-check the patch destroys nothing.

[Shinichiro Hamaji]

Committed as http://trac.webkit.org/changeset/51429