Bug 25252

I'll try to find out when this regressed.

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.

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

Should we give eric a chance to fix it?

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

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.

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

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

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.

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

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.

Created attachment 29944 [details] Screenshot of problem

This is all inside the "resultDisplayArea" table.

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

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.

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.

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

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.

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

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.

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

(In reply to comment #25) > But I will deal with this by end of day Monday. Eric, are you still working on this?

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.

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

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

Created attachment 30081 [details] A test case! Tada! A test case!

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.

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

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

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

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

I committed this with revision 43378.

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.