As part of the plan for fixing bug 918288, one of the things we need to do is move the input block queue code (mTouchBlockQueue, mTouchBlockBalance, etc.) out of the AsyncPanZoomController class and into the APZCTreeManager class. The reason for this is that once the event regions (bug 928833) is fully operational, regions which have touch event listeners will be included in the dispatch-to-content regions. In the APZCTreeManager, we won't know if a particular dispatch-to-content region is because of a touch-event listener or because of a non-rectangular hit region, or because of an inactive subframe. We will have to hold on to the events in the APZCTreeManager until we have resolved this dilemma (via a round-trip to the gecko thread) and only after that can we dispatch the events to the right APZC. Since we already have the code to deal with content touch responses and timing out in case of non-responsiveness and all that, we should move that into the APZCTreeManager and use it for this purpose.

My plan is to (with this patch) encapsulate the relevant code I want to move, and then (in a follow-up patch) move the hookup from AsyncPanZoomController to APZCTreeManager.

Braindumping here about some issues which are influencing how I'm implementing this bug. - I believe there is a temporal violation at [1]. Consider the case when you get inputs like touchstart, touchmove, touchmove, (2nd)touchstart. The first three events are a single input block and go to a particular APZ. Let's say those events are waiting for a content response before they can be processed. In the meantime, the last event comes in to APZCTreeManager. At this point, the call to HasOverscrolledApzc will return false at [1], and the event will be sent to the APZC. However, it's possible for that first input block (once it gets processed) to send the APZC into overscroll, and so that second touchstart should never arrive at the APZC. - In general there seems to be a bunch of stuff such as [2] and [3] that we want to do at the start of processing a touch input block, but only once the target APZ has been definitively determined. Doing it in ReceiveInput rather than HandleInput will result in temporal violations. It would probably be safer to move all this code into the InputQueue class. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=a19ccad6a934#628 [2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=a19ccad6a934#646 [3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=a19ccad6a934#653

Braindump: Something I hadn't considered was the security implications of this bug. My plan was to have a unified input event queue in APZCTM rather than a per-APZC input queue. Part of this involved removing the aOutTargetGuid parameter to APZCTM::ProcessInputEvent and the corresponding parameters to APZCTM::ContentReceivedTouch. However that's a bad idea because then it allows malicious child processes to send the wrong number of ContentReceivedTouch calls, and those might potentially interfere with other processes' input events. Even replacing the ScrollableLayerGuid with a unique-per-input-block id would be subject to the same problem (in effect, the check at [1] would no longer work). So it makes sense to leave the ScrollableLayerGuid bit as-is. Another difference with the unified input queue is there might be an input block in the queue for an APZ that gets destroyed while the input block is pending. In such a case we should be sure to remove the input block and make sure that it doesn't mess up the flow. [1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=0f5c20e2b2c2#522

Hopefuly final set of WIP patches; after this I should just need to clean up the documentation and style and verify all platforms build and don't explode.

Try push with the above patches is mostly green with a fair number of inttermittents https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c1f29c4a3822

Comment on attachment 8509575 [details] [diff] [review] Part 1 - Extract some helper functions Review of attachment 8509575 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +3028,5 @@ > } > } > > +bool > +AsyncPanZoomController::NeedToWaitForContent() const

Comment on attachment 8509576 [details] [diff] [review] Part 2 - Extract an InputQueue class from APZC Review of attachment 8509576 [details] [diff] [review]: ----------------------------------------------------------------- r- for routing events to the last APZC in the handoff chain. Otherwise the patch looks reasonable. ::: gfx/layers/apz/src/AsyncPanZoomController.h @@ +778,5 @@ > */ > void ContentReceivedTouch(bool aPreventDefault); > > /** > + * See InputQueue::SetAllowedTouchBehavior We seem to be removing important information here. For example, "must be called after receiving the TOUCH_START even that started the touch session". ::: gfx/layers/apz/src/InputQueue.cpp @@ +39,5 @@ > + > + TouchBlockState* block = nullptr; > + if (aEvent.AsMultiTouchInput().mType == MultiTouchInput::MULTITOUCH_START) { > + block = StartNewTouchBlock(aTarget, false); > + INPQ_LOG("%p started new touch block %p\n", this, block); Perhaps printing the address of aTarget would be useful here (and in other log statements where the target is available), so these outputs can be correlated with APZC_LOG outputs? @@ +231,5 @@ > + } > + > + INPQ_LOG("%p processing input block %p; preventDefault %d\n", > + this, curBlock, curBlock->IsDefaultPrevented()); > + nsRefPtr<AsyncPanZoomController> target = curBlock->GetOverscrollHandoffChain()->LastApzc(); The last APZC in the handoff chain is not the one we should be giving the events to. The current behaviour is to give the events to the APZC that received them (here, the APZC whose InputQueue 'this' is). This is usually the same APZC as the first APZC in the handoff chain, but doesn't have to be thanks to scrollgrab. It's not immediately obvious to me whether routing the events to the first APZC in the handoff chain rather than the APZC that received them would be correct. I'll think more about this. In the meantime, r- since routing them to the last APZC is definitely incorrect. ::: gfx/layers/apz/src/OverscrollHandoffState.cpp @@ +51,5 @@ > +nsRefPtr<AsyncPanZoomController> > +OverscrollHandoffChain::LastApzc() const > +{ > + if (Length() == 0) { > + return nullptr; The only caller asserts that this is not the case. Perhaps we should just assert here? That would match the semantics of |GetApzcAtIndex()| ("don't call this unless you know there's an APZC there"). We can then also return |const nsRefPtr<AsyncPanZoomController>&|. (This avoids an unnecessary refcount increment/decrement.)

(In reply to Botond Ballo [:botond] from comment #23) > The last APZC in the handoff chain is not the one we should be giving the > events to. > > The current behaviour is to give the events to the APZC that received them > (here, the APZC whose InputQueue 'this' is). This is usually the same APZC > as the first APZC in the handoff chain, but doesn't have to be thanks to > scrollgrab. > > It's not immediately obvious to me whether routing the events to the first > APZC in the handoff chain rather than the APZC that received them would be > correct. I'll think more about this. In the meantime, r- since routing them > to the last APZC is definitely incorrect. I originally had it sending to FirstApzc() but that was misbehaving on-device and it appeared to be because the handoff chain was reversed from what I was expecting. Of course, in retrospect that's obviously because of scrollgrab, but at the time I just thought I had the chain order backwards in my head. I'll see if I can figure out what the right thing to do here is, thanks for pointing it out. > ::: gfx/layers/apz/src/OverscrollHandoffState.cpp > The only caller asserts that this is not the case. Perhaps we should just > assert here? That would match the semantics of |GetApzcAtIndex()| ("don't > call this unless you know there's an APZC there"). > > We can then also return |const nsRefPtr<AsyncPanZoomController>&|. (This > avoids an unnecessary refcount increment/decrement.) Yeah that sounds reasonable. We should never have a handoff chain with no APZCs in it, right?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > We should never have a handoff chain with no APZCs in it, right? Nope, we should not.

Comment on attachment 8509579 [details] [diff] [review] Part 4 - Replace the touch-block-balance with an input block id Review of attachment 8509579 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I'll wait with r+'ing until I look at Part 5 and have a better understanding of the SetOverscrollHandoffChain bits. ::: dom/ipc/TabChild.h @@ +600,5 @@ > ScreenOrientation mOrientation; > bool mUpdateHitRegion; > bool mPendingTouchPreventedResponse; > ScrollableLayerGuid mPendingTouchPreventedGuid; > + uint64_t mPendingTouchPreventedBlockId; Let's initialize this to something (probably 0) in the constructor. It'll be one less thing for :jrmuizel to fix when he makes it an error to leave a field uninitialized without explicit use of a "yes, this is what I want" attribute. ::: gfx/layers/apz/src/InputBlockState.h @@ +37,5 @@ > protected: > bool HasOverscrollHandoffChain() const; > private: > nsRefPtr<const OverscrollHandoffChain> mOverscrollHandoffChain; > + uint64_t mBlockId; We can make this a 'const uint64_t' to indicate that its value does not change over the lifetime of the input block. ::: gfx/layers/apz/src/InputQueue.cpp @@ +186,5 @@ > } > + if (success) { > + ProcessPendingInputBlocks(); > + } else { > + NS_WARNING("INPQ received useless ContentResponseTimeout"); Does this not occur as a matter of course in the following case: - we schedule the timeout - content gets back to us promptly - we process the touch block - we remove the touch block from the queue - the timeout fires ? @@ +205,5 @@ > } > + if (success) { > + ProcessPendingInputBlocks(); > + } else { > + NS_WARNING("INPQ received useless ContentReceivedTouch"); Likewise, I would expect this to happen every time content responds late (after the timeout has fired and we have processed the block). ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ +377,5 @@ > ApzcPanNoFling(AsyncPanZoomController* aApzc, > int& aTime, > int aTouchStartY, > + int aTouchEndY, > + uint64_t *aOutInputBlockId = nullptr) nit: star goes beside type

Comment on attachment 8509580 [details] [diff] [review] Part 5 - Move to a single input queue owned by the APZCTM Review of attachment 8509580 [details] [diff] [review]: ----------------------------------------------------------------- This generally looks good. In fact, moving the InputQueue from APZC to APZCTM ended up being quite clean! I can think of an unfortunate side effect of this design, though it's a pretty edge case scenario: - Say you have two apps visible (for example, a regular app and the keyboard, or, in a hypothetical future where you can have two regular apps side by side, two regular apps). - One app, A, has content listeners, the other, B, doesn't. - You perform a touch gesture on A, and then quickly a touch gesture on B. - A's content listeners take long to respond, and/or time out. - In the previous design, this would not block the gesture on B from being processed. In the current design, it does. I'm not sure what the granularity of FrameMetrics::mMayHaveTouchListeners is - i.e. whether it can be different for different APZCs within the same app - but if it can be, then the same scenario can also happen with two frames in the same app, with the first having mMayHaveTouchListeners = true and the other having mMayHaveTouchListeners = false. Have we considered these scenarios, and evaluated possible alternatives that avoid this problem? ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +85,5 @@ > } > > APZCTreeManager::~APZCTreeManager() > { > + mInputQueue = nullptr; Is this here because mInputQueue needs to be Release()'d before a member's destructor is called? If so, add a comment and say which member. If not, this is redundant, the destructor of mInputQueue will accomplish the same thing. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ -929,5 @@ > > nsRefPtr<InputQueue> > AsyncPanZoomController::GetInputQueue() const { > - MonitorAutoLock lock(mRefPtrMonitor); > - MOZ_ASSERT(mInputQueue); Now that no one's going to null out mInputQueue, we can return it by |const nsRefPtr<InputQueue>&|. ::: gfx/layers/apz/src/AsyncPanZoomController.h @@ +95,5 @@ > static float GetTouchStartTolerance(); > > AsyncPanZoomController(uint64_t aLayersId, > APZCTreeManager* aTreeManager, > + InputQueue* aInputQueue, While it's technically OK to get the raw pointer out of an nsRefPtr and construct a new nsRefPtr out of it (the reason this is OK is that since nsRefPtr's ref-counting is intrusive, the two nsRefPtrs will share the same refcount), I'd rather we not do it and pass |nsRefPtr<InputQueue>| (or better, |const nsRefPtr<InputQueue>&|) here. Rationale: if someone ever changes the code to use, say, std::shared_ptr (which uses non-intrusive refcounting), we have a problem.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > (In reply to Botond Ballo [:botond] from comment #23) > > The last APZC in the handoff chain is not the one we should be giving the > > events to. > > > > The current behaviour is to give the events to the APZC that received them > > (here, the APZC whose InputQueue 'this' is). This is usually the same APZC > > as the first APZC in the handoff chain, but doesn't have to be thanks to > > scrollgrab. > > > > It's not immediately obvious to me whether routing the events to the first > > APZC in the handoff chain rather than the APZC that received them would be > > correct. I'll think more about this. In the meantime, r- since routing them > > to the last APZC is definitely incorrect. > > I originally had it sending to FirstApzc() but that was misbehaving > on-device and it appeared to be because the handoff chain was reversed from > what I was expecting. Of course, in retrospect that's obviously because of > scrollgrab, but at the time I just thought I had the chain order backwards > in my head. I'll see if I can figure out what the right thing to do here is, > thanks for pointing it out. Perhaps we can simply save the target APZC when we construct the TouchBlockState (and when the dispatch-to-content stuff necessitates providing the handoff chain after the fact, then provide the target together with that)?

Comment on attachment 8509578 [details] [diff] [review] Part 3 - Make the handoff chain optional, but a requirement for block handling Review of attachment 8509578 [details] [diff] [review]: ----------------------------------------------------------------- I have a hard time reasoning about these changes without seeing the call sites of InputQueue::SetOverscrollHandoffChain. If such call sites are not forthcoming in this bug, may I suggest leaving this patch out and instead putting it into the bug that does introduce them? ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +996,5 @@ > // We could probably enhance this logic to determine things like "we're > // not pannable, so we can only zoom in, and the zoom is already maxed > // out, so we're not zoomable either" but no need for that at this point. > > + bool pannable = BuildOverscrollHandoffChain()->CanBePanned(this); Can we have the caller pass in the chain rather than building a new one here?

(In reply to Botond Ballo [:botond] from comment #29) > ::: gfx/layers/apz/src/AsyncPanZoomController.cpp > @@ +996,5 @@ > > // We could probably enhance this logic to determine things like "we're > > // not pannable, so we can only zoom in, and the zoom is already maxed > > // out, so we're not zoomable either" but no need for that at this point. > > > > + bool pannable = BuildOverscrollHandoffChain()->CanBePanned(this); > > Can we have the caller pass in the chain rather than building a new one here? (Ignore this comment. It was an old comment that Bugzilla somehow remembered.)

Comment on attachment 8509579 [details] [diff] [review] Part 4 - Replace the touch-block-balance with an input block id Review of attachment 8509579 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comments in comment 26 addressed. This patch might need minor modifications after addressing review comments for other patches, but nothing significant enough to warrant re-review.

(In reply to Botond Ballo [:botond] from comment #28) > Perhaps we can simply save the target APZC when we construct the > TouchBlockState (and when the dispatch-to-content stuff necessitates > providing the handoff chain after the fact, then provide the target together > with that)? In fact, the target is the only thing that should need to provided. The TouchBlockState can then store the target, and target->BuildOverscrollHandoffChain().

(In reply to Botond Ballo [:botond] from comment #22) > Comment on attachment 8509575 [details] [diff] [review] > Part 1 - Extract some helper functions > > const Done (In reply to Botond Ballo [:botond] from comment #23) > Comment on attachment 8509576 [details] [diff] [review] > Part 2 - Extract an InputQueue class from APZC > > We seem to be removing important information here. For example, "must be > called after receiving the TOUCH_START even that started the touch session". Fixed, but a later patch takes it back out anyway because once we have the block id requirement it's implied that you need to send the TOUCH_START first (otherwise you can't have the block id). > Perhaps printing the address of aTarget would be useful here (and in other > log statements where the target is available), so these outputs can be > correlated with APZC_LOG outputs? Added logging of the target. > The last APZC in the handoff chain is not the one we should be giving the > events to. I changed it so that we only pass in the target apzc to the input block, and the input block calls BuildOverscrollHandoffChain to get the handoff chain. Client code can get back the target apzc, and so I use that instead of LastApzc()/FirstApzc(). I think this is what you meant in comment 28 and comment 32 as well, or should be equivalent. > The only caller asserts that this is not the case. Perhaps we should just > assert here? That would match the semantics of |GetApzcAtIndex()| ("don't > call this unless you know there's an APZC there"). > > We can then also return |const nsRefPtr<AsyncPanZoomController>&|. (This > avoids an unnecessary refcount increment/decrement.) I cleaned up the assert situation so that the functions contain the asserts as opposed to the call sites. Also used const nsRefPtr<APZC>& everywhere I was previously using APZC* (In reply to Botond Ballo [:botond] from comment #26) > Comment on attachment 8509579 [details] [diff] [review] > Part 4 - Replace the touch-block-balance with an input block id > > + uint64_t mPendingTouchPreventedBlockId; > > Let's initialize this to something (probably 0) in the constructor. It'll be > one less thing for :jrmuizel to fix when he makes it an error to leave a > field uninitialized without explicit use of a "yes, this is what I want" > attribute. Done > > + uint64_t mBlockId; > > We can make this a 'const uint64_t' to indicate that its value does not > change over the lifetime of the input block. Done > > + NS_WARNING("INPQ received useless ContentResponseTimeout"); > > Does this not occur as a matter of course in the following case: > > - we schedule the timeout > - content gets back to us promptly > - we process the touch block > - we remove the touch block from the queue > - the timeout fires > > ? > > > + NS_WARNING("INPQ received useless ContentReceivedTouch"); > > Likewise, I would expect this to happen every time content responds late > (after the timeout has fired and we have processed the block). > It does, good point. I removed the warning for the ContentResponseTimeout and ContentReceivedTouch functions, as there's no good way to consistently do a warning there. (The best thing I could catch easily is the case where the function is called twice with the same block id, but assuming the block doesn't get processed and removed in between). > > + int aTouchEndY, > > + uint64_t *aOutInputBlockId = nullptr) > > nit: star goes beside type Fixed (In reply to Botond Ballo [:botond] from comment #27) > Comment on attachment 8509580 [details] [diff] [review] > Part 5 - Move to a single input queue owned by the APZCTM > > I can think of an unfortunate side effect of this design, though it's a > pretty edge case scenario: > > - Say you have two apps visible (for example, a regular app and the > keyboard, > or, in a hypothetical future where you can have two regular apps side > by > side, two regular apps). > - One app, A, has content listeners, the other, B, doesn't. > - You perform a touch gesture on A, and then quickly a touch gesture on B. > - A's content listeners take long to respond, and/or time out. > - In the previous design, this would not block the gesture on B from being > processed. In the current design, it does. > > I'm not sure what the granularity of FrameMetrics::mMayHaveTouchListeners is > - i.e. whether it can be different for different APZCs within the same app - > but if it can be, then the same scenario can also happen with two frames in > the same app, with the first having mMayHaveTouchListeners = true and the > other having mMayHaveTouchListeners = false. > > Have we considered these scenarios, and evaluated possible alternatives that > avoid this problem? This shouldn't be hard to fix. If we can assume that input blocks for a given APZC are always ready for handling in the order that they are received (i.e. gecko makes them ready in the order we ask it) then we can just always process blocks as they become ready, regardless of whether they are at the head of the input queue or not. Depending on how the rest of bug 918288 goes we may not be able to assume this, though (for example we get one block that hits a dispatch-to-content region and the second one does not). In this case, we could have additional checking when a block becomes ready - if it is the frontmost block in the queue for a particular APZC, it can be processed. There's another ordering problem we may want to consider, which is the situation you described, but both A and B share the same parent, and so have overlapping/intersecting handoff chains. In this case we may want to hold off processing B until A is done processing, because it might change what happens higher up in the handoff chain. Again we can handle this by expanding the check for the target APZC to check for non-overlapping handoff chains. That is, when a block is made ready, it can be processed right away if there are no blocks ahead of it in the queue with overlapping targets or handoff chains. This shouldn't be very hard to implement if we need to do that, although I would rather hold off on it for now. > > + mInputQueue = nullptr; > > Is this here because mInputQueue needs to be Release()'d before a member's > destructor is called? If so, add a comment and say which member. If not, > this is redundant, the destructor of mInputQueue will accomplish the same > thing. Good point, removed that line. > > nsRefPtr<InputQueue> > > AsyncPanZoomController::GetInputQueue() const { > > - MonitorAutoLock lock(mRefPtrMonitor); > > - MOZ_ASSERT(mInputQueue); > > Now that no one's going to null out mInputQueue, we can return it by |const > nsRefPtr<InputQueue>&|. Done > > APZCTreeManager* aTreeManager, > > + InputQueue* aInputQueue, > > While it's technically OK to get the raw pointer out of an nsRefPtr and > construct a new nsRefPtr out of it (the reason this is OK is that since > nsRefPtr's ref-counting is intrusive, the two nsRefPtrs will share the same > refcount), I'd rather we not do it and pass |nsRefPtr<InputQueue>| (or > better, |const nsRefPtr<InputQueue>&|) here. Rationale: if someone ever > changes the code to use, say, std::shared_ptr (which uses non-intrusive > refcounting), we have a problem. Done (In reply to Botond Ballo [:botond] from comment #29) > Comment on attachment 8509578 [details] [diff] [review] > Part 3 - Make the handoff chain optional, but a requirement for block > handling > > I have a hard time reasoning about these changes without seeing the call > sites of InputQueue::SetOverscrollHandoffChain. > > If such call sites are not forthcoming in this bug, may I suggest leaving > this patch out and instead putting it into the bug that does introduce them? Fair enough. I took that patch out of the queue and rebased the others around it. I guess I only threw that in because I knew it would be needed eventually to deal with dispatch-to-content regions but you're right, it doesn't need to be in this bug. New patches coming shortly.

Updated, carrying r+

Comment on attachment 8510405 [details] [diff] [review] Part 4 - Move to a single input queue owned by the APZCTM (v2) Review of attachment 8510405 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks! We can leave the issues mentioned in comment 27 for a follow-up.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00fdb9ef3be6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e194c411165c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6efc9dfeae46 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e6dce29017 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6d24dc1b01f

Follow-up for bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d515b995286

https://hg.mozilla.org/mozilla-central/rev/00fdb9ef3be6 https://hg.mozilla.org/mozilla-central/rev/e194c411165c https://hg.mozilla.org/mozilla-central/rev/6efc9dfeae46 https://hg.mozilla.org/mozilla-central/rev/a5e6dce29017 https://hg.mozilla.org/mozilla-central/rev/b6d24dc1b01f https://hg.mozilla.org/mozilla-central/rev/7d515b995286