Bug 46761

This is well worth doing. Someone tried this and ran into problems, but I can’t seem to find the bug report with the details.

I tried this and some layout tests ended up differing by one empty text node, so there was some parser state that changed. It would be interesting to instrument the number of layouts, and see if removing this reduces the layout paint on some common pages (and sets of pages, since this causes interactions between tabs).

<rdar://problem/8856934>

I tried this out and I can remove all calls to Document::updateStyleForAllDocuments() except the one in ScriptControllerBase.cpp. Removing that one causes layout test failures. Attached is a test case of code that fails. We get an extra blank line at the end if we don't call updateStyleForAllDocuments. Debugging the code, I see that the extra whitespace node is created at #0 WebCore::Text::Text (this=0x10595f410, document=0x1060a3a00, data=@0x7fff5fbfdc70) at Text.h:48 #1 0x0000000101b4f6dc in WebCore::Text::create (document=0x1060a3a00, data=@0x7fff5fbfdc70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Text.cpp:46 #2 0x000000010136d3f4 in WebCore::HTMLConstructionSite::insertTextNode (this=0x1059601c8, characters=@0x7fff5fbfdc70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLConstructionSite.cpp:326 #3 0x00000001013e882c in WebCore::HTMLTreeBuilder::processCharacterBuffer (this=0x105960190, buffer=@0x7fff5fbfdcd0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2465 #4 0x00000001013e8ed2 in WebCore::HTMLTreeBuilder::processCharacter (this=0x105960190, token=@0x7fff5fbfdd70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2408 #5 0x00000001013e2bea in WebCore::HTMLTreeBuilder::processToken (this=0x105960190, token=@0x7fff5fbfdd70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:480 #6 0x00000001013ec070 in WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken (this=0x105960190, token=@0x7fff5fbfdd70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:452 #7 0x00000001013ec14a in WebCore::HTMLTreeBuilder::constructTreeFromToken (this=0x105960190, rawToken=@0x1060a14b0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:447 #8 0x000000010137231f in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x1060a1400, mode=WebCore::HTMLDocumentParser::AllowYield) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:232 #9 0x0000000101372636 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x1060a1400, mode=WebCore::HTMLDocumentParser::AllowYield) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:169 #10 0x0000000101372ad8 in WebCore::HTMLDocumentParser::append (this=0x1060a1400, source=@0x7fff5fbfded0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:320 #11 0x00000001010efbb4 in WebCore::DecodedDataDocumentParser::appendBytes (this=0x1060a1400, writer=0x1060848a8, data=0x0, length=0, shouldFlush=true) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/DecodedDataDocumentParser.cpp:54 #12 0x0000000101152190 in WebCore::DocumentWriter::addData (this=0x1060848a8, str=0x0, len=0, flush=true) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentWriter.cpp:200 #13 0x0000000101152212 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x1060848a8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentWriter.cpp:220 #14 0x000000010115225b in WebCore::DocumentWriter::end (this=0x1060848a8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentWriter.cpp:206 #15 0x00000001011426b8 in WebCore::DocumentLoader::finishedLoading (this=0x1060ea600) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentLoader.cpp:279 #16 0x00000001012ac961 in WebCore::FrameLoader::finishedLoading (this=0x1060846b8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/FrameLoader.cpp:2175 .... In current code the node has a NULL renderer because of this check in Text::rendererIsNeeded() which returns false. RenderObject *prev = previousRenderer(); if (prev && prev->isBR()) // <span><br/> <br/></span> return false; This works because the renderer for the BR tag is created by the call to updateStyleForAllDocuments(). Here is the stacktrace #0 WebCore::HTMLBRElement::createRenderer (this=0x111509c50, arena=0x1059438b0, style=0x113555a20) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/HTMLBRElement.cpp:79 #1 0x00000001017e94f6 in WebCore::Node::createRendererIfNeeded (this=0x111509c50) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Node.cpp:1381 #2 0x000000010122c893 in WebCore::Element::attach (this=0x111509c50) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:918 #3 0x000000010122c0f0 in WebCore::Element::recalcStyle (this=0x111509c50, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1008 #4 0x000000010122c6dd in WebCore::Element::recalcStyle (this=0x116921350, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1071 #5 0x000000010122c6dd in WebCore::Element::recalcStyle (this=0x105943100, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1071 #6 0x0000000101112ad8 in WebCore::Document::recalcStyle (this=0x1060f5800, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:1599 #7 0x00000001011127ab in WebCore::Document::updateStyleIfNeeded (this=0x1060f5800) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:1641 #8 0x000000010110b461 in WebCore::Document::updateStyleForAllDocuments () at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:1658 #9 0x00000001019eb50c in WebCore::ScriptController::executeScript (this=0x1060ae7b8, sourceCode=@0x7fff5fbfde70, shouldAllowXSS=WebCore::DoNotAllowXSS) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/bindings/ScriptControllerBase.cpp:64 #10 0x00000001019f6c8a in WebCore::ScriptElement::executeScript (this=0x105d00e70, sourceCode=@0x7fff5fbfde70) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/ScriptElement.cpp:214 #11 0x00000001013c5fa4 in WebCore::HTMLScriptRunner::runScript (this=0x11690e7c0, script=0x105d00df0, scriptStartPosition=@0x7fff5fbfdfb0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLScriptRunner.cpp:316 ... In my updated code, if I remove the call to updateStyleForAllDocuments from ScriptControllerBase.cpp, I see my empty node's renderer being created first. At that point, BR element renderer is NULL and thus the optimization in Text::rendererIsNeeded() does not work. The BR node renderer is created later as #0 WebCore::HTMLBRElement::createRenderer (this=0x113d917c0, arena=0x105978140, style=0x113dbc2f0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/HTMLBRElement.cpp:79 #1 0x00000001017e94fe in WebCore::Node::createRendererIfNeeded (this=0x113d917c0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Node.cpp:1381 #2 0x000000010122c89b in WebCore::Element::attach (this=0x113d917c0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:918 #3 0x000000010122c0f8 in WebCore::Element::recalcStyle (this=0x113d917c0, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1008 #4 0x000000010122c6e5 in WebCore::Element::recalcStyle (this=0x113dc3ae0, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1071 #5 0x000000010122c6e5 in WebCore::Element::recalcStyle (this=0x1059da420, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Element.cpp:1071 #6 0x0000000101112ae0 in WebCore::Document::recalcStyle (this=0x1060b1200, change=WebCore::Node::NoChange) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:1599 #7 0x00000001011127b3 in WebCore::Document::updateStyleIfNeeded (this=0x1060b1200) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:1641 #8 0x0000000101110c06 in WebCore::Document::finishedParsing (this=0x1060b1200) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:4258 #9 0x00000001013ec23c in WebCore::HTMLTreeBuilder::finished (this=0x113d475a0) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2805 #10 0x0000000101371fca in WebCore::HTMLDocumentParser::end (this=0x1060bdc00) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:332 #11 0x00000001013720b5 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x1060bdc00) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:341 #12 0x0000000101372cce in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x1060bdc00) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:150 #13 0x0000000101371ef0 in WebCore::HTMLDocumentParser::attemptToEnd (this=0x1060bdc00) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:353 #14 0x0000000101371f28 in WebCore::HTMLDocumentParser::finish (this=0x1060bdc00) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/html/parser/HTMLDocumentParser.cpp:381 #15 0x0000000101109b78 in WebCore::Document::finishParsing (this=0x1060b1200) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/dom/Document.cpp:2290 #16 0x000000010115222e in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x10608daa8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentWriter.cpp:221 #17 0x0000000101152263 in WebCore::DocumentWriter::end (this=0x10608daa8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentWriter.cpp:206 #18 0x00000001011426c0 in WebCore::DocumentLoader::finishedLoading (this=0x106104000) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/DocumentLoader.cpp:279 #19 0x00000001012ac969 in WebCore::FrameLoader::finishedLoading (this=0x10608d8b8) at /Volumes/Data/psolanki/sources/external/WebKit.git/Source/WebCore/loader/FrameLoader.cpp:2175 ....

Adam/Eric: so in the current state, we create an empty text node at parse time, then make a <br> at style resolve time, and merge it with the preceding empty text node. After removing updateStyleForAllDocuments(), we create the text node, and then the <br> first (through style resolve), and fail to merge them. Should we try harder to merge in ::attach()?

Created attachment 79228 [details] Testcase

*** Bug 47831 has been marked as a duplicate of this bug. ***

I would have thought that the two nodes would get lazyAttached, which means neither should have their renderer when you finish running the script. If lazyAttach was being defeated somehow, then yes, you'd make the renderer for the text node first, since the br wouldn't even exist yet. Then having the extra renderer would be expected. So I guess what you should investigate is if lazyAttach is happening for the two nodes, and if it is, why is the text node getting attached before the br?

Here is the DOM tree for the testcase (gdb) call showTree(m_document) *#document 0x106060600 HTML 0x105925a80 HEAD 0x1059235d0 BODY 0x10592f960 #text 0x105931e80 "\n\n" SCRIPT 0x1078806b0 #text 0x1078506c0 "\n\nfunction log(message) {\n document.body.appendChild(document.createTextNode(message));\n document.body.appendChild(document.createElement('br'));\n}\n\nlog('PASS');\n" #text 0x105943e20 "PASS" BR 0x10785a950 #text 0x107880990 "\n" The problem is the last text node (0x107880990). The 'PASS' (0x105943e20) and BR (0x10785a950) nodes do get lazy attached. The last one, however, does not get lazy attached. The code in HTMLConstructionSite::attachAtSite() does 127 // JavaScript run from beforeload (or DOM Mutation or event handlers) 128 // might have removed the child, in which case we should not attach it. 129 if (child->parentNode() && site.parent->attached() && !child->attached()) 130 child->attach(); Just before the attach I see (gdb) fr #0 WebCore::HTMLConstructionSite::attachAtSite (this=0x1059ef888, site=@0x7fff5fbfdbb0, prpChild=@0x7fff5fbfdbc0) at /Volumes/Data/psolanki/sources/external/WebKit/Source/WebCore/html/parser/HTMLConstructionSite.cpp:129 129 if (child->parentNode() && site.parent->attached() && !child->attached()) (gdb) call showTree(child.m_ptr) BODY 0x1059dd250 #text 0x105992a10 "\n\n" SCRIPT 0x108620200 #text 0x10862e0b0 "\n\nfunction log(message) {\n document.body.appendChild(document.createTextNode(message));\n document.body.appendChild(document.createElement('br'));\n}\n\nlog('PASS');\n" #text 0x1059e7470 "PASS" BR 0x105d020a0 * #text 0x10860fdc0 "\n" So we are calling attach() on this textnode while the previous node (BR) is in lazy attach.

There was a effort to move everything to using lazyAttach last summer... but then it stalled. I can't remember why exactly. There were performance concerns. I'd have to look back through email threads, or perhaps others (jamesr, mjs, etc.) remember. (That explains why we see a lot of places using lazyAttach() but many still using attach().)

(In reply to comment #10) > There was a effort to move everything to using lazyAttach last summer... but then it stalled. I can't remember why exactly. There were performance concerns. I'd have to look back through email threads, or perhaps others (jamesr, mjs, etc.) remember. I found bug 47316. Looks like it was a perf loss.

I thought making the HTML parser use lazyAttach() (bug 47316) would fix my issue. It didn't. This is because we end up with the following (partial) dom tree * #text 0x1059da2c0 "PASS" BR 0x1059c2c90 #text 0x113ec25f0 "\n\n" At this point all 3 nodes are attached via lazyAttach() but none have a renderer. We start by calculating style for the PASS element and we create its renderer. And then we come to Node::attach(). The loop in Node::attach() looks at the nodes siblings and decides to create the renderer for the empty text node. And since the BR node has not been created (i.e. its renderer is NULL), the optimization in Text::rendererIsNeeded() eludes us again. The order for creating renderers is - PASS text node - empty text node - BR node

(In reply to comment #12) > I thought making the HTML parser use lazyAttach() (bug 47316) would fix my issue. It didn't. This is because we end up with the following (partial) dom tree > > * #text 0x1059da2c0 "PASS" > BR 0x1059c2c90 > #text 0x113ec25f0 "\n\n" > > At this point all 3 nodes are attached via lazyAttach() but none have a renderer. We start by calculating style for the PASS element and we create its renderer. And then we come to Node::attach(). The loop in Node::attach() looks at the nodes siblings and decides to create the renderer for the empty text node. And since the BR node has not been created (i.e. its renderer is NULL), the optimization in Text::rendererIsNeeded() eludes us again. The order for creating renderers is > > - PASS text node > - empty text node > - BR node It seems like the optimization in Text::rendererIsNeeded() is wrong and is thus being defeated here. One "fix" would be to make that check require that style is resolved first (I'm not sure that's safe or a good idea though). But I'm wondering why the check is needed in the first place. Have we tested to see what Firefox's behavior here is? Do they create an empty renderer? I'm not sure this "regression" matters much in practice. If this is the worse consequence of removing updateStyleForAllDocuments() I would just remove it. :) However I also support further investigation.

(In reply to comment #13) > Have we tested to see what Firefox's behavior here is? Do they create an empty renderer? I mean "empty text node" of course.

*** Bug 76679 has been marked as a duplicate of this bug. ***

(In reply to comment #13) > Have we tested to see what Firefox's behavior here is? Do they create an empty renderer? > > I'm not sure this "regression" matters much in practice. If this is the worse consequence of removing updateStyleForAllDocuments() I would just remove it. :) However I also support further investigation. The actual DOM is the same here with or without this patch. For example on LayoutTests/fast/canvas/resize-while-save-active.html the last element of document.body.childNodes is a #text node containing three linebreaks. This is the same as Firefox. I don't think this renderobject has any impact on the actual rendering of the page or on any APIs exposed to authors. It might have a small perf impact, but certainly the impact is a whole lot smaller than synchronously updating styles on all documents all the time! Here's a list of all tests that show this issue on a ToT chromium build: editing/selection/caret-ltr-2-left.html editing/selection/caret-ltr-2.html editing/selection/caret-ltr-right.html editing/selection/caret-ltr.html editing/selection/caret-rtl-2-left.html editing/selection/caret-rtl-2.html editing/selection/caret-rtl-right.html editing/selection/caret-rtl.html fast/canvas/resize-while-save-active.html fast/dom/title-directionality.html fast/xsl/import-non-document-node.xhtml http/tests/cache/subresource-expiration-1.html http/tests/cache/subresource-expiration-2.html http/tests/xmlhttprequest/send-array-buffer.html http/tests/xmlhttprequest/send-undefined-and-null.html I think we should move forward with this, rebaseline those 15 tests, and then if anyone feels motivated to optimize this so there's no 0x0 RenderText node they can feel free.

Created attachment 123240 [details] Patch

Comment on attachment 123240 [details] Patch Can we remove updateStyleForAllDocuments() itself?

Comment on attachment 123240 [details] Patch Attachment 123240 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11299288 New failing tests: fast/multicol/span/span-as-immediate-columns-child-removal.html fast/multicol/span/span-as-immediate-child-property-removal.html

(In reply to comment #18) > (From update of attachment 123240 [details]) > Can we remove updateStyleForAllDocuments() itself? There are 4 remaining callers: *) Source/WebCore/bindings/js/JSCallbackData.cpp invokeCallback() *) Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp lookupNamespaceURI() These do not have direct equivalents in the V8 bindings and I'm not sure exactly what they are for, so I'd prefer that they be removed in a separate patch by someone with a rough understanding of when these are called. I'm pretty sure the callers here were just copied from other similar callsites. *) Source/WebCore/dom/ContainerNode.cpp setActive() synchronous repaint This is the synchronous paint-and-sleep logic. It's also the only caller to RenderObject::repaint(true). I've wanted to delete this section for a while, especially since it doesn't work at all in WebKit2 or Chromium, but other people want to preserve the WK1 behavior here. I think this one needs to stay, since this code bypasses the deferred style recalc/layout/paint loop. *) Source/WebCore/svg/animation/SMILTimeContainer.cpp updateAnimations() I haven't investigated this one at all, nor do I have much of an idea how SMIL animations are supposed to work. I think they are fairly rare so this one has a smaller impact on performance overall. I'll file bugs on these callsites individually and investigate the multicol failure before landing. Thank you for the prompt review.

(In reply to comment #19) > (From update of attachment 123240 [details]) > Attachment 123240 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11299288 > > New failing tests: > fast/multicol/span/span-as-immediate-columns-child-removal.html > fast/multicol/span/span-as-immediate-child-property-removal.html These are real, bug I think we're just exposing a bug in deferred style calculations within multicol. If I add "document.body.offsetTop" to strategic locations in the test HTML files, I get a different output. I'll file a separate bug for this.

Committed r106043: <http://trac.webkit.org/changeset/106043>

Landed with new expectations for the multicol tests, since they aren't regressions (the style bugs with multicol have existed for some time). Will keep a close eye out for unexpected side effects.