Closed
Bug 1247964
Opened 8 years ago
Closed 8 years ago
APZ crash on scrolling
Categories
(Core :: Panning and Zooming, defect)
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)
15.46 KB,
text/plain
|
Details | |
2.60 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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
Updated•8 years ago
|
Blocks: fennec-aboard-apz
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
QA Contact: rbarker
Assignee | ||
Updated•8 years ago
|
QA Contact: rbarker
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Comment 6•8 years ago
|
||
(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.
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
Created Bug 1257264 to address the handoff issue in Fennec.
Comment 10•8 years ago
|
||
Marking this as blocking bug 1230552, since it's a regression from that.
Blocks: 1230552
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28ec277c75ac
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•