Closed Bug 981029 Opened 6 years ago Closed 6 years ago

GetTransformToLastDispatchedPaint might be incorrect if the paint request was partially or fully rejected

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(5 files, 5 obsolete files)

43.31 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.52 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.06 KB, patch
kats
: review+
Details | Diff | Splinter Review
11.93 KB, patch
kats
: review+
Details | Diff | Splinter Review
55.97 KB, patch
Details | Diff | Splinter Review
There are some cases where APZC will send a repaint request with a new scroll offset and/or zoom, but the code in APZCCallbackHelper will not apply the scroll offset. In such cases, the GetTransformToLastDispatchedPaint function in APZ will return the wrong value. Well actually it returns what the name says, but there's a broken assumption at the call site that assumes the transform will always map to the state of gecko at the time the input event is received there.

In practice this almost never gets hit and is only a transient state. However with the patches for bug 975962 this causes a reproducible problem where you can zoom in on an overflow:hidden page and try to tap links, but the wrong links will get tapped. Commenting out the use sites of GetTransformToLastDispatchedPaint fixes the problem, but would regress the issue fixed in bug 947337.
STR:
1) Apply patches from bug 975962
2) Load http://people.mozilla.org/~kgupta/bug/981029.html in the B2G browser
3) Zoom in and pan down/right as far as you can go
4) Click on one of the grid links

Expected:
The grid link you click on ends up in the top-left

Actual:
Some other grid link ends up in the top-left.
Do we need this on 1.3?
Yeah, I think so. I want to actually write a fix for this problem before I uplift bug 975962, so that they can both be uplifted together.
blocking-b2g: --- → 1.3?
I discussed this with Botond a bit on Friday but want to record my thoughts here as well:

I think that conceptually GetTransformToLastDispatchedPaint is correct, because it maintains a consistent coordinate system across the GeckoContentController interface. That is, the messages the APZC sends to the GeckoCC are consistent with respect to the previous message. The problem here is on the GeckoCC implementation side, because that is where sometimes the repaint requests get rejected or ignored. And therefore, I think the correct fix is to track when that happens and adjust the incoming input events accordingly.

What this means in terms of the fix is that TabChild needs to record the difference between the requested scroll offset and the actual scroll offset after processing a repaint request, and then (un)apply that difference to subsequent incoming input events (touch events, clicks, etc.). Note that in theory it might also be able to pass that difference back to the APZC but since this involves an IPC message with potentially large latency it doesn't seem like a good solution because other things might get out of sync during that time. I think it's better to do this directly in the child process.
Assignee: nobody → bugmail.mozilla
Attached patch Part 3 (WIP) (obsolete) — Splinter Review
I didn't run into cases where this is strictly needed, but it helps to reduce propagated rounding error and simplifies the code in part 4 because I don't have to keep converting between CSSPoint and CSSIntPoint.
Attachment #8388760 - Attachment is obsolete: true
Attachment #8389159 - Flags: review?(botond)
From The Big Comment On GetInputTransforms it is clear (to me at least) that inputs to the LD transform are going to be in layer pixels, because they just got transformed by LT.Inverse(). So the translation component of LD should be in layer pixels. I also walked through some numerical examples I ran into in order to convince myself that this is correct.
Attachment #8389162 - Flags: review?(botond)
Attachment #8388775 - Attachment is obsolete: true
Attachment #8389163 - Flags: review?(botond)
(Whoops, part 4 needs an extra |typedef mozilla::layers::ScrollableLayerGuid ScrollableLayerGuid;| in APZCCallbackHelper.h to compile. Made the change locally)
blocking-b2g: 1.3? → 1.3+
Comment on attachment 8389158 [details] [diff] [review]
Part 1 (refactoring) - Stop updating mLastRootMetrics in ProcessUpdateFrame

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

I'm not a huge fan of in-out parameters and I'd prefer that they be used sparingly. Would it be unduly costly to have the method return the new FrameMetrics by value?
Attachment #8389159 - Flags: review?(botond) → review+
Attachment #8389162 - Flags: review?(botond) → review+
Comment on attachment 8389163 [details] [diff] [review]
Part 4 - Save and apply an input transform on the content side

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

Looks good overall! A couple of comments:

 - Do TabChild::RecvMouseEvent/RecvRealMouseEvent/RecvMouseWheelEvent
   need to use the transform as well?

 - It would be great to have a more descriptive name than "input transform".
   I don't pretend to have any great ideas though. Perhaps "refused APZ
   scroll transform", as in a transform to account for the portion of an
   APZ scroll that we refused, or something like that?

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +324,5 @@
>  }
>  
> +static void
> +DestroyCSSPoint(void* aObject, nsIAtom* aPropertyName,
> +                void* aPropertyValue, void* aData)

I see several other places in the code where we do this, just for different types. Perhaps we can take this opportunity to introduce a DeleteObject template in an appropriate place and use an instance of that here?

::: widget/xpwidgets/APZCCallbackHelper.h
@@ +70,5 @@
> +
> +    /* Save an "input transform" property on the content element corresponding to
> +       the scrollable content. This is needed because in some cases when the APZ code
> +       sends a paint request via the GeckoContentController interface, we do not
> +       honor all of it. Since the APZ code doesn't know about our dishonorable actions,

I find "do not honor all of it" confusing. Can we be more specific and say "don't always accept the scroll offset that APZ requested"?
Attachment #8389163 - Flags: review?(botond) → review+
Comment on attachment 8389158 [details] [diff] [review]
Part 1 (refactoring) - Stop updating mLastRootMetrics in ProcessUpdateFrame

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

r=me with the suggested change of returning FrameMetrics by value in ProcessUpdateFrame.
Attachment #8389158 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #14)
>  - Do TabChild::RecvMouseEvent/RecvRealMouseEvent/RecvMouseWheelEvent
>    need to use the transform as well?

MouseEvent does, fixed. WheelEvent doesn't since it doesn't go through the APZ.

>  - It would be great to have a more descriptive name than "input transform".
>    I don't pretend to have any great ideas though. Perhaps "refused APZ
>    scroll transform", as in a transform to account for the portion of an
>    APZ scroll that we refused, or something like that?

The best I could come up with here is "APZ callback transform". I didn't want to make it too specific in terms of describing exactly what it is because that makes it too verbose and also means it'll be incorrect sooner (if this code changes). Not really happy with any of the names I came up with though, I'll sleep on it.

> >  
> > +static void
> > +DestroyCSSPoint(void* aObject, nsIAtom* aPropertyName,
> > +                void* aPropertyValue, void* aData)
> 
> I see several other places in the code where we do this, just for different
> types. Perhaps we can take this opportunity to introduce a DeleteObject
> template in an appropriate place and use an instance of that here?

Good idea. I'll file a follow-up bug for this (would make a good first mentored bug for somebody).

> > +       sends a paint request via the GeckoContentController interface, we do not
> > +       honor all of it. Since the APZ code doesn't know about our dishonorable actions,
> 
> I find "do not honor all of it" confusing. Can we be more specific and say
> "don't always accept the scroll offset that APZ requested"?

Done.
Made ProcessUpdateFrame return a FrameMetrics by value. Carrying r+
Attachment #8389158 - Attachment is obsolete: true
Attachment #8389877 - Flags: review+
Renamed some stuff, also transformed the point on the mouse event. Did that by adding an overload to APZCCallbackHelper to deal with nsIntPoint.

Pushed the full set of patches to try at https://tbpl.mozilla.org/?tree=Try&rev=94a5a8dbcb8f to make sure there's no issues.
Attachment #8389163 - Attachment is obsolete: true
Attachment #8389878 - Flags: review+
Please request approval-mozilla-b2g28 on these patches when you feel it is ready for uplift to 1.3.
NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 975962
User impact if declined: If bug 975962 is applied but not this one, then you will be able to zoom in and pan around the visible area of overflow:hidden pages, but touch events and clicks will not go to the right place
Testing completed: locally
Risk to taking this patch (and alternatives if risky): Medium risk; if we take bug 975962 then we should take this one too. If we don't take that then this one isn't really needed (although technically still fixes a transient issue).
String or UUID changes made by this patch: none
Attachment #8390820 - Flags: approval-mozilla-b2g28?
Attachment #8390820 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
You need to log in before you can comment on or make changes to this bug.