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-

Description Berend-Jan Wever 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
                ...
Comment 1 Berend-Jan Wever 2011-08-16 03:24:16 PDT
Chromium: https://code.google.com/p/chromium/issues/detail?id=93029
Comment 3 Alexey Proskuryakov 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.
Comment 4 Hyungwook Lee 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.
Comment 5 Hyungwook Lee 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.
Comment 6 Hyungwook Lee 2011-08-19 02:16:13 PDT
Created attachment 104480 [details]
Patch
Comment 7 Hyungwook Lee 2011-08-22 04:09:24 PDT
Created attachment 104661 [details]
Patch for review
Comment 8 Alexey Proskuryakov 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?
Comment 9 Hyungwook Lee 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.
Comment 10 Alexey Proskuryakov 2011-08-22 22:08:52 PDT
CC'ing Web DOM Core editors for input.
Comment 11 Ms2ger (he/him; ⌚ UTC+1/+2) 2011-08-23 02:21:10 PDT
A display:none documentElement should definitely be supported, and, preferably, shouldn't crash.
Comment 12 Hyungwook Lee 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.
Comment 13 Anne van Kesteren 2023-12-30 02:49:13 PST
This seems to work fine these days.