Bug 46761

Summary: Remove calls to Document::updateStyleForAllDocuments()
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, dglazkov, eric, hyatt, jamesr, japhet, mihaip, pfeldman, psolanki, simon.fraser, tonikitoo, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
Testcase
none
Patch simon.fraser: review+, webkit.review.bot: commit-queue-

Description Simon Fraser (smfr) 2010-09-28 15:59:44 PDT 
We have calls to Document::updateStyleForAllDocuments() sprinkled around, after script execution, timers etc. Now that we have timer-based style resolution and layout, we should never need to update style on a single document at these places, let alone all documents.
Comment 1 Darin Adler 2010-09-28 16:17:29 PDT 
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.
Comment 2 Simon Fraser (smfr) 2011-01-12 15:39:06 PST 
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).
Comment 3 Pratik Solanki 2011-01-12 16:50:17 PST 
<rdar://problem/8856934>
Comment 4 Pratik Solanki 2011-01-17 16:15:23 PST 
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
....
Comment 5 Simon Fraser (smfr) 2011-01-17 16:21:11 PST 
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()?
Comment 6 Pratik Solanki 2011-01-17 16:24:34 PST 
Created attachment 79228 [details]
Testcase
Comment 7 Pratik Solanki 2011-01-17 21:15:07 PST 
*** Bug 47831 has been marked as a duplicate of this bug. ***
Comment 8 Dave Hyatt 2011-01-18 11:43:40 PST 
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?
Comment 9 Pratik Solanki 2011-01-18 12:33:27 PST 
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.
Comment 10 Eric Seidel (no email) 2011-01-18 12:39:01 PST 
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().)
Comment 11 Pratik Solanki 2011-01-18 12:54:50 PST 
(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.
Comment 12 Pratik Solanki 2011-01-18 17:53:08 PST 
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
Comment 13 Eric Seidel (no email) 2011-01-18 18:21:35 PST 
(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.
Comment 14 Eric Seidel (no email) 2011-01-18 18:22:16 PST 
(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.
Comment 15 James Robinson 2012-01-19 18:27:04 PST 
*** Bug 76679 has been marked as a duplicate of this bug. ***
Comment 16 James Robinson 2012-01-19 18:51:34 PST 
(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.
Comment 17 James Robinson 2012-01-19 19:02:00 PST 
Created attachment 123240 [details]
Patch
Comment 18 Simon Fraser (smfr) 2012-01-19 20:01:42 PST 
Comment on attachment 123240 [details]
Patch

Can we remove updateStyleForAllDocuments() itself?
Comment 19 WebKit Review Bot 2012-01-19 21:30:05 PST 
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
Comment 20 James Robinson 2012-01-20 11:07:09 PST 
(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.
Comment 21 James Robinson 2012-01-20 12:03:15 PST 
(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.
Comment 22 James Robinson 2012-01-26 14:24:24 PST 
Committed r106043: <http://trac.webkit.org/changeset/106043>
Comment 23 James Robinson 2012-01-26 14:25:02 PST 
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.