Bug 25252

Summary: REGRESSION (r41469): mis-layout at endless.com before window resize
Product: WebKit Reporter: Alice Liu <alice.barraclough>
Component: Layout and RenderingAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, mitz, simon.fraser
Priority: P2 Keywords: InRadar, NeedsReduction, Regression
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://www.endless.com/Womens-Shoes/b/242169011/ref=topnav_sd_wm_gw
Attachments:
Description Flags
Roll out r41469
eric: review-
Screenshot of problem
none
A test case!
none
Fix regression caused by r41469, add test case to prevent it from happening again. bdakin: review+

Description Alice Liu 2009-04-16 16:27:02 PDT
below most items at endless.com, there is a "More Colors" box, which on latest builds appears to be mis- layedout far to the left.  The layout corrects itself when the window is resized. 

<rdar://problem/6800282>
Comment 1 Cameron Zwarich (cpst) 2009-04-18 17:12:33 PDT
I'll try to find out when this regressed.
Comment 2 Cameron Zwarich (cpst) 2009-04-18 17:24:38 PDT
This regressed between the r41431 and r41545 nightlies. There is a nightly in the middle, r41521, but the site doesn't display correctly at all in that nightly. I'll try to bisect with local builds to see if I can narrow it down any more.
Comment 3 Cameron Zwarich (cpst) 2009-04-18 19:21:50 PDT
This regressed in r41469:

http://trac.webkit.org/changeset/41469
Comment 4 mitz 2009-05-01 01:05:11 PDT
*** Bug 25508 has been marked as a duplicate of this bug. ***
Comment 5 mitz 2009-05-01 01:09:43 PDT
Created attachment 29936 [details]
Roll out r41469
Comment 6 Simon Fraser (smfr) 2009-05-01 09:11:08 PDT
Should we give eric a chance to fix it?
Comment 7 Eric Seidel (no email) 2009-05-01 10:12:59 PDT
Comment on attachment 29936 [details]
Roll out r41469

This roll out covers more than I changed.  I don't remember making any changes to

     // Grab the arena from node()->document()->renderArena() before clearing the node pointer.
103      // Clear the node before deref-ing, as this may be deleted when deref is called.
104      RenderArena* arena = renderArena();
 99     if (style() && (style()->height().isPercent() || style()->minHeight().isPercent() || style()->maxHeight().isPercent()))
 100         RenderBlock::removePercentHeightDescendant(this);

http://trac.webkit.org/changeset/41469
Comment 8 Eric Seidel (no email) 2009-05-01 10:13:54 PDT
You're welcome to roll out this change.  I have no clue how it's causing any problems.  It was not meant to be a functional change at all.

I made some cleanup changes (like the renderBox stuff) which I should re-do.
Comment 9 Eric Seidel (no email) 2009-05-01 10:14:11 PDT
http://trac.webkit.org/changeset/41469 is the original change for those playing along at home.
Comment 10 Eric Seidel (no email) 2009-05-01 10:16:39 PDT
@smfr: This bug has been sitting idle for a couple weeks... I've kinda had my chance.
Comment 11 mitz 2009-05-01 10:21:28 PDT
(In reply to comment #7)
> (From update of attachment 29936 [details] [review])
> This roll out covers more than I changed.  I don't remember making any changes
> to
> 
>      // Grab the arena from node()->document()->renderArena() before clearing
> the node pointer.
> 103      // Clear the node before deref-ing, as this may be deleted when deref
> is called.
> 104      RenderArena* arena = renderArena();
>  99     if (style() && (style()->height().isPercent() ||
> style()->minHeight().isPercent() || style()->maxHeight().isPercent()))
>  100         RenderBlock::removePercentHeightDescendant(this);
> 
> http://trac.webkit.org/changeset/41469
> 

Can you be more specific about what this changes that you did not change?
Comment 12 Simon Fraser (smfr) 2009-05-01 10:28:02 PDT
The only behavioral changes that I can possibly see from the original commit are ordering dependencies in the RenderWidget code and the destroyLayer() code, or breakage of stackingContext(). The rest really looks benign.
Comment 13 Simon Fraser (smfr) 2009-05-01 10:28:37 PDT
Also I tried testing at www.endless.com, but it's very unclear what the issue is. I was not able to see the regression.
Comment 14 mitz 2009-05-01 10:31:57 PDT
(In reply to comment #13)
> Also I tried testing at www.endless.com, but it's very unclear what the issue
> is. I was not able to see the regression.

The instructions aren't very clear. Here is what I do to reproduce the bug:
1) Navigate to <http://www.endless.com/>
2) From the menu on the left hand side, choose shop by Department > Women's shoes
3) In the Category section on the left hand side, click Athletic & Outdoor
4) Force a repaint to see more of the problem
Comment 15 Eric Seidel (no email) 2009-05-01 11:03:21 PDT
I should give a whack at trying to produce a reduction.  I worry that my change may have simply exposed some other bug in WebKit which is now being hit by this one site, but would be hit by others even w/o my change.
Comment 16 Simon Fraser (smfr) 2009-05-01 11:09:09 PDT
Created attachment 29944 [details]
Screenshot of problem
Comment 17 Eric Seidel (no email) 2009-05-01 11:16:18 PDT
This is all inside the "resultDisplayArea" table.
Comment 18 Eric Seidel (no email) 2009-05-01 11:16:52 PDT
It does not look like this involves RenderWidgets.  The div which is shown in the wrong place in your screenshot is just a div.
Comment 19 Eric Seidel (no email) 2009-05-01 11:22:15 PDT
I wonder if changing the order to set that it has no layer before destroying the layer would change anything.

	    ASSERT(hasLayer()); 
 	61	    ASSERT(m_layer); 
 	62	    m_layer->destroy(renderArena()); 
 	63	    m_layer = 0; 
 	64	    setHasLayer(false);

I do seem to have changed the relative order between setHasLayer(false) and m_layer->destory(), but I really really doubt that should matter.

I guess RenderWidgets would always be involved since the FrameView is a RenderWidget, no?

Is this a paint problem or a layout problem?  Changing the order in which I clear clip rects vs. destroy the layer could cause a paint problem, but shouldn't cause a layout problem.
Comment 20 Eric Seidel (no email) 2009-05-01 11:24:31 PDT
It would also be possible to try moving the clip rect removal up back above:
        RenderBlock::removePercentHeightDescendant(this); 

but again, I doubt that that' the issue... then again, we're grasping at straws here due to how little changed there was in this commit.

I need to finish getting ready to fly... then I can look at this further.
Comment 21 Simon Fraser (smfr) 2009-05-01 11:32:31 PDT
The key seems to be the RenderObject::destroy() change.
Comment 22 Simon Fraser (smfr) 2009-05-01 12:00:23 PDT
The behavioral change from the original commit is that the state of hasLayer() changed when removeOnlyThisLayer() is being called. Previously, it was false, because RenderBoxModelObject::styleDidChange() had already set it so. After the change, it remains true until destroyLayer is called. This is affecting the behavior of updateLayerPositions() on the children.
Comment 23 Simon Fraser (smfr) 2009-05-01 12:02:58 PDT
This patch fixes it:

diff --git a/WebCore/rendering/RenderBoxModelObject.cpp b/WebCore/rendering/RenderBoxModelObject.cpp
index f995b0f..72ba16b 100644
--- a/WebCore/rendering/RenderBoxModelObject.cpp
+++ b/WebCore/rendering/RenderBoxModelObject.cpp
@@ -57,7 +57,6 @@ RenderBoxModelObject::~RenderBoxModelObject()
 
 void RenderBoxModelObject::destroyLayer()
 {
-    ASSERT(hasLayer());
     ASSERT(m_layer);
     m_layer->destroy(renderArena());
     m_layer = 0;
@@ -114,6 +113,7 @@ void RenderBoxModelObject::styleDidChange(StyleDifference diff, const RenderStyl
                 m_layer->updateLayerPositions();
         }
     } else if (layer() && layer()->parent()) {
+        setHasLayer(false);
         setHasTransform(false); // Either a transform wasn't specified or the object doesn't support transforms, so just null out the bit.
         setHasReflection(false);
         m_layer->removeOnlyThisLayer(); // calls destroyLayer() which clears m_layer
Comment 24 Eric Seidel (no email) 2009-05-01 13:10:39 PDT
Specifically void RenderLayer::updateLayerPosition() (note Position not Positions!) which is called from void RenderLayer::updateLayerPositions(bool doFullRepaint, bool checkForRepaint) crawls up the rendering tree checking hasLayer().

I would like to make a test case, and then plan to make a fix similar to what Simon posted.
Comment 25 Eric Seidel (no email) 2009-05-01 14:24:56 PDT
I have to go get on a plane.  But I will deal with this by end of day Monday.
Comment 26 Darin Adler 2009-05-06 16:19:17 PDT
(In reply to comment #25)
> But I will deal with this by end of day Monday.

Eric, are you still working on this?
Comment 27 Beth Dakin 2009-05-06 16:59:46 PDT
This affects Facebook too:

Log into Facebook
Find something that at least two people have liked
Click on "X people like this"

Profile picture appear for the people that like the item, but they appear in the wrong place.
Comment 28 Eric Seidel (no email) 2009-05-06 17:03:21 PDT
I looked at it briefly yesterday.  I'll finish it today, for realz.
Comment 29 Eric Seidel (no email) 2009-05-06 18:24:40 PDT
So it seems to be changing the opacity on a parent div which is triggering the case:
    } else if (layer() && layer()->parent()) {
        setHasTransform(false); // Either a transform wasn't specified or the object doesn't support transforms, so just null out the bit.
        setHasReflection(false);
        m_layer->removeOnlyThisLayer(); // calls destroyLayer() which clears m_layer
        if (s_wasFloating && isFloating())
            setChildNeedsLayout(true);
    }

which is misbehaving.  Still trying to make a test (so that we don't regress this again)!
Comment 30 Eric Seidel (no email) 2009-05-06 18:52:36 PDT
Created attachment 30081 [details]
A test case!

Tada!  A test case!
Comment 31 Eric Seidel (no email) 2009-05-06 19:14:48 PDT
I'm testing my fix now.  Will post it shortly.  Again, my apologies for the delay!  I just didn't want to fix this w/o first adding a test case.
Comment 32 Eric Seidel (no email) 2009-05-06 19:15:55 PDT
Also, for the record: Facebook's site is surprisingly easy to debug.  No JS obfuscation (at least in event handlers).
Comment 33 Eric Seidel (no email) 2009-05-07 00:30:18 PDT
Created attachment 30092 [details]
Fix regression caused by r41469, add test case to prevent it from happening again.

 10 files changed, 92 insertions(+), 5 deletions(-)
Comment 34 Eric Seidel (no email) 2009-05-07 00:32:50 PDT
Sorry, I thought I posted this like 5 hours ago!
Comment 35 Beth Dakin 2009-05-07 00:39:40 PDT
Comment on attachment 30092 [details]
Fix regression caused by r41469, add test case to prevent it from happening again.

r=me
Comment 36 Beth Dakin 2009-05-07 17:17:04 PDT
I committed this with revision 43378.
Comment 37 Eric Seidel (no email) 2009-05-07 18:01:01 PDT
I chose not to commit this last night because I had to walk away from my desk to go to dinner and didn't want to break the bots.  I'm surprised ya'll are in such a hurry. :)  But thank you for committing it anyway.