1154130 - APZ target notifications can arrive before dependent layers transactions

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Right now, APZ target notifications can arrive before dependent layers transactions in two ways: (1) by dispatching to the input thread, which can be dequeued before the compositor message, and (2) by dispatching to the compositor message loop, which can be dequeued before the IPC I/O thread wakes up.

This is causing sporadic failures on some event tests.

The fix I implemented is to send dependent confirmations through the compositor (via PLayerTransaction), so they are guaranteed to arrive in the right order. Another fix would be to attach the current layers sequence number, and have APZ/CompositorParent enforce the ordering.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 2

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

Comment on attachment 8592032 [details] [diff] [review]
fix

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

The patch seems good. Only thing I'm unsure about is the heavy use of mozilla::Move. I'm not that familiar with move semantics but it seems to me that in a lot of the places you're passing it around you could get away with a const-reference and it would be simpler. It's only when it actually gets copied for storage that I think using move semantics would be useful. Or is it just that if you're using it in a nested call you should always be using up the stack as well so that the contract is more explicit at the call site?

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +15,5 @@
>  #include "nsIContent.h"
>  #include "nsIDocument.h"
>  #include "nsIDOMWindow.h"
> +#include "mozilla/layers/ShadowLayers.h"
> +#include "mozilla/layers/LayerTransactionChild.h"

Move these up to be in alphabetical order

::: gfx/layers/apz/util/APZCCallbackHelper.h
@@ +153,5 @@
>  
>      /* Perform hit-testing on the touch points of |aEvent| to determine
>       * which scrollable frames they target. If any of these frames don't have
> +     * a displayport, set one.
> +     * 

nit: ws

@@ +156,5 @@
> +     * a displayport, set one.
> +     * 
> +     * If any displayports need to be set, the actual notification to APZ is
> +     * sent to the compositor, which will then post a message back to APZ's
> +     * input thread. Otherwise, the provided callback is invoked immediately.

s/input thread/controller thread/
s/provided callback/provided widget's SetConfirmedTargetAPZC method/

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/879e74f2e74c

w/ comments addressed - I got rid of the move semantics, I went overboard there.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/879e74f2e74c

Comment 5

[William Hsu [:whsu]]

Hi, David,

Sorry to disturb you.

May I have your help?
This patch injected a critical bug. (Bug 1154614).
Could you solve this issue?

Comment 6

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

I'll take it. There's a thread deadlocking itself by trying to acquire the same non-reentrant lock twice.