Closed Bug 1431130 Opened 2 years ago Closed 2 years ago

Ensure off compositor thread accesses to CompositorBridgeParent::GetIndirectShadowTree are safe

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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
Whiteboard: [gfx-noted]
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.
Attachment #8943294 - Attachment is obsolete: true
Attachment #8943336 - Flags: review?(botond)
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 aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3919edc65313
Ensure we retain the lock when accessing sLayerIndirectTree off the compositor thread. r=botond
https://hg.mozilla.org/mozilla-central/rev/3919edc65313
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.