Closed Bug 1135677 Opened 6 years ago Closed 6 years ago

BaseRect::Intersect returns incorrect result in the face of overflow


(Core :: Graphics, defect)

Not set



Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: kats, Assigned: BenWa)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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
Attached patch Patch (obsolete) — Splinter Review
Attachment #8567934 - Flags: review?(bas)
Comment on attachment 8567934 [details] [diff] [review]

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.
Attachment #8567934 - Attachment is obsolete: true
Attachment #8567934 - Flags: feedback?(bas)
Assignee: bugmail.mozilla → bgirard
Attachment #8568066 - Flags: review?(bas)
Attachment #8568066 - Flags: review?(bas) → review+
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8568066 [details] [diff] [review]
alternative patch

NOTE: Please see 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.
Attachment #8568066 - Flags: approval-mozilla-b2g37?
Attachment #8568066 - Flags: approval-mozilla-b2g34?
waiting for verifcation in that will done alone with the patch here.
Flags: needinfo?(npark)
Attachment #8568066 - Flags: approval-mozilla-b2g37?
Attachment #8568066 - Flags: approval-mozilla-b2g37+
Attachment #8568066 - Flags: approval-mozilla-b2g34?
Attachment #8568066 - Flags: approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.