From a794a12c8c1ad946c848c457cbe6026886b5f5e3 Mon Sep 17 00:00:00 2001 From: Doug Thayer Date: Fri, 22 Mar 2019 18:29:04 +0000 Subject: [PATCH] Bug 1441308 - Always send parent commands when sending mDestroyedActors r=kats,sotaro If we try to send them separately as we were before, we can run into cases where we try to destroy the actors and then send the OpRemoveTexture, which crashes. Differential Revision: https://phabricator.services.mozilla.com/D23987 --HG-- extra : moz-landing-system : lando --- gfx/layers/wr/RenderRootTypes.h | 5 +- gfx/layers/wr/WebRenderBridgeChild.h | 3 + gfx/layers/wr/WebRenderBridgeParent.cpp | 78 ++++++++++++++----------- gfx/layers/wr/WebRenderBridgeParent.h | 7 +-- gfx/layers/wr/WebRenderLayerManager.cpp | 10 +++- gfx/webrender_bindings/WebRenderAPI.cpp | 4 +- 6 files changed, 61 insertions(+), 46 deletions(-) diff --git a/gfx/layers/wr/RenderRootTypes.h b/gfx/layers/wr/RenderRootTypes.h index 21e2e2114732..18969d49b927 100644 --- a/gfx/layers/wr/RenderRootTypes.h +++ b/gfx/layers/wr/RenderRootTypes.h @@ -21,12 +21,12 @@ struct RenderRootDisplayListData { gfx::IntRect mRect; nsTArray mCommands; wr::LayoutSize mContentSize; - mozilla::ipc::ByteBuf mDL; + Maybe mDL; wr::BuiltDisplayListDescriptor mDLDesc; nsTArray mResourceUpdates; nsTArray mSmallShmems; nsTArray mLargeShmems; - WebRenderScrollData mScrollData; + Maybe mScrollData; }; struct RenderRootUpdates { @@ -65,5 +65,4 @@ struct IPDLParamTraits { } // namespace ipc } // namespace mozilla - #endif /* GFX_RENDERROOTTYPES_H */ diff --git a/gfx/layers/wr/WebRenderBridgeChild.h b/gfx/layers/wr/WebRenderBridgeChild.h index 9bcd721c0772..34c33ba17d26 100644 --- a/gfx/layers/wr/WebRenderBridgeChild.h +++ b/gfx/layers/wr/WebRenderBridgeChild.h @@ -66,6 +66,9 @@ class WebRenderBridgeChild final : public PWebRenderBridgeChild, void AddWebRenderParentCommand(const WebRenderParentCommand& aCmd, wr::RenderRoot aRenderRoot); + bool HasWebRenderParentCommands(wr::RenderRoot aRenderRoot) { + return !mParentCommands[aRenderRoot].IsEmpty(); + } void UpdateResources(wr::IpcResourceUpdateQueue& aResources, wr::RenderRoot aRenderRoot); diff --git a/gfx/layers/wr/WebRenderBridgeParent.cpp b/gfx/layers/wr/WebRenderBridgeParent.cpp index bbd0835dc2bf..956c619a6fee 100644 --- a/gfx/layers/wr/WebRenderBridgeParent.cpp +++ b/gfx/layers/wr/WebRenderBridgeParent.cpp @@ -896,25 +896,13 @@ void WebRenderBridgeParent::SetAPZSampleTime() { bool WebRenderBridgeParent::SetDisplayList( wr::RenderRoot aRenderRoot, const gfx::IntRect& aRect, - const nsTArray& aCommands, const wr::LayoutSize& aContentSize, ipc::ByteBuf&& aDL, const wr::BuiltDisplayListDescriptor& aDLDesc, const nsTArray& aResourceUpdates, const nsTArray& aSmallShmems, const nsTArray& aLargeShmems, const TimeStamp& aTxnStartTime, - wr::TransactionBuilder& aTxn, Maybe& aTxnSender, - wr::Epoch aWrEpoch, bool aValidTransaction, bool aObserveLayersUpdate) { - wr::WebRenderAPI* api = Api(aRenderRoot); - aTxn.SetLowPriority(!IsRootWebRenderBridgeParent()); - if (aValidTransaction) { - aTxnSender.emplace(api, &aTxn); - } - - if (NS_WARN_IF( - !ProcessWebRenderParentCommands(aCommands, aTxn, aRenderRoot))) { - return false; - } - + wr::TransactionBuilder& aTxn, wr::Epoch aWrEpoch, bool aValidTransaction, + bool aObserveLayersUpdate) { if (NS_WARN_IF(!UpdateResources(aResourceUpdates, aSmallShmems, aLargeShmems, aTxn))) { return false; @@ -954,7 +942,7 @@ bool WebRenderBridgeParent::SetDisplayList( MakeUnique(this, aWrEpoch, aTxnStartTime)); } - api->SendTransaction(aTxn); + Api(aRenderRoot)->SendTransaction(aTxn); // We will schedule generating a frame after the scene // build is done, so we don't need to do it here. @@ -1006,16 +994,26 @@ mozilla::ipc::IPCResult WebRenderBridgeParent::RecvSetDisplayList( mReceivedDisplayList = true; bool observeLayersUpdate = ShouldParentObserveEpoch(); - // The IsFirstPaint() flag should be the same for all the scrolldata across - // all the renderroot display lists in a given transaction. We assert this - // below. So we can read the flag from any one of them. - if (aDisplayLists[0].mScrollData.IsFirstPaint()) { - mIsFirstPaint = true; - } + // The IsFirstPaint() flag should be the same for all the non-empty + // scrolldata across all the renderroot display lists in a given + // transaction. We assert this below. So we can read the flag from any one + // of them. + Maybe firstScrollDataIndex; for (size_t i = 1; i < aDisplayLists.Length(); i++) { - // Ensure the flag is the same on all of them. - MOZ_RELEASE_ASSERT(aDisplayLists[i].mScrollData.IsFirstPaint() == - aDisplayLists[0].mScrollData.IsFirstPaint()); + auto& scrollData = aDisplayLists[i].mScrollData; + if (scrollData) { + if (firstScrollDataIndex.isNothing()) { + firstScrollDataIndex = Some(i); + if (scrollData && scrollData->IsFirstPaint()) { + mIsFirstPaint = true; + } + } else { + auto firstNonEmpty = aDisplayLists[*firstScrollDataIndex].mScrollData; + // Ensure the flag is the same on all of them. + MOZ_RELEASE_ASSERT(scrollData->IsFirstPaint() == + firstNonEmpty->IsFirstPaint()); + } + } } // aScrollData is moved into this function but that is not reflected by the @@ -1026,8 +1024,10 @@ mozilla::ipc::IPCResult WebRenderBridgeParent::RecvSetDisplayList( // sent to WebRender, so that the UpdateHitTestingTree call is guaranteed to // be in the updater queue at the time that the scene swap completes. for (auto& displayList : aDisplayLists) { - UpdateAPZScrollData(wrEpoch, std::move(displayList.mScrollData), - displayList.mRenderRoot); + if (displayList.mScrollData) { + UpdateAPZScrollData(wrEpoch, std::move(displayList.mScrollData.ref()), + displayList.mRenderRoot); + } } bool validTransaction = aIdNamespace == mIdNamespace; @@ -1037,13 +1037,25 @@ mozilla::ipc::IPCResult WebRenderBridgeParent::RecvSetDisplayList( for (auto& displayList : aDisplayLists) { MOZ_ASSERT(displayList.mRenderRoot == wr::RenderRoot::Default || IsRootWebRenderBridgeParent()); - if (!SetDisplayList( - displayList.mRenderRoot, displayList.mRect, displayList.mCommands, - displayList.mContentSize, std::move(displayList.mDL), - displayList.mDLDesc, displayList.mResourceUpdates, - displayList.mSmallShmems, displayList.mLargeShmems, aTxnStartTime, - txns[displayList.mRenderRoot], senders[displayList.mRenderRoot], - wrEpoch, validTransaction, observeLayersUpdate)) { + auto renderRoot = displayList.mRenderRoot; + auto& txn = txns[renderRoot]; + + txn.SetLowPriority(!IsRootWebRenderBridgeParent()); + if (validTransaction) { + senders[renderRoot].emplace(Api(renderRoot), &txn); + } + + if (NS_WARN_IF(!ProcessWebRenderParentCommands(displayList.mCommands, txn, + renderRoot))) { + return IPC_FAIL(this, "Invalid parent command found"); + } + + if (displayList.mDL && + !SetDisplayList(renderRoot, displayList.mRect, displayList.mContentSize, + std::move(displayList.mDL.ref()), displayList.mDLDesc, + displayList.mResourceUpdates, displayList.mSmallShmems, + displayList.mLargeShmems, aTxnStartTime, txn, wrEpoch, + validTransaction, observeLayersUpdate)) { return IPC_FAIL(this, "Failed call to SetDisplayList"); } } diff --git a/gfx/layers/wr/WebRenderBridgeParent.h b/gfx/layers/wr/WebRenderBridgeParent.h index 32fd8f630a91..1bae80cbfc83 100644 --- a/gfx/layers/wr/WebRenderBridgeParent.h +++ b/gfx/layers/wr/WebRenderBridgeParent.h @@ -296,17 +296,14 @@ class WebRenderBridgeParent final } bool SetDisplayList(wr::RenderRoot aRenderRoot, const gfx::IntRect& aRect, - const nsTArray& aCommands, const wr::LayoutSize& aContentSize, ipc::ByteBuf&& aDL, const wr::BuiltDisplayListDescriptor& aDLDesc, const nsTArray& aResourceUpdates, const nsTArray& aSmallShmems, const nsTArray& aLargeShmems, const TimeStamp& aTxnStartTime, - wr::TransactionBuilder& aTxn, - Maybe& aTxnSender, - wr::Epoch aWrEpoch, bool aValidTransaction, - bool aObserveLayersUpdate); + wr::TransactionBuilder& aTxn, wr::Epoch aWrEpoch, + bool aValidTransaction, bool aObserveLayersUpdate); void UpdateAPZFocusState(const FocusTarget& aFocus); void UpdateAPZScrollData(const wr::Epoch& aEpoch, WebRenderScrollData&& aData, diff --git a/gfx/layers/wr/WebRenderLayerManager.cpp b/gfx/layers/wr/WebRenderLayerManager.cpp index a75fa2eeab68..542db776b481 100644 --- a/gfx/layers/wr/WebRenderLayerManager.cpp +++ b/gfx/layers/wr/WebRenderLayerManager.cpp @@ -238,7 +238,8 @@ bool WebRenderLayerManager::EndEmptyTransaction(EndTransactionFlags aFlags) { for (auto& stateManager : mStateManagers) { auto renderRoot = stateManager.GetRenderRoot(); if (stateManager.mAsyncResourceUpdates || - !mPendingScrollUpdates[renderRoot].empty()) { + !mPendingScrollUpdates[renderRoot].empty() || + WrBridge()->HasWebRenderParentCommands(renderRoot)) { auto updates = renderRootUpdates.AppendElement(); updates->mRenderRoot = renderRoot; if (stateManager.mAsyncResourceUpdates) { @@ -418,12 +419,15 @@ void WebRenderLayerManager::EndTransactionWithoutLayer( auto renderRootDL = renderRootDLs.AppendElement(); renderRootDL->mRenderRoot = renderRoot; builder.Finalize(*renderRootDL); - mLastDisplayListSizes[renderRoot] = renderRootDL->mDL.mCapacity; + mLastDisplayListSizes[renderRoot] = renderRootDL->mDL->mCapacity; resourceUpdates.SubQueue(renderRoot) .Flush(renderRootDL->mResourceUpdates, renderRootDL->mSmallShmems, renderRootDL->mLargeShmems); renderRootDL->mRect = RoundedToInt(rects[renderRoot]).ToUnknownRect(); - renderRootDL->mScrollData = std::move(mScrollDatas[renderRoot]); + renderRootDL->mScrollData.emplace(std::move(mScrollDatas[renderRoot])); + } else if (WrBridge()->HasWebRenderParentCommands(renderRoot)) { + auto renderRootDL = renderRootDLs.AppendElement(); + renderRootDL->mRenderRoot = renderRoot; } } diff --git a/gfx/webrender_bindings/WebRenderAPI.cpp b/gfx/webrender_bindings/WebRenderAPI.cpp index c602068c593b..185eccb6f639 100644 --- a/gfx/webrender_bindings/WebRenderAPI.cpp +++ b/gfx/webrender_bindings/WebRenderAPI.cpp @@ -730,8 +730,8 @@ void DisplayListBuilder::Finalize( wr_api_finalize_builder(SubBuilder(aOutTransaction.mRenderRoot).mWrState, &aOutTransaction.mContentSize, &aOutTransaction.mDLDesc, &dl.inner); - aOutTransaction.mDL = - ipc::ByteBuf(dl.inner.data, dl.inner.length, dl.inner.capacity); + aOutTransaction.mDL.emplace(dl.inner.data, dl.inner.length, + dl.inner.capacity); dl.inner.capacity = 0; dl.inner.data = nullptr; }