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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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 1

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

Attachment: Patch

Comment 2

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

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.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: alternative patch

This fixes the crashtest and passes the gtest locally.

Comment 4

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

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.

Comment 5

[Benoit Girard (:BenWa)]

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?

Comment 6

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

Sounds good to me.

Comment 7

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aaeff3424c0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8aaeff3424c0

Comment 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[bhavana bajaj [:bajaj]]

waiting for verifcation in https://bugzilla.mozilla.org/show_bug.cgi?id=1134762 that will done alone with the patch here.

Comment 11

[No-Jun Park [:njpark]]

See comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1134762#c28

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f98926a278dd
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/88b24a189901

Comment 13

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/88b24a189901