Closed Bug 1161040 Opened 9 years ago Closed 9 years ago

Possible infinite loop in BuildOverscrollHandoffChain

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

Jim reports at https://bugzilla.mozilla.org/show_bug.cgi?id=1157264#c3 that scrolling with the mouse wheel hangs the browser. The crash report there implies an infinite loop in APZCTreeManager::BuildOverscrollHandoffChain, which is something I ran into as well locally. I only saw it once and thought it was just my debugger playing tricks on me, but now I suspect it might be a legitimate regression from bug 1159405. I will investigate.
Attached patch Part 2 - The actual fix (obsolete) — Splinter Review
Attachment #8600926 - Flags: review?(tnikkel)
No functional changes intended here but hopefully this makes it easier to follow the different cases.
Attachment #8600927 - Flags: review?(tnikkel)
Comment on attachment 8600926 [details] [diff] [review]
Part 2 - The actual fix

So it seems like the problem is that we will set the same scroll id again when we build display list for the root scroll frame?

If that's the case then wouldn't a better fix be to just check if the document has a root scroll frame (that will then set the same scroll id)?
Attachment #8600926 - Flags: review?(tnikkel)
Attachment #8600927 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #4)
> So it seems like the problem is that we will set the same scroll id again
> when we build display list for the root scroll frame?

Yeah.

> If that's the case then wouldn't a better fix be to just check if the
> document has a root scroll frame (that will then set the same scroll id)?

I was considering that but I couldn't convince myself that the logic in nsGfxScrollFrame::BuildDisplayList would always do that because of the conditions at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=ade50933c777#2762. If you think that's a more correct fix I can do that instead.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> (In reply to Timothy Nikkel (:tn) from comment #4)
> > If that's the case then wouldn't a better fix be to just check if the
> > document has a root scroll frame (that will then set the same scroll id)?
> 
> I was considering that but I couldn't convince myself that the logic in
> nsGfxScrollFrame::BuildDisplayList would always do that because of the
> conditions at
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp?rev=ade50933c777#2762. If you think that's a more
> correct fix I can do that instead.

The code you are changing is in the else branch of an if that checks for ignore viewport scrolling, that should guarantee we don't take that path.
Attached patch Part 2 - FixSplinter Review
Attachment #8600926 - Attachment is obsolete: true
Attachment #8600927 - Attachment is obsolete: true
Attachment #8601002 - Flags: review?(tnikkel)
Attachment #8601002 - Flags: review?(tnikkel) → review+
I landed part 2 since it should fix the browser hang with APZ. Flagging leave-open for part 1.
Keywords: leave-open
Attachment #8600923 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/376c53019483
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1189443
You need to log in before you can comment on or make changes to this bug.