Bug 51647

Summary:

chrome.dll!WebCore::RenderBox::paintBoxDecorationsWithSize ReadAV@NULL (214b527fa4dab86d8d344b0220263689)

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

abarth, dglazkov, eae, eric, tonyg, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

Attachments:

Description	Flags
Repro	none
Patch	none

Berend-Jan Wever

Reported 2010-12-27 07:47:38 PST

Created attachment 77508 [details] Repro http://code.google.com/p/chromium/issues/detail?id=68094 Repro: <script> function go() { oHTMLElement = document.documentElement.firstChild; document.open(); document.appendChild(oHTMLElement) document.writeln("<object>" + "<style>" + "@page {border-right: thick;}" + "* {border-right: medium dashed;}" + "</style>") location.reload(); } </script> <body onload="go()"> id: chrome.dll!WebCore::RenderBox::paintBoxDecorationsWithSize ReadAV@NULL (214b527fa4dab86d8d344b0220263689) description: Attempt to read from unallocated NULL pointer+0x4 in chrome.dll!WebCore::RenderBox::paintBoxDecorationsWithSize application: Chromium 10.0.623.0 stack: chrome.dll!WebCore::RenderBox::paintBoxDecorationsWithSize chrome.dll!WebCore::RenderBox::paintBoxDecorations chrome.dll!WebCore::RenderBlock::paintObject chrome.dll!WebCore::RenderBlock::paint chrome.dll!WebCore::RenderBlock::paintChildren chrome.dll!WebCore::RenderBlock::paintContents chrome.dll!WebCore::RenderBlock::paintObject chrome.dll!WebCore::RenderBlock::paint chrome.dll!WebCore::RenderBlock::paintChildren chrome.dll!WebCore::RenderBlock::paintContents chrome.dll!WebCore::RenderBlock::paintObject chrome.dll!WebCore::RenderLayer::paintLayer chrome.dll!WebCore::RenderLayer::paint chrome.dll!WebCore::FrameView::paintContents chrome.dll!WebCore::ScrollView::paint chrome.dll!WebKit::WebFrameImpl::paintWithContext chrome.dll!WebKit::WebFrameImpl::paint chrome.dll!RenderWidget::PaintRect chrome.dll!RenderWidget::DoDeferredUpdate chrome.dll!RenderWidget::CallDoDeferredUpdate chrome.dll!MessageLoop::RunTask chrome.dll!MessageLoop::DoWork chrome.dll!base::MessagePumpDefault::Run chrome.dll!MessageLoop::RunInternal ...

Attachments
Repro (347 bytes, text/html) 2010-12-27 07:47 PST, Berend-Jan Wever	no flags	Details
Patch (4.30 KB, patch) 2011-01-07 15:08 PST, Emil A Eklund	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2010-12-27 10:06:41 PST

We'll have to catch this in the debugger to find out which line is actually crashing.

Berend-Jan Wever

Comment 2 2010-12-28 06:58:25 PST

http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderBox.cpp#L814 797 void RenderBox::paintBoxDecorationsWithSize(PaintInfo& paintInfo, int tx, int ty, int width, int height) 798 { 799 // border-fit can adjust where we paint our border and background. If set, we snugly fit our line box descendants. (The iChat 800 // balloon layout is an example of this). 801 borderFitAdjust(tx, width); 802 803 // FIXME: Should eventually give the theme control over whether the box shadow should paint, since controls could have 804 // custom shadows of their own. 805 paintBoxShadow(paintInfo.context, tx, ty, width, height, style(), Normal); 806 807 // If we have a native theme appearance, paint that before painting our background. 808 // The theme will tell us whether or not we should also paint the CSS background. 809 bool themePainted = style()->hasAppearance() && !theme()->paint(this, paintInfo, IntRect(tx, ty, width, height)); 810 if (!themePainted) { 811 // The <body> only paints its background if the root element has defined a background 812 // independent of the body. Go through the DOM to get to the root element's render object, 813 // since the root could be inline and wrapped in an anonymous block. 814 if (!isBody() || document()->documentElement()->renderer()->hasBackground()) 815 paintFillLayers(paintInfo, style()->visitedDependentColor(CSSPropertyBackgroundColor), style()->backgroundLayers(), tx, ty, width, height); 816 if (style()->hasAppearance()) 817 theme()->paintDecorations(this, paintInfo, IntRect(tx, ty, width, height)); 818 } 819 paintBoxShadow(paintInfo.context, tx, ty, width, height, style(), Inset); 820 821 // The theme will tell us whether or not we should also paint the CSS border. 822 if ((!style()->hasAppearance() || (!themePainted && theme()->paintBorderOnly(this, paintInfo, IntRect(tx, ty, width, height)))) && style()->hasBorder()) 823 paintBorder(paintInfo.context, tx, ty, width, height, style()); 824 } "document()->documentElement()->renderer()" returns NULL, so "(NULL)->hasBackground()" causes a NULL deref. chrome_639d0000!WebCore::RenderBox::paintBoxDecorationsWithSize+0x9a [c:\b\build\slave\win\build\src\third_party\webkit\webcore\rendering\renderbox.cpp @ 814]: 6431292a 3400 xor al,0 6431292c e8af6ff8ff call chrome_639d0000!WebCore::RenderObject::isBody (642998e0) 64312931 84c0 test al,al 64312933 7431 je chrome_639d0000!WebCore::RenderBox::paintBoxDecorationsWithSize+0xd6 (64312966) 64312935 8b4e08 mov ecx,dword ptr [esi+8] 64312938 8b4914 mov ecx,dword ptr [ecx+14h] 6431293b 83b98c02000000 cmp dword ptr [ecx+28Ch],0 64312942 894c242c mov dword ptr [esp+2Ch],ecx 64312946 7509 jne chrome_639d0000!WebCore::RenderBox::paintBoxDecorationsWithSize+0xc1 (64312951) 64312948 e833acf3ff call chrome_639d0000!WebCore::Document::cacheDocumentElement (6424d580) 6431294d 8b4c242c mov ecx,dword ptr [esp+2Ch] 64312951 8b918c020000 mov edx,dword ptr [ecx+28Ch] 64312957 8b4220 mov eax,dword ptr [edx+20h] chrome_639d0000!WebCore::RenderBox::paintBoxDecorationsWithSize+0xca [c:\b\build\slave\win\build\src\third_party\webkit\webcore\rendering\renderbox.cpp @ 814]: eax=00000000 ebx=0037e9c8 ecx=0187e900 edx=0412fec0 esi=00ab82f8 edi=00000299 eip=6431295a esp=0037e8f4 ebp=00000008 6431295a 8b4804 mov ecx,dword ptr [eax+4] 6431295d e80e8efbff call chrome_639d0000!WebCore::RenderStyle::hasBackground (642cb770) 64312962 84c0 test al,al 64312964 7432 je chrome_639d0000!WebCore::RenderBox::paintBoxDecorationsWithSize+0x108 (64312998) 64312966 8b542430 mov edx,dword ptr [esp+30h] 6431296a 8b4e04 mov ecx,dword ptr [esi+4] 6431296d 8b4114 mov eax,dword ptr [ecx+14h] 64312970 6a00 push 0 64312972 6a02 push 2 64312974 57 push edi 64312975 52 push edx 64312976 8b542438 mov edx,dword ptr [esp+38h] 6431297a 55 push ebp 6431297b 52 push edx 6431297c 83c004 add eax,4 6431297f 50 push eax 64312980 68fb030000 push 3FBh 64312985 8d442430 lea eax,[esp+30h] 64312989 50 push eax

Emil A Eklund

Comment 3 2011-01-07 15:08:15 PST

Created attachment 78279 [details] Patch I tried to make do without adding a new field but couldn't find any way to detect the state properly using existing data.

WebKit Review Bot

Comment 4 2011-01-07 15:39:17 PST

Attachment 78279 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7404036

Adam Barth

Comment 5 2011-01-07 15:42:03 PST

Comment on attachment 78279 [details] Patch Hum... This doesn't seem right. Why doesn't the spec need to track this bool?

Adam Barth

Comment 6 2011-01-07 16:36:06 PST

Generally, we should be robust to document.writing and adding elements via the DOM in arbitrary orders. I don't quite understand why it's crashing.

Emil A Eklund

Comment 7 2011-01-07 16:37:33 PST

Document::write assumes the document is open for writing if the parser has an insert point. Appending an element after calling document.open does not reset the insert point thus the assumption is incorrect when the document.write call is expected. Another way around this would possibly be to reset the insert point (either directly or by detaching the parser) when append is called.

Adam Barth

Comment 8 2011-01-07 17:06:36 PST

> Document::write assumes the document is open for writing if the parser has an insert point. That should be a correct assumption. > Appending an element after calling document.open does not reset the insert point thus the assumption is incorrect when the document.write call is expected. I'm not sure I understand. Why should appending an element to the DOM affect the insertion point? > Another way around this would possibly be to reset the insert point (either directly or by detaching the parser) when append is called. The insertion point has to do with the front-end to the HTML parser. The DOM is the backend output of the parser. The parser should know how to operate on a DOM that's been manipulated by means other than the parser...

Emil A Eklund

Comment 9 2011-01-07 17:12:57 PST

You are right, the proposed change clearly isn't the right way to do this. I'll rethink the approach, thanks for the pointers.

Emil A Eklund

Comment 10 2011-01-10 13:01:48 PST

The problem seems to be that document parser is unaware of the html element being appended and as such it's still in the InitialMode InsertionMode. It then calls Document::setCompatibilityMode which fails the ASSERT(!documentElement()) assertion as it already has a document element. IE and Mozilla throws a HIERARCHY_REQUEST_ERR exception for the appendChild call and IE throws a invalid argument. Perhaps that's how we should handle it too?

Berend-Jan Wever

Comment 11 2011-12-16 05:57:21 PST

This has been fixed in the duplicate bug. *** This bug has been marked as a duplicate of bug 64284 ***

Note You need to log in before you can comment on or make changes to this bug.