APZCTreeManager::GetTargetAPZC(point) can return non-null even if an overscrolled APZC was hit

RESOLVED FIXED in mozilla33

Status

()

Core
Panning and Zooming
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

({perf})

Trunk
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [c=effect p= s=2014.07.18.t u=])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
My intention in the changes I made to APZCTreeManager::GetTargetAPZC() in the Part 9 patch for bug 998025 is that it return nullptr if an overscrolled APZC was matched.

Mason pointed out that he sometimes saw non-nullptr being returned even if an overscrolled APZC was matched, because after the overscrolled APZC is matched and the 'result' variable is set to true [1], we continue looping and can potentially match another APZC.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=764a33b3eca8#1162
(Assignee)

Comment 1

4 years ago
Created attachment 8454073 [details] [diff] [review]
bug1037191.patch
Assignee: nobody → botond
Attachment #8454073 - Flags: review?(bugmail.mozilla)
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P1
Whiteboard: [c=effect p= s= u=]
Comment on attachment 8454073 [details] [diff] [review]
bug1037191.patch

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

Hey Botond, thanks for getting to this so fast! I tested it and I can still sometimes get mInOverscrolledApzc and mApzcInputBlock->IsOverscrolled in different states. I instrumented here - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#497. Thanks!

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +931,5 @@
>    MonitorAutoLock lock(mTreeLock);
>    nsRefPtr<AsyncPanZoomController> target;
>    // The root may have siblings, so check those too
>    gfxPoint point(aPoint.x, aPoint.y);
> +  bool inOverscrolledApzc;

This should be initialized to false, just tested on a nexus 4 and it can sometimes cause issues.
Comment on attachment 8454073 [details] [diff] [review]
bug1037191.patch

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

r=me with the comment below and mason's comment also addressed.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1115,5 @@
>    // This walks the tree in depth-first, reverse order, so that it encounters
>    // APZCs front-to-back on the screen.
>    for (AsyncPanZoomController* child = aApzc->GetLastChild(); child; child = child->GetPrevSibling()) {
>      AsyncPanZoomController* match = GetAPZCAtPoint(child, hitTestPointForChildLayers, aOutInOverscrolledApzc);
> +    if (match || *aOutInOverscrolledApzc) {

I would prefer if the code here read like this:

if (*aOutInOverscrolledApzc) {
  // we matched an overscrolled APZC, abort
  return nullptr;
}
if (match) {
  ...
Attachment #8454073 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

4 years ago
greentry
Created attachment 8454625 [details] [diff] [review]
bug1037191.patch

Updated patch to address review comments. Carrying r+.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2a977fe44123
Attachment #8454073 - Attachment is obsolete: true
Attachment #8454625 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/797f916f7380
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Updated

4 years ago
Whiteboard: [c=effect p= s= u=] → [c=effect p= s=2014.07.18.t u=]
Botond, does this need uplifting to 2.0?
(Assignee)

Comment 8

4 years ago
I believe we only ran into an actual problem when the absence of this fix was combined with Mason's patches from bug 930939. That said, the code before this fix is wrong, and the fix is low-risk, so I wouldn't be opposed to uplifting it.
(Assignee)

Updated

4 years ago
Blocks: 1046013
(Assignee)

Comment 9

4 years ago
[Blocking Requested - why for this release]:

[Feature/regressing bug #]: 
  Overscrolling (bug 1020045).

[User impact if declined]: 
  On some pages APZ hit testing can give incorrect results, leading
  to an element that is supposed to scroll not scrolling, or the
  wrong element is scrolling. I'm not aware of any pages that
  actually run into this without bug 930939 (which hasn't landed
  yet and will not be landing on 2.0), but they could exist.

[Describe test coverage new/current, TBPL]: 
  On master for 4 weeks, caused 1 regression (bug 1046013)
  which was found and fixed 2 weeks ago.

[Risks and why]: 
  Low risk. This only affects hit testing in some edge-case scenarios.

[String/UUID change made/needed]: 
  None.

Note: If this is uplifted, the fix in bug 1046013 needs to be uplifted with it (it's a 2-line fix). I can prepare a combined patch for simplicity.
(Assignee)

Comment 10

4 years ago
Flag change got overwritten. See previous comment.
blocking-b2g: --- → 2.0?
Discussed in triage, and the win doesn't outweigh the risk of taking the change on the branch. No broken new feature, no regression of existing feature, no dataloss, so not blocking.

Updated

4 years ago
blocking-b2g: 2.0? → backlog
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.