Bug 101047 - REGRESSION (r132516): Javascript menu text incorrectly disappearing and reappearing
Summary: REGRESSION (r132516): Javascript menu text incorrectly disappearing and reapp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.8
: P1 Normal
Assignee: zalan
URL: http://www.kosmont.com/
Keywords: InRadar, Regression
: 117004 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-02 06:30 PDT by Kevin M. Dean
Modified: 2013-10-26 08:02 PDT (History)
11 users (show)

See Also:


Attachments
test reduction (736 bytes, text/html)
2013-08-09 08:57 PDT, zalan
no flags Details
test case (1.13 KB, text/html)
2013-08-15 11:15 PDT, zalan
no flags Details
Patch (17.07 KB, patch)
2013-08-16 04:33 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (17.06 KB, patch)
2013-08-16 04:45 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (18.89 KB, patch)
2013-08-23 01:49 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (543.19 KB, application/zip)
2013-08-23 04:19 PDT, Build Bot
no flags Details
Patch (18.20 KB, patch)
2013-09-12 05:13 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin M. Dean 2012-11-02 06:30:29 PDT
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.
Comment 1 Alexey Proskuryakov 2012-11-02 12:38:27 PDT
<rdar://problem/12626511>
Comment 2 Kevin M. Dean 2012-11-03 06:47:26 PDT
No longer appears to be an issue with r133386.
Comment 3 Antti Koivisto 2012-11-12 06:44:16 PST
Alexey, what made you think there is a regression from r132516 here? Should this really be closed?
Comment 4 Kevin M. Dean 2012-11-12 06:46:11 PST
Don't know about r132516, but I closed it because the specific issue I was seeing is no longer occurring.
Comment 5 Alexey Proskuryakov 2012-11-12 11:57:59 PST
> 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.
Comment 6 Antti Koivisto 2012-11-12 12:03:58 PST
Bringing back for another look. r132516 should not cause observable behavior changes.
Comment 7 Alexey Proskuryakov 2013-05-31 22:46:50 PDT
*** Bug 117004 has been marked as a duplicate of this bug. ***
Comment 8 Alexey Proskuryakov 2013-05-31 22:48:27 PDT
This is happening again, because subpixel antialiasing is disabled.

Antti, do you think that you could have another look?
Comment 9 Kevin M. Dean 2013-08-06 17:35:41 PDT
Any update?
Comment 10 Alexey Proskuryakov 2013-08-07 09:30:31 PDT
"because subpixel antialiasing is disabled"

subpixel layout, of course.

Antti, Andreas, will you have a chance to look into this?
Comment 11 Antti Koivisto 2013-08-07 09:35:12 PDT
Zalan said he is looking.
Comment 12 zalan 2013-08-08 09:36:34 PDT
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*)
Comment 13 zalan 2013-08-09 08:57:34 PDT
Created attachment 208438 [details]
test reduction

It seems that -webkit-backface-visibility has some side effect here.
Comment 14 Simon Fraser (smfr) 2013-08-09 09:35:11 PDT
Feels a bit similar to bug 115991. Got a regression range?
Comment 15 zalan 2013-08-09 10:14:13 PDT
(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)
Comment 16 Alexey Proskuryakov 2013-08-09 10:47:49 PDT
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.
Comment 17 zalan 2013-08-10 07:19:08 PDT
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)
Comment 18 zalan 2013-08-15 11:15:34 PDT
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)
Comment 19 zalan 2013-08-16 04:33:26 PDT
Created attachment 208910 [details]
Patch
Comment 20 zalan 2013-08-16 04:45:24 PDT
Created attachment 208914 [details]
Patch
Comment 21 Simon Fraser (smfr) 2013-08-19 12:01:56 PDT
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.
Comment 22 zalan 2013-08-23 01:49:24 PDT
Created attachment 209442 [details]
Patch
Comment 23 zalan 2013-08-23 03:26:29 PDT
(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.
Comment 24 Build Bot 2013-08-23 04:19:05 PDT
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
Comment 25 Build Bot 2013-08-23 04:19:08 PDT
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
Comment 26 Simon Fraser (smfr) 2013-09-11 15:07:07 PDT
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.
Comment 27 zalan 2013-09-12 05:13:41 PDT
Created attachment 211423 [details]
Patch
Comment 28 zalan 2013-09-12 05:14:20 PDT
Comment on attachment 211423 [details]
Patch

EWS testing
Comment 29 WebKit Commit Bot 2013-09-12 07:02:23 PDT
Comment on attachment 211423 [details]
Patch

Clearing flags on attachment: 211423

Committed r155607: <http://trac.webkit.org/changeset/155607>
Comment 30 WebKit Commit Bot 2013-09-12 07:02:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Kevin M. Dean 2013-09-12 16:11:59 PDT
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.
Comment 32 Kevin M. Dean 2013-10-26 08:02:35 PDT
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.