REGRESSION (r35318): A press release at pfizer.com does not display correctly

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jim Oase]

I am using build  r36519 on a MBP 17"

I accessed this page  
http://pfizer.com/news/press_releases/pfizer_press_releases.jsp?rssUrl=http://mediaroom.pfizer.com/portal/site/pfizer/index.jsp?ndmViewId=news_view&ndmConfigId=1010794&newsId=20080225006247&newsLang=en
and it does not display correctly.... see attachment.   Safari works OK

[Jim Oase]

Created attachment 23518 [details]
Screen shot of displayed page

[Alexey Proskuryakov]

Confirmed with r36292.

[Cameron Zwarich (cpst)]

This regresses between the r35289 and r35359 nightlies. I'll bisect with local builds to find the exact revision.

[Cameron Zwarich (cpst)]

This regressed in r35318:

http://trac.webkit.org/changeset/35318

I'll try to make a reduction.

[Jim Oase]

This site also displays incorrectly in the same manner   http://www.wired.com/

[Jim Oase]

Here is another site with this display problem
http://blog.wired.com/defense/2009/03/us-jet-shoots-d.html

[Beth Dakin]

With my build of tip of tree WebKit, I can reproduce a problem at pfizer, but not at wired.com

[Beth Dakin]

<rdar://problem/6680073>

[Cameron Zwarich (cpst)]

Same here, Beth. I can't reproduce the problem at Wired. However, when I make a local reduction of the Pfizer page, the appearance of the bug becomes fairly random, differing depending on reload. I guess there is either a timing issue or a read from some uninitialized or freed memory. This makes it difficult to construct a reduction.

The Pfizer problem appears with only the page in the iframe, at least when it is loaded over the network:

http://mediaroom.pfizer.com/portal/site/pfizer/index.jsp?ndmViewId=news_view&ndmConfigId=1010794&newsId=20080225006247&newsLang=en

[Beth Dakin]

Hey Cameron, even if your reduction only reproduces intermittently, it could still be helpful. You should post whatever you have! Even if it's not that small yet.

[Beth Dakin]

This is the part of the change that causes the regression. This code now lives in RenderObjectChildList::updateBeforeAfterContent(). It's also critical to the original crash fix.

@@ -357,15 +373,11 @@
                 // to find the original content properly.
                 generatedContentContainer = RenderObject::createObject(document(), pseudoElementStyle);
                 generatedContentContainer->setStyle(pseudoElementStyle);
+                addChild(generatedContentContainer, insertBefore);
             }
             generatedContentContainer->addChild(renderer);
         }
     }
-
-    // Add the pseudo after we've installed all our content so that addChild will be able to find the text
-    // inside the inline for e.g., first-letter styling.
-    if (generatedContentContainer)
-        addChild(generatedContentContainer, insertBefore);

[Cameron Zwarich (cpst)]

Created attachment 28597 [details]
Partial reduction

Here is a partial reduction. For the reduction to work locally, I need to keep hitting command-R to reload and then stop at the right time. It's hard to describe. Including the style sheet into a <style> tag seems to make it not happen, and the same with removing the <script> tags for files that don't exist.

I am sure this can be reduced further, but I can't make it fail reliably past this point with a local debug build of ToT.

[Beth Dakin]

Thanks, Cameron!

[Cameron Zwarich (cpst)]

My reduction seems to work 100% on bugs.webkit.org, although it might not from Apple. I had trouble getting it to work with a closer and less laggy HTTP server. I might make a new bug and use consecutive attachments to reduce it further (while trying not to spam both of you).

[Beth Dakin]

Looks like this is the style declaration that is taking us down the before-after codepath:

#releaseMain:after
 {
    content:".";
    display:block;
    height:0;
    clear:both;
    visibility:hidden;
}

[Jim Oase]

Created attachment 28617 [details]
Screen shot 1 Wired.com

[Jim Oase]

Created attachment 28618 [details]
Screen shot 2....... wired blog

[Cameron Zwarich (cpst)]

Created attachment 28629 [details]
Reduction

Here's my best reduction. It reproduces locally. It seems to depend on the following things:

1) The :after style for the <div>
2) The <div> being in a table.
3) The inline <script> element for a network resource (in this case, a 404 on webkit.org). The <script> element has to be in the same table cell as the <div> and before it.

[Cameron Zwarich (cpst)]

I'll unassign this because I will probably not be able to fix this.

[Beth Dakin]

Okay, I think I have a fix for this. I will post it for review soon, but first I need to turn Cameron's awesome reduction into a layout test. I need to try to avoid including the remote script because we don't like to have such things in layout tests. I could make it into an http test, and I might have to. But I am going to explore other options first.

[Cameron Zwarich (cpst)]

The HTTP layout test works if you use misc/resources/script-slow1.pl, which waits a second. Using the Time::HiRes module with a quarter second delay will also work, but I can't go much lower than that while still reproducing the bug. Hopefully you can find a better way to test it.

[Beth Dakin]

Doh! It looks like my patch fixes the reduction, but not the Pfizer page. Grr.

[Beth Dakin]

Created attachment 28764 [details]
Patch

Okay, here is a new patch that works for the reduction AND the original Pfizer page. I am not sure it is the best way to fix the problem. Here is a summary of the problem:

When we add the generated content to the owner, the owner itself is not yet attached to the document…the call to RenderObjectChildList::updateBeforeAfterContent() is propagated through Node::createRendererIfNeeded(), and it is called before the new renderer (for the "owner") is actually added to the document. So when we add the generated content and the generated content container as children of the owner, we call setNeedsLayoutAndPrefWidthsRecalc(). This call propagates up the container hierarchy. However, since the owner has not been added to the document just yet, the hierarchy is not very long. 

So then, after all of that is done, the owner is added to the document back in Node::createRendererIfNeeded(). So again, setNeedsLayoutAndPrefWidthsRecalc() is called from within  the addChild() call. However, it doesn't end up propagating up the hierarchy this time either because of this optimization in RenderObject::setPrefWidthsDirty():

if (b && !alreadyDirty && markParents && (isText() || (style()->position() != FixedPosition && style()->position() != AbsolutePosition)))
        invalidateContainerPrefWidths();

alreadyDirty is true because this function was already called for the owner of the generated content back when the generated content was added.

What I don't understand is why this bug only gets exposed in this weird timing situation. In other words, I don't know how we ever get it right!

All of that being said, I could imagine this bug being fixed in a number of other places, and I would love some feedback on moving or optimizing this fix.

[Dave Hyatt]

Comment on attachment 28764 [details]
Patch

This should have been done automatically in the rendering code.  I don't think this is right, and think you need to investigate further.

[Dave Hyatt]

What about just deliberately not dirtying the outermost object when setNeedsLayout and setPrefWidthsDirty are called on unrooted subtrees?  I think that might be a general solution to this problem.

[Beth Dakin]

Created attachment 28767 [details]
New Patch

Here is a patch implementing Hyatt's idea.

[Beth Dakin]

Created attachment 28772 [details]
Newer Patch

This one!

[Dave Hyatt]

Comment on attachment 28772 [details]
Newer Patch

r=me

[Beth Dakin]

Fixed with r41865.