APZ target notifications can arrive before dependent layers transactions

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment)

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.
Posted patch fixSplinter Review
Attachment #8592032 - Flags: review?(bugmail.mozilla)
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/
Attachment #8592032 - Flags: review?(bugmail.mozilla) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/879e74f2e74c

w/ comments addressed - I got rid of the move semantics, I went overboard there.
https://hg.mozilla.org/mozilla-central/rev/879e74f2e74c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Hi, David,

Sorry to disturb you.

May I have your help?
This patch injected a critical bug. (Bug 1154614).
Could you solve this issue?
Flags: needinfo?(dvander)
I'll take it. There's a thread deadlocking itself by trying to acquire the same non-reentrant lock twice.
Flags: needinfo?(dvander)
Blocks: 1154751
Blocks: 1154749
No longer blocks: 1154749
Depends on: 1154749
Blocks: 1155779
No longer blocks: 1155779
You need to log in before you can comment on or make changes to this bug.