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

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
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

Updated

2 years ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee

Comment 1

2 years ago
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.
Assignee

Comment 2

2 years ago
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+

Comment 4

Last year
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

Comment 5

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/3919edc65313
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.