Closed Bug 1247964 Opened 8 years ago Closed 8 years ago

APZ crash on scrolling

Categories

(Core :: Panning and Zooming, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: esawin, Assigned: rbarker)

References

Details

(Keywords: crash, Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

Attached file Logcat
STR
1. Open http://stadtohnenamen.arte.tv/de (there may be localization-based redirects, re-open the noted URL in that case).
2. Wait for page to load and the animation to finish.
3. Scroll to the bottom, scroll to the top.
4. Repeat 3 until it crashes.

I've noted two different assertions:
1. F MOZ_Assert: Assertion failure: !mScrolledApzc || mScrolledApzc == aApzc, at /home/esawin/dev/g/gfx/layers/apz/src/InputBlockState.cpp:94
2. F MOZ_Assert: Assertion failure: mInTransform, at /home/esawin/dev/g/gfx/layers/apz/src/PotentialCheckerboardDurationTracker.cpp:55
Keywords: crash
Whiteboard: [gfx-noted]
QA Contact: rbarker
QA Contact: rbarker
Assignee: nobody → rbarker
This the scenario that was causing the assert:
1) page was scrolled to the end.
2) Scolling up again would cause the <embed> object to scroll onto the screen.
3) Without lifting the scrolling finger changing scroll directions would cause the assert.

The assert would be hit because changing directions would change the APZC being scrolled because while the page could not scroll up any more it could scroll down so the scrolling was being handed from the <embed> object to the page and causing the APZC being scrolled to change which triggered the assert: 

MOZ_ASSERT(!mScrolledApzc || mScrolledApzc == aApzc);

in InputBlockState::SetScrolledApzc(AsyncPanZoomController* aApzc)
Since :botond actually wrote the patch, we need some one else to review it.
Attachment #8730440 - Attachment is obsolete: true
Attachment #8730450 - Flags: review?(bugmail.mozilla)
Comment on attachment 8730450 [details] [diff] [review]
0001-Bug-1247964-Allow-InputBlockState-SetScrolledApzc-to-accept-new-APZC-when-it-is-an-ancestor-of-the-current-APZC-r-16031416-21c15fc.patch

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

To me the behaviour seems wrong (that changing scroll direction can change what you're scrolling) but if you guys think that's the desired behaviour then this patch is fine.
Attachment #8730450 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 8730450 [details] [diff] [review]
> 0001-Bug-1247964-Allow-InputBlockState-SetScrolledApzc-to-accept-new-APZC-
> when-it-is-an-ancestor-of-the-current-APZC-r-16031416-21c15fc.patch
> 
> Review of attachment 8730450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To me the behaviour seems wrong (that changing scroll direction can change
> what you're scrolling) but if you guys think that's the desired behaviour
> then this patch is fine.

After thinking about this a little I agree. I don't believe changing directions should allow the APZC to change. I would imagine that when a drag starts, it captures the APZC for the duration of the drag.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> To me the behaviour seems wrong (that changing scroll direction can change
> what you're scrolling) but if you guys think that's the desired behaviour
> then this patch is fine.

That behaviour has been in place since the beginning of support for scroll handoff. I'm open to changing it, but I think we should do it in a different bug, as this bug is a regression from the (much more recent) introduction of the "no-immediate-handoff" mode for Fennec.
Yeah that's fair, I'm ok with this landing.

Just to braindump some stuff:

Without the "no-immediate-handoff" behaviour (i.e on B2G), if you panned a subframe and reached the end, it would hand off to the parent, and if you reversed direction it would scroll the subframe the other way. This is fine and consistent with a user mental model that says "the first frame that scrolls when I pan my finger is the one that always takes priority".

However, on Fennec, with the no-immediate-handoff behaviour, this user mental model is violated. Consider the case where you pan a subframe to the end, and it doesn't get handed off because of "no-immediate-handoff". You have to lift your finger and pan again to start scrolling the parent frame - at this point, the user mental model says that the parent frame is the one that should take priority - yet on a direction switch it is the subframe that ends up taking priority. That's what I thought was confusing.

From an implementation point of view it's consistent because the model there is "the frame you put your finger down on is the one that takes priority" which is subtly different.

Then I thought about it some more and realized that user model can already be violated in the B2G behaviour, if you have a subframe that is already scrolled to the end. If you start with a subframe scrolled to the end then panning on it will hand off to the parent, and reversing direction will scroll the subframe the other way (i.e. it does the same thing as Fennec). So yeah, I think you're right and this patch should land as-is. If we want to change the implementation to what I described as the "user mental model" above we can do that in a separate bug.
Created Bug 1257264 to address the handoff issue in Fennec.
Marking this as blocking bug 1230552, since it's a regression from that.
Blocks: 1230552
https://hg.mozilla.org/mozilla-central/rev/28ec277c75ac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.