Bug 84920 – Add ASSERTs to avoid querying dirtied z-index or normal flow lists on RenderLayer

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

[Julien Chaffraix]

Currently some call sites are explicitly checking that none of the RenderLayer's lists (negative z-index, positive z-index and normal flow) are not dirtied. I can't think of a time where we would be fine getting a dirtied value on a stacking context so it would make sense to just push the ASSERT down to the getters for broader coverage.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 138920 [details]
Proposed change 1: added the ad-hoc ASSERTs.

[Adrienne Walker]

Comment on attachment 138920 [details]
Proposed change 1: added the ad-hoc ASSERTs.

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

> Source/WebCore/rendering/RenderLayer.h:391
> +    // Those 2 lists only make sense for stacking contexts. Currently we don't update the dirtied lists on a
> +    // non-stacking context (see e.g. updateZOrderLists) as they are cleared when dirtyZOrderLists is called.

Maybe we should? It seems a little fraught to have dirty lists sticking around, even if they're unused.  It almost seems like it'd be cleaner from calling code to just iterate through all the lists that are non-empty rather than having to constantly check if we're allowed to see this list.

(As an aside, this seems like somewhat of a design wart in RenderLayer, in that RenderLayers that have stacking contexts have extra functionality that other RenderLayers don't.  Maybe this could be refactored out in the future somehow.)

> Source/WebCore/rendering/RenderLayer.h:393
> +    Vector<RenderLayer*>* posZOrderList() const { ASSERT(!m_zOrderListsDirty && isStackingContext()); return m_posZOrderList; }
> +    Vector<RenderLayer*>* negZOrderList() const { ASSERT(!m_zOrderListsDirty && isStackingContext()); return m_negZOrderList; }

These should probably be separate asserts so it's clear which clause is failing when it triggers.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:809
> +        layer->reflectionLayer()->updateLayerListsIfNeeded();

I am not an expert on reflection layers, but this seems like the wrong place to do this.  Maybe it should be happening in RenderLayer::updateLayerListsIfNeeded?

[Simon Fraser (smfr)]

Comment on attachment 138920 [details]
Proposed change 1: added the ad-hoc ASSERTs.

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

> Source/WebCore/rendering/RenderLayer.cpp:4176
> +    if (layer->isStackingContext()) {
> +        if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {

I'd prefer to not do this, and just ensure that non-stacking contexts have null lists.

[Julien Chaffraix]

Comment on attachment 138920 [details]
Proposed change 1: added the ad-hoc ASSERTs.

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

>> Source/WebCore/rendering/RenderLayer.h:391
>> +    // non-stacking context (see e.g. updateZOrderLists) as they are cleared when dirtyZOrderLists is called.
> 
> Maybe we should? It seems a little fraught to have dirty lists sticking around, even if they're unused.  It almost seems like it'd be cleaner from calling code to just iterate through all the lists that are non-empty rather than having to constantly check if we're allowed to see this list.
> 
> (As an aside, this seems like somewhat of a design wart in RenderLayer, in that RenderLayers that have stacking contexts have extra functionality that other RenderLayers don't.  Maybe this could be refactored out in the future somehow.)

For non-stacking contexts, those lists are empty (maybe null if they never were allocated) as we clear them when dirtyZOrderLists() is called.

I agree on the design wart and had it on my radar, but currently I won't go around attacking it.

>> Source/WebCore/rendering/RenderLayer.h:393
>> +    Vector<RenderLayer*>* negZOrderList() const { ASSERT(!m_zOrderListsDirty && isStackingContext()); return m_negZOrderList; }
> 
> These should probably be separate asserts so it's clear which clause is failing when it triggers.

Per consensus, it seems we would rather allow callers to call that on non-stacking context. That should satisfy your comment.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:809
>> +        layer->reflectionLayer()->updateLayerListsIfNeeded();
> 
> I am not an expert on reflection layers, but this seems like the wrong place to do this.  Maybe it should be happening in RenderLayer::updateLayerListsIfNeeded?

You are totally right, this was a shortsightedness on my side.

[Julien Chaffraix]

Created attachment 139571 [details]
Proposed refactoring 2: taking comments into account.

[Simon Fraser (smfr)]

Comment on attachment 139571 [details]
Proposed refactoring 2: taking comments into account.

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

> Source/WebCore/rendering/RenderLayer.h:933
> +        ASSERT(!m_negZOrderList || m_negZOrderList->isEmpty());
> +        ASSERT(!m_posZOrderList || m_posZOrderList->isEmpty());

Can we actually null out m_negZOrderList for non-stacking contexts? I'd prefer that to be an invariant.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1406
> +    // FIXME: We disable AC for elements in RenderFlowThread as it doesn't work properly.

I don't like AC used as an abbreviation for accelerated compositing.

[Julien Chaffraix]

Comment on attachment 139571 [details]
Proposed refactoring 2: taking comments into account.

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

>> Source/WebCore/rendering/RenderLayer.h:933
>> +        ASSERT(!m_posZOrderList || m_posZOrderList->isEmpty());
> 
> Can we actually null out m_negZOrderList for non-stacking contexts? I'd prefer that to be an invariant.

It would be possible but that means we need to change the logic in RenderBoxModelObject::styleWillChange() to recognize when we are demoted from being a stacking context. Currently RenderLayer doesn't know when it switches from being a stacking context to not being one. I can put a FIXME about that as I would like to investigate moving this logic to RenderLayer::styleChanged(). If I can make that work, it could simplify the logic even more.

[Simon Fraser (smfr)]

(In reply to comment #7)
> (From update of attachment 139571 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139571&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.h:933
> >> +        ASSERT(!m_posZOrderList || m_posZOrderList->isEmpty());
> > 
> > Can we actually null out m_negZOrderList for non-stacking contexts? I'd prefer that to be an invariant.
> 
> It would be possible but that means we need to change the logic in RenderBoxModelObject::styleWillChange() to recognize when we are demoted from being a stacking context. Currently RenderLayer doesn't know when it switches from being a stacking context to not being one. I can put a FIXME about that as I would like to investigate moving this logic to RenderLayer::styleChanged(). If I can make that work, it could simplify the logic even more.

You could just always null out the pointers in updateZOrderLists().

[Julien Chaffraix]

Comment on attachment 139571 [details]
Proposed refactoring 2: taking comments into account.

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

>>>> Source/WebCore/rendering/RenderLayer.h:933
>>>> +        ASSERT(!m_posZOrderList || m_posZOrderList->isEmpty());
>>> 
>>> Can we actually null out m_negZOrderList for non-stacking contexts? I'd prefer that to be an invariant.
>> 
>> It would be possible but that means we need to change the logic in RenderBoxModelObject::styleWillChange() to recognize when we are demoted from being a stacking context. Currently RenderLayer doesn't know when it switches from being a stacking context to not being one. I can put a FIXME about that as I would like to investigate moving this logic to RenderLayer::styleChanged(). If I can make that work, it could simplify the logic even more.
> 
> You could just always null out the pointers in updateZOrderLists().

I think this solution makes sense for the time being. We cannot check the invariant in updateZOrderLists though but could do it in the 2 list getters.

[Simon Fraser (smfr)]

Fine by me if you want to do that in the current patch.

[Julien Chaffraix]

(In reply to comment #10)
> Fine by me if you want to do that in the current patch.

I will implement your idea as it makes a lot of sense and give us coverage for bad usage sooner rather than later.

I will file a bug about making that a real invariant that we enforce at stacking context change (the reason being that there may be a reason why we need to dirty our list on styleWillChange and not styleDidChange like most of the other operations so there is some investigation needed on my side).

[Julien Chaffraix]

Committed r115913: <http://trac.webkit.org/changeset/115913>

[Julien Chaffraix]

(In reply to comment #11)
> (In reply to comment #10)
> > Fine by me if you want to do that in the current patch.
> 
> I will implement your idea as it makes a lot of sense and give us coverage for bad usage sooner rather than later.

Filed bug 85437 to follow up.

[Simon Fraser (smfr)]

Hitting the new assertion under:


#0  0x0000000105634ed5 in WebCore::RenderLayer::negZOrderList (this=0x121d27018) at RenderLayer.h:398
#1  0x0000000105652249 in WebCore::RenderLayerCompositor::layerHas3DContent (this=0x121a1b070, layer=0x121d27018) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebCore/rendering/RenderLayerCompositor.cpp:2237
#2  0x0000000105652334 in WebCore::RenderLayerCompositor::layerHas3DContent (this=0x121a1b070, layer=0x121a02a18) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebCore/rendering/RenderLayerCompositor.cpp:2250
#3  0x0000000105652334 in WebCore::RenderLayerCompositor::layerHas3DContent (this=0x121a1b070, layer=0x121a04168) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebCore/rendering/RenderLayerCompositor.cpp:2250
#4  0x0000000105652185 in WebCore::RenderLayerCompositor::has3DContent (this=0x121a1b070) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebCore/rendering/RenderLayerCompositor.cpp:1324
#5  0x0000000104a09191 in WebCore::FrameView::isSoftwareRenderable (this=0x121a19ec0) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebCore/page/FrameView.cpp:850
#6  0x0000000102787169 in -[WebView(WebPrivate) _isSoftwareRenderable] (self=0x10fb72320, _cmd=0x7fff8c2e64c6) at /Volumes/DataSSD/Development/apple/webkit/WebKit.git/Source/WebKit/mac/WebView/WebView.mm:2469
#7  0x000000010059012b in -[WebView(SafariSnapshotGeneration) createImageForRect:inSubview:] (self=0x10fb72320, _cmd=0x7fff8c2e63de, rectToCapture={origin = {x = 0, y = 0}, size = {width = 974, height = 887}}, subview=0x110915dd0) at /Volumes/WebKit/Internal/Safari/mac/SafariWebViewSnapshotGeneration.mm:65

[Simon Fraser (smfr)]

Another one


>  1 com.apple.WebCore              0x10b70291d WebCore::RenderLayer::negZOrderList() const + 0x5d (RenderLayer.h:398)
   2 com.apple.WebCore              0x10b712732 WebCore::RenderLayerBacking::hasVisibleNonCompositingDescendantLayers() const + 0x122 (RenderLayerBacking.cpp:928)
   3 com.apple.WebCore              0x10b712ea5 WebCore::RenderLayerBacking::paintsChildren() const + 0x55 (RenderLayerBacking.cpp:841)
   4 com.apple.WebCore              0x10b713017 WebCore::RenderLayerBacking::isSimpleContainerCompositingLayer() const + 0x67 (RenderLayerBacking.cpp:857)
   5 com.apple.WebCore              0x10b712b89 WebCore::RenderLayerBacking::containsPaintedContent() const + 0x19 (RenderLayerBacking.cpp:952)
   6 com.apple.WebCore              0x10b712ad9 WebCore::RenderLayerBacking::updateDrawsContent() + 0x19 (RenderLayerBacking.cpp:611)
   7 com.apple.WebCore              0x10b71fdda WebCore::RenderLayerCompositor::rootLayerAttachmentChanged() + 0x6a (RenderLayerCompositor.cpp:2199)
   8 com.apple.WebCore              0x10b71e2f7 WebCore::RenderLayerCompositor::detachRootLayer() + 0x1d7 (RenderLayerCompositor.cpp:2185)
   9 com.apple.WebCore              0x10b71e115 WebCore::RenderLayerCompositor::willMoveOffscreen() + 0x45 (RenderLayerCompositor.cpp:1269)
  10 com.apple.WebCore              0x10b8548db WebCore::RenderView::willMoveOffscreen() + 0x4b (RenderView.cpp:887)
  11 com.apple.WebCore              0x10abd6fe5 WebCore::FrameView::willMoveOffscreen() + 0x65 (FrameView.cpp:877)
  12 com.apple.WebCore              0x10b572776 WebCore::Page::willMoveOffscreen() + 0x56 (Page.cpp:704)
  13 com.apple.WebKit2              0x108cc9cd1 WebKit::WebPage::setIsInWindow(bool) + 0x71 (WebPage.cpp:1700)
  14 com.apple.WebKit2              0x108cc7f92 WebKit::WebPage::WebPage(unsigned long long, WebKit::WebPageCreationParameters const&) + 0xa12 (WebPage.cpp:298)
  15 com.apple.WebKit2              0x108cc7575 WebKit::WebPage::WebPage(unsigned long long, WebKit::WebPageCreationParameters const&) + 0x25 (WebPage.cpp:312)
  16 com.apple.WebKit2              0x108cc74a1 WebKit::WebPage::create(unsigned long long, WebKit::WebPageCreationParameters const&) + 0x41 (WebPage.cpp:176)
  17 com.apple.WebKit2              0x108d7e54c WebKit::WebProcess::createWebPage(unsigned long long, WebKit::WebPageCreationParameters const&) + 0xec (WebProcess.cpp:530)
  18 com.apple.WebKit2              0x108d96518 void CoreIPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(unsigned long long, WebKit::WebPageCreationParameters const&), unsigned long long, WebKit::WebPageCreationParameters>(CoreIPC::Arguments2<unsigned long long, WebKit::WebPageCreationParameters> const&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(unsigned long long, WebKit::WebPageCreationParameters const&)) + 0x98 (HandleMessage.h:26)
  19 com.apple.WebKit2              0x108d94c91 void CoreIPC::handleMessage<Messages::WebProcess::CreateWebPage, WebKit::WebProcess, void (WebKit::WebProcess::*)(unsigned long long, WebKit::WebPageCreationParameters const&)>(CoreIPC::ArgumentDecoder*, WebKit::WebProcess*, void (WebKit::WebProcess::*)(unsigned long long, WebKit::WebPageCreationParameters const&)) + 0x91 (HandleMessage.h:303)
  20 com.apple.WebKit2              0x108d94322 WebKit::WebProcess::didReceiveWebProcessMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 0xb2 (WebProcessMessageReceiver.cpp:94)
  21 com.apple.WebKit2              0x108d7e9bb WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 0x5b (WebProcess.cpp:604)
  22 com.apple.WebKit2              0x108c2c17e WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 0x15e (WebConnectionToUIProcess.cpp:88)
  23 com.apple.WebKit2              0x108c2c1cd non-virtual thunk to WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*) + 0x3d
  24 com.apple.WebKit2              0x108ad655c CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder>&) + 0x15c (Connection.cpp:692)
  25 com.apple.WebKit2              0x108ad8ca8 CoreIPC::Connection::dispatchMessages() + 0xc8 (Connection.cpp:720)
  26 com.apple.WebKit2              0x108adf752 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*) + 0x72 (Functional.h:173)
  27 com.apple.WebKit2              0x108adf6d5 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()() + 0x35 (Functional.h:405)
  28 com.apple.WebCore              0x10b89f155 WTF::Function<void ()>::operator()() const + 0x85 (Functional.h:613)
  29 com.apple.WebCore              0x10b89eee7 WebCore::RunLoop::performWork() + 0x87 (RunLoop.cpp:66)
  30 com.apple.WebCore              0x10b8a0270 WebCore::RunLoop::performWork(void*) + 0x60 (RunLoopMac.mm:65)