1413666 - Gecko sticky positioning code creates sticky margins that it shouldn't

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

The gecko sticky positioning code is "mind-bendingly clever" as Markus put it, because it tries to use the difference between two rects to represent sticky ranges. It also initializes the rects using nscoord's min and max values [1] in what I can only assume is a clever attempt to avoid overflow.

Unfortunately, it turns out that the code has a slight off-by-one problem, because the YMost() of this rect (if unchanged) is (nscoord_MIN/2 + nscoord_MAX). However, the test uses nscoord_MAX/2 which is almost, but not quite, the same thing.

This results in bottom/right margins getting set on elements that shouldn't have them, and in the conversion to the webrender API these margins gets turned into Very Large Numbers which then cause problems. In particular, with the WIP series from bug 1411627 applied, scrolling down on a github review page like [3] results in an assertion failure which crashes a debug build.

[1] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/layout/generic/StickyScrollContainer.cpp#160
[2] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/layout/generic/StickyScrollContainer.cpp#288
[3] https://github.com/servo/webrender/pull/1969/commits/1c50cc04367171cc1e260d467b6a62cce5ca8325

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1413666 - Fix off-by-one error in sticky positioning code.

Review commit: https://reviewboard.mozilla.org/r/195506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195506/

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8924289 [details]
Bug 1413666 - Fix off-by-one error in sticky positioning code.

https://reviewboard.mozilla.org/r/195506/#review200722

You may want to make the bug title more specific - after all, there may be other ways in which the Gecko sticky positioning code is too clever for its own good :p

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Fine..  :p

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0540b49a6244
Fix off-by-one error in sticky positioning code. r=botond

Comment 5

[Corey Ford [:coyotebush]]

Guilty as charged :)
Some of that turned out a bit hairy, particularly on the "scroll ranges" side (vs. positioning during reflow).

I believe the MAX/MIN values only serve as infinities, for the cases where the frame is not sticky in a given direction or the containing block doesn't factor in. Since the rectangles don't leave StickyScrollContainer, it probably wouldn't be too hard to refactor to use more explicit flags for those cases.

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0540b49a6244

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Corey Ford [:coyotebush] from comment #5)
> Guilty as charged :)
> Some of that turned out a bit hairy, particularly on the "scroll ranges"
> side (vs. positioning during reflow).

Yeah, that's the part I mostly ended up dealing with, for APZ and now WebRender :)

> I believe the MAX/MIN values only serve as infinities, for the cases where
> the frame is not sticky in a given direction or the containing block doesn't
> factor in. Since the rectangles don't leave StickyScrollContainer, it
> probably wouldn't be too hard to refactor to use more explicit flags for
> those cases.

Well, there are inner/outer rectangles on the layer as well, but you're right that it probably wouldn't be too hard to refactor that and use more explicit flags. It's fairly self-contained which is nice.