Bug 101047 – REGRESSION (r132516): Javascript menu text incorrectly disappearing and reappearing

• Component: CSS
• Status: RESOLVED FIXED

[Kevin M. Dean]

When rolling over the menu at the top of the page, the dropdown menu text sometimes disappears leaving an empty menu column, and sometimes reappears.

[Alexey Proskuryakov]

<rdar://problem/12626511>

[Kevin M. Dean]

No longer appears to be an issue with r133386.

[Antti Koivisto]

Alexey, what made you think there is a regression from r132516 here? Should this really be closed?

[Kevin M. Dean]

Don't know about r132516, but I closed it because the specific issue I was seeing is no longer occurring.

[Alexey Proskuryakov]

> Alexey, what made you think there is a regression from r132516 here?

I bisected.

> Should this really be closed?

Probably not.

It stopped occurring with <http://trac.webkit.org/r133351>, so the issue is probably hidden now, not fully fixed.

[Antti Koivisto]

Bringing back for another look. r132516 should not cause observable behavior changes.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

This is happening again, because subpixel antialiasing is disabled.

Antti, do you think that you could have another look?

[Kevin M. Dean]

Any update?

[Alexey Proskuryakov]

"because subpixel antialiasing is disabled"

subpixel layout, of course.

Antti, Andreas, will you have a chance to look into this?

[Antti Koivisto]

Zalan said he is looking.

[alan]

Debug build asserts when hovering the menu. It probably has a lot to do with the blank content.

ASSERTION FAILED: parentClipRect != PaintInfo::infiniteRect()
RenderLayerBacking.cpp(681) : void WebCore::RenderLayerBacking::updateGraphicsLayerGeometry()
1   0x106bcaec0 WTFCrash
2   0x108d40eab WebCore::RenderLayerBacking::updateGraphicsLayerGeometry()
3   0x108d5417a WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer*, WebCore::RenderLayer*, bool)
4   0x108d543d0 WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer*, WebCore::RenderLayer*, bool)
5   0x108d543d0 WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer*, WebCore::RenderLayer*, bool)
6   0x108d543d0 WebCore::RenderLayerCompositor::updateCompositingDescendantGeometry(WebCore::RenderLayer*, WebCore::RenderLayer*, bool)
7   0x108d40659 WebCore::RenderLayerBacking::updateAfterLayout(unsigned int)
8   0x108d0d8e7 WebCore::RenderLayer::updateLayerPositions(WebCore::RenderGeometryMap*, unsigned int)
9   0x108d0d201 WebCore::RenderLayer::updateLayerPositionsAfterLayout(WebCore::RenderLayer const*, unsigned int)
10  0x10802fba8 WebCore::FrameView::layout(bool)
11  0x107d92158 WebCore::Document::updateLayout()
12  0x108d23f76 WebCore::RenderLayer::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&)
13  0x108ea2c8f WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&)
14  0x108ea2c44 WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestResult&)
15  0x107d99bbe WebCore::Document::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::LayoutPoint const&, WebCore::PlatformMouseEvent const&)
16  0x107f21341 WebCore::EventHandler::prepareMouseEvent(WebCore::HitTestRequest const&, WebCore::PlatformMouseEvent const&)
17  0x107f218a9 WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const&, WebCore::HitTestResult*, bool)
18  0x107f213d6 WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const&)
19  0x1052e78a8 WebKit::handleMouseEvent(WebKit::WebMouseEvent const&, WebKit::WebPage*, bool)
20  0x1052e76a7 WebKit::WebPage::mouseEvent(WebKit::WebMouseEvent const&)
21  0x105331987 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&), WebKit::WebMouseEvent>(CoreIPC::Arguments1<WebKit::WebMouseEvent> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&))
22  0x1053237b5 void CoreIPC::handleMessage<Messages::WebPage::MouseEvent, WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&))
23  0x10531d821 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
24  0x1052ebe77 WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
25  0x1052ebeb7 non-virtual thunk to WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
26  0x105092ae0 CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
27  0x1053ccd6a WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
28  0x104fe6aa3 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
29  0x104fe2dda CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
30  0x104fe6a3b CoreIPC::Connection::dispatchOneMessage()
31  0x104ff2042 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)

[alan]

Created attachment 208438 [details]
test reduction

It seems that -webkit-backface-visibility has some side effect here.

[Simon Fraser (smfr)]

Feels a bit similar to bug 115991. Got a regression range?

[alan]

(In reply to comment #14)
> Feels a bit similar to bug 115991. Got a regression range?

It's not a regression afaict. Oddly, subpixel layout makes this issue hidden, so you can't repro it starting from http://trac.webkit.org/changeset/133351 up until http://trac.webkit.org/changeset/149208, when we disabled subpixel layout (at r149209)

[Alexey Proskuryakov]

There is a regression number right in the title, and I just re-tested that this is when the site regressed. The issue is 100% reproducible for me.

The reduction fails in r132514, so it has to be less than 100% precise.

[alan]

r132516 indeed has some impact on the page which is caused by the smaller number of setNeedsStyleRecalc() calls. I suspect that might just be a side effect. 
Extending the reduced test case (and if it turns out to be different from the original test case, I'll create a new bug for the old one)

[alan]

Created attachment 208822 [details]
test case

Here is a test case for r132516. Patch is coming up that covers the generic case (broken since the beginning as the first test case indicates) and it also fixes the r132516 regression. (r132516 changeset makes the problem more visible)

[alan]

Created attachment 208910 [details]
Patch

[alan]

Created attachment 208914 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 208914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208914&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1186
> +                RenderLayerCompositor* compositor = child->compositor();

All RenderLayers in the same document share the same RenderLayerCompositor. You could fetch this once at the top of the function.

> Source/WebCore/rendering/RenderLayer.cpp:1188
> +                    compositor->setCompositingLayersNeedRebuild();

Would be nice to do this incrementally if we can.

> Source/WebCore/rendering/RenderLayer.h:1058
> +    void updateDescendantClippingContext(bool);

Unclear what the bool means here.

> LayoutTests/ChangeLog:22
> +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants-expected.html: Added.
> +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants.html: Added.
> +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants2-expected.html: Added.
> +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants2.html: Added.

I think text-output tests using layerTreeAsText() might be a better way to test this.

[alan]

Created attachment 209442 [details]
Patch

[alan]

(In reply to comment #21)
> (From update of attachment 208914 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208914&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1186
> > +                RenderLayerCompositor* compositor = child->compositor();
> 
> All RenderLayers in the same document share the same RenderLayerCompositor. You could fetch this once at the top of the function.
Thanks for pointing it out. Fixed.

> 
> > Source/WebCore/rendering/RenderLayer.h:1058
> > +    void updateDescendantClippingContext(bool);
> 
> Unclear what the bool means here.
Fixed.

> 
> > LayoutTests/ChangeLog:22
> > +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants-expected.html: Added.
> > +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants.html: Added.
> > +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants2-expected.html: Added.
> > +        * compositing/overflow/clipping-behaviour-change-is-not-propagated-to-descendants2.html: Added.
> 
> I think text-output tests using layerTreeAsText() might be a better way to test this.
Agreed. Fixed.


> 
> > Source/WebCore/rendering/RenderLayer.cpp:1188
> > +                    compositor->setCompositingLayersNeedRebuild();
> 
> Would be nice to do this incrementally if we can.
Indeed, though dirty layers need to be rebuilt before the first subsequent painting happens. It is pretty much the time slot between layout and paint, which leaves us with little room for being incremental. -unless I missed something here.
I moved the code around a bit, but that's mainly about improving code quality with no functionality change.

[Build Bot]

Comment on attachment 209442 [details]
Patch

Attachment 209442 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1545414

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209447 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Simon Fraser (smfr)]

Comment on attachment 209442 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209442&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1189
> +            layerNeedsRebuild = backing->updateAncestorClippingLayer(compositor->clippedByAncestor(child)) || layerNeedsRebuild;

I'd prefer to write this as
if (backing->updateAncestorClippingLayer(compositor->clippedByAncestor(child)))
  layerNeedsRebuild = true;

> Source/WebCore/rendering/RenderLayer.cpp:6242
> +                compositor->scheduleCompositingLayerUpdate();

You don't need to schedule an update. We'll update compositing at the end of this style update or the subsequent layout.

[alan]

Created attachment 211423 [details]
Patch

[alan]

Comment on attachment 211423 [details]
Patch

EWS testing

[WebKit Commit Bot]

Comment on attachment 211423 [details]
Patch

Clearing flags on attachment: 211423

Committed r155607: <http://trac.webkit.org/changeset/155607>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kevin M. Dean]

Should we be concerned that a variation of this bug exists in the Safari 6.1 seeds using the webkit installed by the seed and not the nightly. On the homepage of the test link, the text appears and immediately disappears. Probably reacting to the slider on the page since it doesn't have a problem on the sub-pages.

[Kevin M. Dean]

Looks even worse in Safari 7 now than it did with 6.1. I sure hope the fix in the nightlies makes it to release soon before my clients start complaining.