Bug 20909 - REGRESSION (r35318): A press release at pfizer.com does not display correctly
Summary: REGRESSION (r35318): A press release at pfizer.com does not display correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Beth Dakin
URL: http://pfizer.com/news/press_releases...
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-09-17 19:46 PDT by Jim Oase
Modified: 2009-03-20 12:57 PDT (History)
2 users (show)

See Also:


Attachments
Screen shot of displayed page (266.91 KB, image/jpeg)
2008-09-17 19:47 PDT, Jim Oase
no flags Details
Partial reduction (756 bytes, text/html)
2009-03-13 14:00 PDT, Cameron Zwarich (cpst)
no flags Details
Screen shot 1 Wired.com (172.35 KB, image/jpeg)
2009-03-14 00:47 PDT, Jim Oase
no flags Details
Screen shot 2....... wired blog (151.75 KB, image/jpeg)
2009-03-14 00:47 PDT, Jim Oase
no flags Details
Reduction (349 bytes, text/html)
2009-03-14 15:04 PDT, Cameron Zwarich (cpst)
no flags Details
Patch (23.51 KB, patch)
2009-03-19 14:48 PDT, Beth Dakin
hyatt: review-
Details | Formatted Diff | Diff
New Patch (24.23 KB, patch)
2009-03-19 15:43 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Newer Patch (25.50 KB, patch)
2009-03-19 17:02 PDT, Beth Dakin
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jim Oase 2008-09-17 19:46:56 PDT
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
Comment 1 Jim Oase 2008-09-17 19:47:46 PDT
Created attachment 23518 [details]
Screen shot of displayed page
Comment 2 Alexey Proskuryakov 2008-09-18 01:30:18 PDT
Confirmed with r36292.
Comment 3 Cameron Zwarich (cpst) 2009-03-12 02:12:27 PDT
This regresses between the r35289 and r35359 nightlies. I'll bisect with local builds to find the exact revision.
Comment 4 Cameron Zwarich (cpst) 2009-03-12 04:22:30 PDT
This regressed in r35318:

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

I'll try to make a reduction.
Comment 5 Jim Oase 2009-03-12 20:28:58 PDT
This site also displays incorrectly in the same manner   http://www.wired.com/
Comment 6 Jim Oase 2009-03-12 20:30:39 PDT
Here is another site with this display problem
http://blog.wired.com/defense/2009/03/us-jet-shoots-d.html
Comment 7 Beth Dakin 2009-03-13 11:40:38 PDT
With my build of tip of tree WebKit, I can reproduce a problem at pfizer, but not at wired.com
Comment 8 Beth Dakin 2009-03-13 11:41:05 PDT
<rdar://problem/6680073>
Comment 9 Cameron Zwarich (cpst) 2009-03-13 12:16:04 PDT
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
Comment 10 Beth Dakin 2009-03-13 13:40:55 PDT
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.
Comment 11 Beth Dakin 2009-03-13 13:43:10 PDT
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);
Comment 12 Cameron Zwarich (cpst) 2009-03-13 14:00:40 PDT
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.
Comment 13 Beth Dakin 2009-03-13 14:02:02 PDT
Thanks, Cameron!
Comment 14 Cameron Zwarich (cpst) 2009-03-13 14:03:14 PDT
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).
Comment 15 Beth Dakin 2009-03-13 14:11:27 PDT
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;
}
Comment 16 Jim Oase 2009-03-14 00:47:13 PDT
Created attachment 28617 [details]
Screen shot 1 Wired.com
Comment 17 Jim Oase 2009-03-14 00:47:54 PDT
Created attachment 28618 [details]
Screen shot 2....... wired blog
Comment 18 Cameron Zwarich (cpst) 2009-03-14 15:04:44 PDT
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.
Comment 19 Cameron Zwarich (cpst) 2009-03-14 15:18:38 PDT
I'll unassign this because I will probably not be able to fix this.
Comment 20 Beth Dakin 2009-03-18 18:18:38 PDT
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.
Comment 21 Cameron Zwarich (cpst) 2009-03-18 19:13:15 PDT
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.
Comment 22 Beth Dakin 2009-03-19 13:06:06 PDT
Doh! It looks like my patch fixes the reduction, but not the Pfizer page. Grr.
Comment 23 Beth Dakin 2009-03-19 14:48:28 PDT
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.
Comment 24 Dave Hyatt 2009-03-19 14:54:35 PDT
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.
Comment 25 Dave Hyatt 2009-03-19 15:16:32 PDT
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.

Comment 26 Beth Dakin 2009-03-19 15:43:14 PDT
Created attachment 28767 [details]
New Patch

Here is a patch implementing Hyatt's idea.
Comment 27 Beth Dakin 2009-03-19 17:02:53 PDT
Created attachment 28772 [details]
Newer Patch

This one!
Comment 28 Dave Hyatt 2009-03-20 12:49:58 PDT
Comment on attachment 28772 [details]
Newer Patch

r=me
Comment 29 Beth Dakin 2009-03-20 12:57:24 PDT
Fixed with r41865.