Node::createRenderer should never return null
Created attachment 175507 [details] Patch
Comment on attachment 175507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:234 > + ASSERT(newRenderer); There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: if (!newRenderer) { ASSERT_NOT_REACHED(); return; }
(In reply to comment #2) > (From update of attachment 175507 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review > > > Source/WebCore/dom/NodeRenderingContext.cpp:234 > > + ASSERT(newRenderer); > > There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: > if (!newRenderer) { > ASSERT_NOT_REACHED(); > return; > } The return value is required for the code to compile, we could return anything there. If you return true from rendererIsNeeded and then return 0 from createRenderer the program should crash in a release build. I don't think it makes sense to protect against this case. (Similarly returning null from styleForRenderer() is a fatal error and we assert about it right above this, but we don't protect you from developer error of doing that)
(In reply to comment #3) > ... Btw, I don't feel very strongly about this, I can add the protection if you want. :)
Created attachment 175517 [details] Patch Add return after assert
Comment on attachment 175507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175507&action=review >>> Source/WebCore/dom/NodeRenderingContext.cpp:234 >>> + ASSERT(newRenderer); >> >> There are some codepaths that ASSERT_NOT_REACHED and then return null. If this isn't actually a performance improvement, it might be better to be a bit more defensive here. How about: >> if (!newRenderer) { >> ASSERT_NOT_REACHED(); >> return; >> } > > The return value is required for the code to compile, we could return anything there. If you return true from rendererIsNeeded and then return 0 from createRenderer the program should crash in a release build. I don't think it makes sense to protect against this case. > > (Similarly returning null from styleForRenderer() is a fatal error and we assert about it right above this, but we don't protect you from developer error of doing that) Yeah. I'm on the fence.
Comment on attachment 175517 [details] Patch Clearing flags on attachment: 175517 Committed r135437: <http://trac.webkit.org/changeset/135437>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 102986
So it turns out there's a bug my CL uncovered: Node::rendererIsNeeded has: (document()->documentElement() == this) || (context.style()->display() != NONE); Element::createRenderer has a special case for this: // Ignore display: none on root elements. Force a display of block in that case. if (document()->documentElement() == this && style->display() == NONE) { RenderBlock* result = new (arena) RenderBlock(this); ... } but then in HTMLElement::createRenderer we don't call super: return RenderObject::createObject(this, style); so in an HTML document display: none on the root element is NOT ignored! This was introduced way back in https://trac.webkit.org/changeset/21405 when adding a special case for <wbr> elements in HTMLElement::createRenderer.
(In reply to comment #10) > ... > so in an HTML document display: none on the root element is NOT ignored! > I fixed the bug with the documentElement in r136331, but having dug deeper I think it'd be a time consuming task to make createRenderer never return null as in this original bug, and I'm not sure it has a lot of benefits since we'd still need the if statement check in NodeRenderingContext.