Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Commit 65b16b4

Browse files
aosmondaosmond
committed
Bug 1691065 - Discard invalid resource update transactions due to namespace changes. r=jrmuizel, a=pascalc
When we change the namespace, we discard all cached resources and their associated keys from the WebRender cache. As such if any transaction comes in with the old namespace ID, we can safefully discard the entire update, since we will need to recreate any that we are using anyways. This patch also adds new asserts to ensure we never get old namespace IDs for individual keys in a valid resource update. This should never happen in practice. Differential Revision: https://phabricator.services.mozilla.com/D104236
1 parent 106276a commit 65b16b4

File tree

6 files changed

+130
-55
lines changed

6 files changed

+130
-55
lines changed

‎gfx/layers/ipc/PWebRenderBridge.ipdl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ parent:
6161
nsCString txnURL, TimeStamp fwdTime,
6262
CompositionPayload[] payloads);
6363
async SetFocusTarget(FocusTarget focusTarget);
64-
async UpdateResources(OpUpdateResource[] aResourceUpdates,
64+
async UpdateResources(IdNamespace aIdNamespace, OpUpdateResource[] aResourceUpdates,
6565
RefCountedShmem[] aSmallShmems, Shmem[] aLargeShmems);
6666
async ParentCommands(WebRenderParentCommand[] commands);
6767
sync GetSnapshot(PTexture texture) returns (bool aNeedsYFlip);

‎gfx/layers/wr/RenderRootTypes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ struct DisplayListData {
3030
};
3131

3232
struct TransactionData {
33+
wr::IdNamespace mIdNamespace;
3334
nsTArray<WebRenderParentCommand> mCommands;
3435
nsTArray<OpUpdateResource> mResourceUpdates;
3536
nsTArray<RefCountedShmem> mSmallShmems;

‎gfx/layers/wr/WebRenderBridgeChild.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ void WebRenderBridgeChild::UpdateResources(
9999
nsTArray<ipc::Shmem> largeShmems;
100100
aResources.Flush(resourceUpdates, smallShmems, largeShmems);
101101

102-
this->SendUpdateResources(resourceUpdates, smallShmems,
102+
this->SendUpdateResources(mIdNamespace, resourceUpdates, smallShmems,
103103
std::move(largeShmems));
104104
}
105105

‎gfx/layers/wr/WebRenderBridgeParent.cpp

Lines changed: 108 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,11 @@ bool WebRenderBridgeParent::UpdateResources(
498498
switch (cmd.type()) {
499499
case OpUpdateResource::TOpAddImage: {
500500
const auto& op = cmd.get_OpAddImage();
501+
if (!MatchesNamespace(op.key())) {
502+
MOZ_ASSERT_UNREACHABLE("Stale image key (add)!");
503+
break;
504+
}
505+
501506
wr::Vec<uint8_t> bytes;
502507
if (!reader.Read(op.bytes(), bytes)) {
503508
return false;
@@ -507,6 +512,11 @@ bool WebRenderBridgeParent::UpdateResources(
507512
}
508513
case OpUpdateResource::TOpUpdateImage: {
509514
const auto& op = cmd.get_OpUpdateImage();
515+
if (!MatchesNamespace(op.key())) {
516+
MOZ_ASSERT_UNREACHABLE("Stale image key (update)!");
517+
break;
518+
}
519+
510520
wr::Vec<uint8_t> bytes;
511521
if (!reader.Read(op.bytes(), bytes)) {
512522
return false;
@@ -516,6 +526,11 @@ bool WebRenderBridgeParent::UpdateResources(
516526
}
517527
case OpUpdateResource::TOpAddBlobImage: {
518528
const auto& op = cmd.get_OpAddBlobImage();
529+
if (!MatchesNamespace(op.key())) {
530+
MOZ_ASSERT_UNREACHABLE("Stale blob image key (add)!");
531+
break;
532+
}
533+
519534
wr::Vec<uint8_t> bytes;
520535
if (!reader.Read(op.bytes(), bytes)) {
521536
return false;
@@ -526,6 +541,11 @@ bool WebRenderBridgeParent::UpdateResources(
526541
}
527542
case OpUpdateResource::TOpUpdateBlobImage: {
528543
const auto& op = cmd.get_OpUpdateBlobImage();
544+
if (!MatchesNamespace(op.key())) {
545+
MOZ_ASSERT_UNREACHABLE("Stale blob image key (update)!");
546+
break;
547+
}
548+
529549
wr::Vec<uint8_t> bytes;
530550
if (!reader.Read(op.bytes(), bytes)) {
531551
return false;
@@ -537,6 +557,10 @@ bool WebRenderBridgeParent::UpdateResources(
537557
}
538558
case OpUpdateResource::TOpSetBlobImageVisibleArea: {
539559
const auto& op = cmd.get_OpSetBlobImageVisibleArea();
560+
if (!MatchesNamespace(op.key())) {
561+
MOZ_ASSERT_UNREACHABLE("Stale blob image key (visible)!");
562+
break;
563+
}
540564
aUpdates.SetBlobImageVisibleArea(op.key(),
541565
wr::ToDeviceIntRect(op.area()));
542566
break;
@@ -592,6 +616,11 @@ bool WebRenderBridgeParent::UpdateResources(
592616
}
593617
case OpUpdateResource::TOpAddFontDescriptor: {
594618
const auto& op = cmd.get_OpAddFontDescriptor();
619+
if (!MatchesNamespace(op.key())) {
620+
MOZ_ASSERT_UNREACHABLE("Stale font key (add descriptor)!");
621+
break;
622+
}
623+
595624
wr::Vec<uint8_t> bytes;
596625
if (!reader.Read(op.bytes(), bytes)) {
597626
return false;
@@ -601,6 +630,12 @@ bool WebRenderBridgeParent::UpdateResources(
601630
}
602631
case OpUpdateResource::TOpAddFontInstance: {
603632
const auto& op = cmd.get_OpAddFontInstance();
633+
if (!MatchesNamespace(op.instanceKey()) ||
634+
!MatchesNamespace(op.fontKey())) {
635+
MOZ_ASSERT_UNREACHABLE("Stale font key (add instance)!");
636+
break;
637+
}
638+
604639
wr::Vec<uint8_t> variations;
605640
if (!reader.Read(op.variations(), variations)) {
606641
return false;
@@ -613,21 +648,43 @@ bool WebRenderBridgeParent::UpdateResources(
613648
}
614649
case OpUpdateResource::TOpDeleteImage: {
615650
const auto& op = cmd.get_OpDeleteImage();
651+
if (!MatchesNamespace(op.key())) {
652+
// TODO(aosmond): We should also assert here, but the callers are less
653+
// careful about checking when cleaning up their old keys. We should
654+
// perform an audit on image key usage.
655+
break;
656+
}
657+
616658
DeleteImage(op.key(), aUpdates);
617659
break;
618660
}
619661
case OpUpdateResource::TOpDeleteBlobImage: {
620662
const auto& op = cmd.get_OpDeleteBlobImage();
663+
if (!MatchesNamespace(op.key())) {
664+
MOZ_ASSERT_UNREACHABLE("Stale blob image key (delete)!");
665+
break;
666+
}
667+
621668
aUpdates.DeleteBlobImage(op.key());
622669
break;
623670
}
624671
case OpUpdateResource::TOpDeleteFont: {
625672
const auto& op = cmd.get_OpDeleteFont();
673+
if (!MatchesNamespace(op.key())) {
674+
MOZ_ASSERT_UNREACHABLE("Stale font key (delete)!");
675+
break;
676+
}
677+
626678
aUpdates.DeleteFont(op.key());
627679
break;
628680
}
629681
case OpUpdateResource::TOpDeleteFontInstance: {
630682
const auto& op = cmd.get_OpDeleteFontInstance();
683+
if (!MatchesNamespace(op.key())) {
684+
MOZ_ASSERT_UNREACHABLE("Stale font instance key (delete)!");
685+
break;
686+
}
687+
631688
aUpdates.DeleteFontInstance(op.key());
632689
break;
633690
}
@@ -650,9 +707,8 @@ bool WebRenderBridgeParent::UpdateResources(
650707
bool WebRenderBridgeParent::AddPrivateExternalImage(
651708
wr::ExternalImageId aExtId, wr::ImageKey aKey, wr::ImageDescriptor aDesc,
652709
wr::TransactionBuilder& aResources) {
653-
Range<wr::ImageKey> keys(&aKey, 1);
654-
// Check if key is obsoleted.
655-
if (keys[0].mNamespace != mIdNamespace) {
710+
if (!MatchesNamespace(aKey)) {
711+
MOZ_ASSERT_UNREACHABLE("Stale private external image key (add)!");
656712
return true;
657713
}
658714

@@ -666,9 +722,8 @@ bool WebRenderBridgeParent::UpdatePrivateExternalImage(
666722
wr::ExternalImageId aExtId, wr::ImageKey aKey,
667723
const wr::ImageDescriptor& aDesc, const ImageIntRect& aDirtyRect,
668724
wr::TransactionBuilder& aResources) {
669-
Range<wr::ImageKey> keys(&aKey, 1);
670-
// Check if key is obsoleted.
671-
if (keys[0].mNamespace != mIdNamespace) {
725+
if (!MatchesNamespace(aKey)) {
726+
MOZ_ASSERT_UNREACHABLE("Stale private external image key (update)!");
672727
return true;
673728
}
674729

@@ -682,9 +737,8 @@ bool WebRenderBridgeParent::UpdatePrivateExternalImage(
682737
bool WebRenderBridgeParent::AddSharedExternalImage(
683738
wr::ExternalImageId aExtId, wr::ImageKey aKey,
684739
wr::TransactionBuilder& aResources) {
685-
Range<wr::ImageKey> keys(&aKey, 1);
686-
// Check if key is obsoleted.
687-
if (keys[0].mNamespace != mIdNamespace) {
740+
if (!MatchesNamespace(aKey)) {
741+
MOZ_ASSERT_UNREACHABLE("Stale shared external image key (add)!");
688742
return true;
689743
}
690744

@@ -718,10 +772,8 @@ bool WebRenderBridgeParent::AddSharedExternalImage(
718772
bool WebRenderBridgeParent::PushExternalImageForTexture(
719773
wr::ExternalImageId aExtId, wr::ImageKey aKey, TextureHost* aTexture,
720774
bool aIsUpdate, wr::TransactionBuilder& aResources) {
721-
auto op = aIsUpdate ? TextureHost::UPDATE_IMAGE : TextureHost::ADD_IMAGE;
722-
Range<wr::ImageKey> keys(&aKey, 1);
723-
// Check if key is obsoleted.
724-
if (keys[0].mNamespace != mIdNamespace) {
775+
if (!MatchesNamespace(aKey)) {
776+
MOZ_ASSERT_UNREACHABLE("Stale texture external image key!");
725777
return true;
726778
}
727779

@@ -731,8 +783,10 @@ bool WebRenderBridgeParent::PushExternalImageForTexture(
731783
return false;
732784
}
733785

786+
auto op = aIsUpdate ? TextureHost::UPDATE_IMAGE : TextureHost::ADD_IMAGE;
734787
WebRenderTextureHost* wrTexture = aTexture->AsWebRenderTextureHost();
735788
if (wrTexture) {
789+
Range<wr::ImageKey> keys(&aKey, 1);
736790
wrTexture->PushResourceUpdates(aResources, op, keys,
737791
wrTexture->GetExternalImageKey());
738792
auto it = mTextureHosts.find(wr::AsUint64(aKey));
@@ -768,9 +822,9 @@ bool WebRenderBridgeParent::PushExternalImageForTexture(
768822
data.PushBytes(Range<uint8_t>(map.mData, size.height * map.mStride));
769823

770824
if (op == TextureHost::UPDATE_IMAGE) {
771-
aResources.UpdateImageBuffer(keys[0], descriptor, data);
825+
aResources.UpdateImageBuffer(aKey, descriptor, data);
772826
} else {
773-
aResources.AddImage(keys[0], descriptor, data);
827+
aResources.AddImage(aKey, descriptor, data);
774828
}
775829

776830
dSurf->Unmap();
@@ -782,9 +836,8 @@ bool WebRenderBridgeParent::UpdateSharedExternalImage(
782836
wr::ExternalImageId aExtId, wr::ImageKey aKey,
783837
const ImageIntRect& aDirtyRect, wr::TransactionBuilder& aResources,
784838
UniquePtr<ScheduleSharedSurfaceRelease>& aScheduleRelease) {
785-
Range<wr::ImageKey> keys(&aKey, 1);
786-
// Check if key is obsoleted.
787-
if (keys[0].mNamespace != mIdNamespace) {
839+
if (!MatchesNamespace(aKey)) {
840+
MOZ_ASSERT_UNREACHABLE("Stale shared external image key (update)!");
788841
return true;
789842
}
790843

@@ -842,10 +895,11 @@ void WebRenderBridgeParent::ObserveSharedSurfaceRelease(
842895
}
843896

844897
mozilla::ipc::IPCResult WebRenderBridgeParent::RecvUpdateResources(
898+
const wr::IdNamespace& aIdNamespace,
845899
nsTArray<OpUpdateResource>&& aResourceUpdates,
846900
nsTArray<RefCountedShmem>&& aSmallShmems,
847901
nsTArray<ipc::Shmem>&& aLargeShmems) {
848-
if (mDestroyed) {
902+
if (mDestroyed || aIdNamespace != mIdNamespace) {
849903
wr::IpcResourceUpdateQueue::ReleaseShmems(this, aSmallShmems);
850904
wr::IpcResourceUpdateQueue::ReleaseShmems(this, aLargeShmems);
851905
return IPC_OK();
@@ -1043,7 +1097,7 @@ bool WebRenderBridgeParent::SetDisplayList(
10431097
const nsTArray<OpUpdateResource>& aResourceUpdates,
10441098
const nsTArray<RefCountedShmem>& aSmallShmems,
10451099
const nsTArray<ipc::Shmem>& aLargeShmems, const TimeStamp& aTxnStartTime,
1046-
wr::TransactionBuilder& aTxn, wr::Epoch aWrEpoch, bool aValidTransaction,
1100+
wr::TransactionBuilder& aTxn, wr::Epoch aWrEpoch,
10471101
bool aObserveLayersUpdate) {
10481102
if (NS_WARN_IF(!UpdateResources(aResourceUpdates, aSmallShmems, aLargeShmems,
10491103
aTxn))) {
@@ -1052,37 +1106,33 @@ bool WebRenderBridgeParent::SetDisplayList(
10521106

10531107
wr::Vec<uint8_t> dlData(std::move(aDL));
10541108

1055-
if (aValidTransaction) {
1056-
if (IsRootWebRenderBridgeParent()) {
1057-
LayoutDeviceIntSize widgetSize = mWidget->GetClientSize();
1058-
LayoutDeviceIntRect rect =
1059-
LayoutDeviceIntRect(LayoutDeviceIntPoint(), widgetSize);
1060-
aTxn.SetDocumentView(rect);
1061-
}
1062-
gfx::DeviceColor clearColor(0.f, 0.f, 0.f, 0.f);
1063-
aTxn.SetDisplayList(clearColor, aWrEpoch,
1064-
wr::ToLayoutSize(RoundedToInt(aRect).Size()),
1065-
mPipelineId, aDLDesc, dlData);
1066-
1067-
if (aObserveLayersUpdate) {
1068-
aTxn.Notify(wr::Checkpoint::SceneBuilt,
1069-
MakeUnique<ScheduleObserveLayersUpdate>(
1070-
mCompositorBridge, GetLayersId(),
1071-
mChildLayersObserverEpoch, true));
1072-
}
1073-
1074-
if (!IsRootWebRenderBridgeParent()) {
1075-
aTxn.Notify(
1076-
wr::Checkpoint::SceneBuilt,
1077-
MakeUnique<SceneBuiltNotification>(this, aWrEpoch, aTxnStartTime));
1078-
}
1079-
1080-
mApi->SendTransaction(aTxn);
1109+
if (IsRootWebRenderBridgeParent()) {
1110+
LayoutDeviceIntSize widgetSize = mWidget->GetClientSize();
1111+
LayoutDeviceIntRect rect =
1112+
LayoutDeviceIntRect(LayoutDeviceIntPoint(), widgetSize);
1113+
aTxn.SetDocumentView(rect);
1114+
}
1115+
gfx::DeviceColor clearColor(0.f, 0.f, 0.f, 0.f);
1116+
aTxn.SetDisplayList(clearColor, aWrEpoch,
1117+
wr::ToLayoutSize(RoundedToInt(aRect).Size()), mPipelineId,
1118+
aDLDesc, dlData);
1119+
1120+
if (aObserveLayersUpdate) {
1121+
aTxn.Notify(
1122+
wr::Checkpoint::SceneBuilt,
1123+
MakeUnique<ScheduleObserveLayersUpdate>(
1124+
mCompositorBridge, GetLayersId(), mChildLayersObserverEpoch, true));
1125+
}
10811126

1082-
// We will schedule generating a frame after the scene
1083-
// build is done, so we don't need to do it here.
1127+
if (!IsRootWebRenderBridgeParent()) {
1128+
aTxn.Notify(wr::Checkpoint::SceneBuilt, MakeUnique<SceneBuiltNotification>(
1129+
this, aWrEpoch, aTxnStartTime));
10841130
}
10851131

1132+
mApi->SendTransaction(aTxn);
1133+
1134+
// We will schedule generating a frame after the scene
1135+
// build is done, so we don't need to do it here.
10861136
return true;
10871137
}
10881138

@@ -1111,12 +1161,11 @@ bool WebRenderBridgeParent::ProcessDisplayListData(
11111161
return false;
11121162
}
11131163

1114-
if (aDisplayList.mDL &&
1164+
if (aDisplayList.mDL && aValidTransaction &&
11151165
!SetDisplayList(aDisplayList.mRect, std::move(aDisplayList.mDL.ref()),
11161166
aDisplayList.mDLDesc, aDisplayList.mResourceUpdates,
11171167
aDisplayList.mSmallShmems, aDisplayList.mLargeShmems,
1118-
aTxnStartTime, txn, aWrEpoch, aValidTransaction,
1119-
aObserveLayersUpdate)) {
1168+
aTxnStartTime, txn, aWrEpoch, aObserveLayersUpdate)) {
11201169
return false;
11211170
}
11221171
return true;
@@ -1161,6 +1210,8 @@ mozilla::ipc::IPCResult WebRenderBridgeParent::RecvSetDisplayList(
11611210

11621211
if (!ProcessDisplayListData(aDisplayList, wrEpoch, aTxnStartTime,
11631212
validTransaction, observeLayersUpdate)) {
1213+
wr::IpcResourceUpdateQueue::ReleaseShmems(this, aDisplayList.mSmallShmems);
1214+
wr::IpcResourceUpdateQueue::ReleaseShmems(this, aDisplayList.mLargeShmems);
11641215
return IPC_FAIL(this, "Failed to process DisplayListData.");
11651216
}
11661217

@@ -1212,7 +1263,8 @@ bool WebRenderBridgeParent::ProcessEmptyTransactionUpdates(
12121263
// AsyncImagePipelineManager.
12131264
Unused << GetNextWrEpoch();
12141265

1215-
if (!UpdateResources(aData.mResourceUpdates, aData.mSmallShmems,
1266+
if (aData.mIdNamespace == mIdNamespace &&
1267+
!UpdateResources(aData.mResourceUpdates, aData.mSmallShmems,
12161268
aData.mLargeShmems, txn)) {
12171269
return false;
12181270
}
@@ -1290,6 +1342,10 @@ mozilla::ipc::IPCResult WebRenderBridgeParent::RecvEmptyTransaction(
12901342
bool scheduleComposite = false;
12911343
if (!ProcessEmptyTransactionUpdates(*aTransactionData,
12921344
&scheduleComposite)) {
1345+
wr::IpcResourceUpdateQueue::ReleaseShmems(this,
1346+
aTransactionData->mSmallShmems);
1347+
wr::IpcResourceUpdateQueue::ReleaseShmems(this,
1348+
aTransactionData->mLargeShmems);
12931349
return IPC_FAIL(this, "Failed to process empty transaction update.");
12941350
}
12951351
scheduleAnyComposite = scheduleAnyComposite || scheduleComposite;

0 commit comments

Comments
 (0)