Closed Bug 1135677 Opened 6 years ago Closed 6 years ago
Rect::Intersect returns incorrect result in the face of overflow
As part of fixing bug 1134762 I uncovered the following bug: nsRect aDirtyRect is x=1073741344, y=1073741344, w=1073756696, h=1073819936 nsRect mScrollPort is x=1073741820, y=1073741820, w=14400, h=77640 dirtyRect = aDirtyRect.Intersect(mScrollPort) dirtyRect is expected to be x=1073741820, y=1073741820, w=14400, h=77640 dirtyRect is actually x=1073741820, y=1073741820, w=1073756220, h=1073819460
Comment on attachment 8567934 [details] [diff] [review] Patch Actually this patch isn't right because there may be legitimate cases where XMost() < 0 (for example if the x coord is negative). However I'd still like feedback on whether this sort of fix is reasonable. It looks like a lot of code in BaseRect is subject to problems when XMost() overflows and I'm not sure if we want a more systemic approach to dealing with this.
Attachment #8567934 - Flags: review?(bas) → feedback?(bas)
This fixes the crashtest and passes the gtest locally.
Attachment #8568066 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8568066 [details] [diff] [review] alternative patch Review of attachment 8568066 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable to fix this specific problem and I'm fine with landing this but I think overall clamping XMost()/YMost() as you suggested might be a better approach.
Attachment #8568066 - Flags: feedback?(bugmail.mozilla) → feedback+
Perhaps we should do the XMost/YMost clamp change separately from this. This has very limited possible side effect and regression risk. It's hard to reason about the potential risk and side effect of XMost/YMost which I default to high/unknown. We can land this patch as a direct fix for the APZ and uplift and consider the clamping as a general safety change we can apply (and backout if required) separately. What do you think?
Sounds good to me.
Assignee: bugmail.mozilla → bgirard
Attachment #8568066 - Flags: review?(bas) → review+
Comment on attachment 8568066 [details] [diff] [review] alternative patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): not a regression, but it exposes a flaw with bug 1134762 and bug 950934 together. User impact if declined: can't uplift bug 1134762 on branches where bug 950934 is landed/enabled. Testing completed: local testing, added a unit test (which we only run on desktop). Risk to taking this patch (and alternatives if risky): low risk, reordering '-' and min operation. String or UUID changes made by this patch: none I think we can get away with applying this on fewer branches, since this patch is only needed on branch that have root APZ enabled, but this is a low risk patch (reorders operation to reduce overflow bugs) so I'm suggesting we just uplift this everywhere we're going to take bug 1134762. This will reduce the changes of backouts.
waiting for verifcation in https://bugzilla.mozilla.org/show_bug.cgi?id=1134762 that will done alone with the patch here.
See comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1134762#c28
You need to log in before you can comment on or make changes to this bug.