Bug 66287

Summary: Element without renderer as documentElement triggers NULL ptr
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: annevk, ap, eric, Ms2ger, withlhw
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Repro
none
Patch
none
Patch for review ap: review-

Berend-Jan Wever
Reported 2011-08-16 03:23:15 PDT
Created attachment 104018 [details] Repro <body onload="go()"> <script> function go() { var oHead = document.documentElement.firstChild; document.open(); document.insertBefore(oHead, null); document.writeln("<style>* {border-top: solid 1px;}</style><x>"); } </script> </body> Any element can be the documentElement, even an element without a renderer such as HEAD. Not all code handles this correctly, eg: void RenderBox::paintBackground(const PaintInfo& paintInfo, const LayoutRect& paintRect, BackgroundBleedAvoidance bleedAvoidance) { if (isRoot()) paintRootBoxFillLayers(paintInfo); else if (!isBody() || document()->documentElement()->renderer()->hasBackground()) { // The <body> only paints its background if the root element has defined a background // independent of the body. if (!backgroundIsObscured()) paintFillLayers(paintInfo, style()->visitedDependentColor(CSSPropertyBackgroundColor), style()->backgroundLayers(), paintRect, bleedAvoidance); } } In the above code "document()->documentElement()->renderer()->..." triggers a NULL ptr if there is no renderer. id: chrome.dll!WebCore::RenderBox::paintBackground ReadAV@NULL (10c48e8cc519a140dff53e68ca553272) description: Attempt to read from unallocated NULL pointer+0x4 in chrome.dll!WebCore::RenderBox::paintBackground stack: chrome.dll!WebCore::RenderBox::paintBackground 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!WebKit::WebViewImpl::paint chrome.dll!RenderWidget::PaintRect ...
Attachments
Repro (253 bytes, text/html)
2011-08-16 03:23 PDT, Berend-Jan Wever 		no flags
Details
Patch (4.05 KB, patch)
2011-08-19 02:16 PDT, Hyungwook Lee 		no flags
DetailsFormatted DiffDiff
Patch for review (4.33 KB, patch)
2011-08-22 04:09 PDT, Hyungwook Lee 		ap: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Berend-Jan Wever
Comment 1 2011-08-16 03:24:16 PDT
Chromium: https://code.google.com/p/chromium/issues/detail?id=93029
Alexey Proskuryakov
Comment 3 2011-08-16 10:31:28 PDT
> Any element can be the documentElement, even an element without a renderer such as HEAD. This doesn't sound right. We should probably make sure that document element is of an expected type.
Hyungwook Lee
Comment 4 2011-08-17 04:31:53 PDT
I think <HEAD> element can not be documentElement in this case. According to DOM2 Core spec, in insertBefore() part mentioned as follows. If refChild is null, insert newChild at the end of the list of children. It means that when we call document.insertBefore(oHead, null); <HEAD> will be inserted at the end of tree not first child position. And documentElement can be only first child element. Many places in WebKit codes assumes that document element has its own renderer. Hence I think we can't allow to be an element which does not has its own renderer. I'm trying to find why we set <HEAD> element in documentElement.
Hyungwook Lee
Comment 5 2011-08-19 01:45:23 PDT
The WebKit browser automatically put <html>, <head>, <body> element in HTML tree builder. But in this case, We remove all childs such as <html> by "document.open();" DOM API after page loads. When we call "document.insertBefore(oHead, null);", the document doens't has any childs. Hence, <head> element be inserted into the first child position of the document and then it gonna be document element which doens't not has its own renderer.
Hyungwook Lee
Comment 6 2011-08-19 02:16:13 PDT
Created attachment 104480 [details] Patch
Hyungwook Lee
Comment 7 2011-08-22 04:09:24 PDT
Created attachment 104661 [details] Patch for review
Alexey Proskuryakov
Comment 8 2011-08-22 08:48:13 PDT
Comment on attachment 104661 [details] Patch for review You should set r? flag, not r+. This patch needs a regression test. Also, even though Firefox also lets HEAD be the document element, I'm still not quite sure if this is the way to go. What does IE do?
Hyungwook Lee
Comment 9 2011-08-22 20:30:02 PDT
Firefox and WebKit allow HEAD be the document element. In case of Opera and IE, It seems not allow HEAD be the document element. Actually, Opera and IE is not properly works well. According DOM 2 Core spec, I think HEAD element can be document element through document.insertBefore() DOM API. Because DOM API do not care its element name and all of element can be document element. But several browser do not allow HEAD to be document element. I was thing what can be solution for this. 1. Let HEAD to be document element and modify WebKit codes to prevent direclty access its renderer which same as my patches. 2.Do not allow to add into the DOM tree which do not has its renderer until there is no html element in the DOM tree via DOM API. 3. Add html element in the DOM tree automaticlaly before add HEAD element same as HTMLTreeBuilder. I think #1(FireFox) and #2(Opera / IE) can be solution and #3 is not good way. My personal opinion is that #1 is more better way to meet DOM spec than others.
Alexey Proskuryakov
Comment 10 2011-08-22 22:08:52 PDT
CC'ing Web DOM Core editors for input.
Ms2ger (he/him; ⌚ UTC+1/+2)
Comment 11 2011-08-23 02:21:10 PDT
A display:none documentElement should definitely be supported, and, preferably, shouldn't crash.
Hyungwook Lee
Comment 12 2011-09-06 21:11:09 PDT
I think there is no regression within the patches. Becuase I just put guard code to prevent the crash. I'm not able to run "run-webkit-tests" scripts successfully in my system (Cygwin on Windows XP). It shows many fails even if I didn't changes anythings. of cause I set WEBKIT_TESTFONTS for make sure for the renderer tree.
Anne van Kesteren
Comment 13 2023-12-30 02:49:13 PST
This seems to work fine these days.
Note You need to log in before you can comment on or make changes to this bug.