Closed Bug 1431130 Opened 5 years ago Closed 5 years ago
Ensure off compositor thread accesses to Compositor
Bridge Parent::Get Indirect Shadow Tree are safe
APZ uses CompositorBridgeParent::GetIndirectShadowTree on the main thread (when its controller thread is set to the main thread) to get some simple state information. This introduces a race because the raw pointer returned is inside the map, and if the compositor thread mutates the map during the operation, it could become invalid or even point to another layer tree. It should hold the lock until it acquires a safe reference to the data it needs (e.g. mController).
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=377e9ac2e3c1c1c96d172e43e51b1ad06c970733&selectedJob=156893511 The try is with an older version of the patch, but it looks like I have some Android kinks to work out.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8262d6881ce54f0ede2a68facc00f282e7e2593e Green. I believe there is another potential problem here: https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/gfx/layers/wr/WebRenderScrollDataWrapper.h#162 As we use WebRenderScrollDataWrapper::GetLastChild in ReverseIterator::FirstChild, and ReverseIterator is used many times in APZCTreeManager, some of which are on the main thread. But WebRenderBridgeParent::GetScrollData asserts already (so nothing new broken with my assert), and there don't seem to be any reports so I've left it for now. Just wrapping it up in the new GetIndirectShadowTree variant would be incomplete/deceiving, as it holds onto a pointer to the scroll data inside WebRenderScrollDataWrapper.
Comment on attachment 8943336 [details] [diff] [review] 0001-Bug-1431130-Ensure-we-retain-the-lock-when-accessing.patch, v2 Review of attachment 8943336 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorBridgeParent.h @@ +374,5 @@ > */ > static LayerTreeState* GetIndirectShadowTree(uint64_t aId); > > /** > + * Lookup the indirect shadow tree for |aId|, call the lamda and return true s/lambda/function object (it doesn't have to be a lambda) @@ +377,5 @@ > /** > + * Lookup the indirect shadow tree for |aId|, call the lamda and return true > + * if found. If not found, return false. > + */ > + static bool GetIndirectShadowTree(uint64_t aId, Let's call this "CallWithIndirectShadowTree" (compare AsyncPanZoomController::CallWithLastContentPaintMetrics()).
Attachment #8943336 - Flags: review?(botond) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3919edc65313 Ensure we retain the lock when accessing sLayerIndirectTree off the compositor thread. r=botond
You need to log in before you can comment on or make changes to this bug.