937130 - APZCTM::CommonAncestor doesn't acquire the tree lock

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The CommonAncestor function in APZCTreeManager should be holding the tree lock as it walks around in the tree, but currently does not.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Also RootAPZCForLayersId and GetInputTransforms

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Needs testing

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

https://tbpl.mozilla.org/?tree=Try&rev=84cf2aded158 is looking green so far

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 830373 [details] [diff] [review]
Patch

Tested on a B2G debug build to ensure none of the assertions are tripping.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 830373 [details] [diff] [review]
Patch

Review of attachment 830373 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +447,5 @@
>                                ScrollableLayerGuid* aOutTargetGuid,
>                                WidgetInputEvent* aOutEvent)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +

ProcessTouchEvent(), ProcessMouseEvent() and ProcessEvent() look like private helpers for the WidgetInputEvent overloads of ReceiveInputEvent(), which already assert this. Is there value in asserting again in the helpers?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I added the asserts to the private helpers because (a) when looking at the function in isolation it's more obvious which thread it's running on and (b) in case code is added to invoke the private helpers from some other method the assertions might catch errors.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c592e423be9d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c592e423be9d