Bug 25252 – REGRESSION (r41469): mis-layout at endless.com before window resize

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alice Liu]

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>

[Cameron Zwarich (cpst)]

I'll try to find out when this regressed.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

This regressed in r41469:

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

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

Created attachment 29936 [details]
Roll out r41469

[Simon Fraser (smfr)]

Should we give eric a chance to fix it?

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/41469 is the original change for those playing along at home.

[Eric Seidel (no email)]

@smfr: This bug has been sitting idle for a couple weeks... I've kinda had my chance.

(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?

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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.

(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

[Eric Seidel (no email)]

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.

[Simon Fraser (smfr)]

Created attachment 29944 [details]
Screenshot of problem

[Eric Seidel (no email)]

This is all inside the "resultDisplayArea" table.

[Eric Seidel (no email)]

It does not look like this involves RenderWidgets.  The div which is shown in the wrong place in your screenshot is just a div.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Simon Fraser (smfr)]

The key seems to be the RenderObject::destroy() change.

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I have to go get on a plane.  But I will deal with this by end of day Monday.

[Darin Adler]

(In reply to comment #25)
> But I will deal with this by end of day Monday.

Eric, are you still working on this?

[Beth Dakin]

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.

[Eric Seidel (no email)]

I looked at it briefly yesterday.  I'll finish it today, for realz.

[Eric Seidel (no email)]

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)!

[Eric Seidel (no email)]

Created attachment 30081 [details]
A test case!

Tada!  A test case!

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Also, for the record: Facebook's site is surprisingly easy to debug.  No JS obfuscation (at least in event handlers).

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

Sorry, I thought I posted this like 5 hours ago!

[Beth Dakin]

Comment on attachment 30092 [details]
Fix regression caused by r41469, add test case to prevent it from happening again.

r=me

[Beth Dakin]

I committed this with revision 43378.

[Eric Seidel (no email)]

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.