1283826 - Heap-buffer-overflow in mozilla::gfx::TiledRegionImpl::AddRect

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Atte Kettunen]

Attachment: firefox-heap-buffer-overflow-mozillagfxTiledRegionImplAddRect.svg

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1467364703/


ASAN-trace:

==24606==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000590080 at pc 0x7efcd8efde1a bp 0x7ffc5e1a03b0 sp 0x7ffc5e1a03a8
WRITE of size 8 at 0x619000590080 thread T0 (Web Content)
    #0 0x7efcd8efde19 in mozilla::gfx::TiledRegionImpl::AddRect(pixman_box32 const&)::$_0::operator()(nsTArray<pixman_box32>&, unsigned long&, mozilla::gfx::TileRange) const /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:291:26
    #1 0x7efcd8edb120 in ProcessIntersectedTiles<(lambda at /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:284:5), (lambda at /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:295:5), nsTArray<pixman_box32> > /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:231:11
    #2 0x7efcd8edb120 in mozilla::gfx::TiledRegionImpl::AddRect(pixman_box32 const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:283
    #3 0x7efcd916696a in mozilla::gfx::TiledRegion<mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> >::Add(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/gfx/TiledRegion.h:122:12
    #4 0x7efcd915a953 in mozilla::layers::BasicPaintedLayer::InvalidateRegion(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/layers/basic/BasicPaintedLayer.h:57:5
    #5 0x7efcddb8bcfe in InvalidatePostTransformRegion /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:1734:3
    #6 0x7efcddb8bcfe in mozilla::FrameLayerBuilder::ComputeGeometryChangeForItem(mozilla::FrameLayerBuilder::DisplayItemData*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:4384
    #7 0x7efcddb8a29e in mozilla::FrameLayerBuilder::WillEndTransaction() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:1898:7
    #8 0x7efcddbafd18 in mozilla::FrameLayerBuilder::AddPaintedDisplayItem(mozilla::PaintedLayerData*, nsDisplayItem*, mozilla::DisplayItemClip const&, mozilla::ContainerState&, mozilla::LayerState, nsPoint const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:4480:7
    #9 0x7efcddb9571a in FinishPaintedLayerData<(lambda at /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:2745:50)> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:3040:5
    #10 0x7efcddb9571a in mozilla::PaintedLayerDataNode::PopPaintedLayerData() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/FrameLayerBuilder.cpp:2745
.
.
.
0x619000590080 is located 0 bytes to the right of 1024-byte region [0x61900058fc80,0x619000590080)
allocated by thread T0 (Web Content) here:
    #0 0x4b553e in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:71:3
    #1 0x7efcd6bd646d in Realloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray.h:169:12
    #2 0x7efcd6bd646d in nsTArrayFallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayFallibleAllocator>(unsigned long, unsigned long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:183
    #3 0x7efcd8efd7dd in InsertSlotsAt<nsTArrayFallibleAllocator> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:286:3
    #4 0x7efcd8efd7dd in InsertElementsAt<nsTArrayFallibleAllocator> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray.h:1828
    #5 0x7efcd8efd7dd in InsertElementsAt /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray.h:1849
    #6 0x7efcd8efd7dd in mozilla::gfx::TiledRegionImpl::AddRect(pixman_box32 const&)::$_0::operator()(nsTArray<pixman_box32>&, unsigned long&, mozilla::gfx::TileRange) const /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:285
    #7 0x7efcd8edb120 in ProcessIntersectedTiles<(lambda at /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:284:5), (lambda at /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:295:5), nsTArray<pixman_box32> > /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:231:11
    #8 0x7efcd8edb120 in mozilla::gfx::TiledRegionImpl::AddRect(pixman_box32 const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/TiledRegion.cpp:283
    #9 0x7efcd916696a in mozilla::gfx::TiledRegion<mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> >::Add(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/gfx/TiledRegion.h:122:12
    #10 0x7efcd915a953 in mozilla::layers::BasicPaintedLayer::InvalidateRegion(mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/layers/basic/BasicPaintedLayer.h:57:5
.
.
.

Comment 1

[Daniel Veditz [:dveditz]]

I'm assuming this crash belongs to "graphics" because it's in TiledRegionImpl and that SVG ("layout") just happens to be the testcase and isn't the cause.

Comment 2

[Daniel Holbert [:dholbert]]

Markus, looks like you added the TiledRegion class (in bug 1236043), which is implicated here -- could you take a look?

Comment 3

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

I can repro a content-process ASAN crash (unsymbolicated) if I use the build linked in comment 0 to load the attachment in comment, and zoom in (ctrl++). I'll do a local ASAN build to see if I can repro and debug it there.

Comment 4

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

Hm. I can consistently reproduce it using the try build but I can't reproduce it with my local ASan build.

Comment 5

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

Quick update: I'm trying again with a newer clang to build the ASan build. I also looked in about:buildconfig of the try build and compared it to the mozconfig I was using (from [1]). I noticed that the try build has a few differences like no --disable-jemalloc and it had some extra options as well. I modified my mozconfig as best I could to match that.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer#ManualBuild

Comment 6

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

I can repro on a local build using clang built from rev 266460. I've updated the MDN page with the new version. I caught the ASan violation in rr so I should be able to track it down relatively easily although probably won't get to it today.

Comment 7

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

Here's a reduced gtest that demonstrates the problem:

TEST(Gfx, TiledRegionOverflow) {
  TiledIntRegion tiledRegion;
  tiledRegion.Add(nsIntRect(-128, 128, 768, 2304));
  tiledRegion.Add(nsIntRegion(nsIntRect(-2147483648, -128, -2147481088, 768)));
}

It's basically an overflow/underflow problem, where the TileRange.Length() returns a "sane" number like 40 and then iterating from the Begin() to the End() of the TileRange produces many more tiles than 40.

We should definitely robustify TiledRegion against this sort of thing, but there's probably badness farther up the stack (in the region code) as well because we're producing this region with a negative width somewhere and that seems like something we shouldn't do. For example, the following gtest (very similar to the one above, but without the nsIntRegion wrapper) doesn't have the same problem:

TEST(Gfx, TiledRegionOverflow) {
  TiledIntRegion tiledRegion;
  tiledRegion.Add(nsIntRect(-128, 128, 768, 2304));
  tiledRegion.Add(nsIntRect(-2147483648, -128, -2147481088, 768));
}

Comment 8

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

Oh, the reason it works with nsIntRect and not nsIntRegion is because of the IsEmpty() check. nsIntRect(-2147483648, -128, -2147481088, 768).IsEmpty() returns true, so if we try to add the rect directly it early-exits. The nsIntRegion version just asserts [1] but since I had an opt build it went right through there. If I make that a release assert it trips and crashes.

[1] http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/gfx/src/TiledRegion.h#113

Comment 9

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

Attachment: WIP

With enough casting I can avoid the heap-buffer-overflow, but I'm not convinced it's the right solution.

Comment 10

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

Attachment: Patch

I tried handle negative-width rects in a sane way but it ended up being quite complicated and counter-intuitive. Considering the AddRect(RectT) version was already discarding negative-width rects, I decided to do the same for the AddRect(RegionT) version, and then just handle the overflow cases sanely. There's still inconsistencies between the rect and region versions, because of how the nsRegion implementation works.

Comment 11

[Markus Stange [:mstange]]

Do you know how the negative-width rect came into existence?

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8769755 [details] [diff] [review]
Patch

Review of attachment 8769755 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you.

Generating negative-size rects in any place is still a bug, which we should fix separately, but making TiledRegion more robust against buggy input is worth doing anyway.

::: gfx/src/TiledRegion.cpp
@@ +134,5 @@
>      }
> +    int64_t numberOfFullRows = (((int64_t)mEnd.y - (int64_t)mStart.y) / kTileSize) - 1;
> +    int64_t tilesInFirstRow = ((int64_t)mTileBounds.x2 - (int64_t)mStart.x) / kTileSize;
> +    int64_t tilesInLastRow = ((int64_t)mEnd.x - (int64_t)mTileBounds.x1) / kTileSize;
> +    int64_t tilesInFullRow = ((int64_t)mTileBounds.x2 - (int64_t)mTileBounds.x1) / kTileSize;

Heh, this is closer to what I had in the beginning, until Jeff asked me to do the " / kTileSize" operation only once. But this time we have actual evidence that this is better.

Comment 13

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

Attachment: Backtrace to negative-width rect

(In reply to Markus Stange [:mstange] from comment #11)
> Do you know how the negative-width rect came into existence?

Not exactly. Although I can repro the bug trivially and even catch it in rr, I haven't been able to convince clang to produce useful debug information. I have a backtrace (attached) if that helps at all, but I can't inspect any variables. If you want I could try harder to bend clang to my will, and/or just printf-debug stuff.

Comment 14

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

So actually I can't seem to reproduce it on a debug build - I see 9-digit widths, but nothing that overflows into negative. Not that it matters because I still can't inspect variables.

Anyway, I printf'd a few of the stack frames. It looks like at [1] the geometry->ComputeInvalidationRegion() value is (x=-503316480, y=-30, w=503317080, h=180) and then at [2] the layerData->mXScale and mYScale values are 256.0 and seems to cause it to overflow.

[1] http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/layout/base/FrameLayerBuilder.cpp#4310
[2] http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/layout/base/FrameLayerBuilder.cpp#4385

Comment 15

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

Also suspiciously a few lines up in the output I see the same thing but with 16.0 as the mXScale and mYScale values. Might this be related to that bug Botond fixed recently with things not properly getting cleared in the layer tree and additional layers getting added per paint? It seems like maybe the scale got cumulatively applied to itself here without getting cleared in between.

Comment 16

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

Comment on attachment 8769755 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not very easily. The patch indicates that there is an overflow/underflow issue but the attacker would have to figure out how to construct a test case that trickles the right values through the layout system to that class.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch makes it fairly obvious that there's overflow/underflow issues in the code, but it shouldn't really be obvious that there's a heap-buffer-overflow problem.

Which older supported branches are affected by this flaw?
The code in question was added in bug 1236043, so 48+

If not all supported branches, which bug introduced the flaw?
Bug 1236043, Gecko 48+

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should apply pretty cleanly as the code is the same on 48/49/50.

How likely is this patch to cause regressions; how much testing does it need?
It may cause different behaviour in the edge cases that trigger the overflow, but I wouldn't really call those regressions, just changing from one "undefined behaviour" to another.

Comment 17

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> then at [2] the layerData->mXScale and mYScale
> values are 256.0 and seems to cause it to overflow.

combined.ScaleToOutsidePixels calls nsRegion::ScaleToOutsidePixels which calls nsRect::ScaleToOutsidePixels which has a comment "// Avoid negative widths and heights due to overflow". Sounds like it's not actually succeeding at that?

Comment 18

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

This is basically what ends up happening:

{
  nsRegion r(nsRect(-503316480,-30,503317080,180));
  nsIntRegion s = r.ScaleToOutsidePixels(256.000000,256.000000,60);
  printf_stderr("scaled: %s\n", Stringify(s).c_str());
}

and the output:

scaled: < (x=-2147483648, y=-128, w=-2147481088, h=768); >

I believe the nsRect scaling is ok, but using XMost() in RectToBox is probably not the best idea when x == INT_MIN.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Can you make and nominate Aurora and Beta patches as well so it can go in there following trunk checkin?

Comment 20

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

Comment on attachment 8769755 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1236043
[User impact if declined]: Potential heap overflow when TiledIntRegion is handed screwy rects (which apparently can happen sometimes)
[Describe test coverage new/current, TreeHerder]: added some gtests to cover this and related problems
[Risks and why]: Low risk of regressions, but it's possible I missed some cases and there are still overflow possibilities lurking in this code.
[String/UUID change made/needed]: none

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7af9f4cf7ed

This patch applies cleanly to aurora and beta as well.

Comment 22

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

I filed bug 1286068 for fixing nsRegion::ScaleToOutsidePixels.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fec701de1290 for GTest failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=31593126&repo=mozilla-inbound

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7af9f4cf7ed

Comment 25

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

Reopening since it was backed out. Not entirely sure what's going on here, the gtests passed for me locally, but I'll look into it.

Comment 26

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

So it passes on clang opt builds. The MOZ_ASSERT trips on debug builds (which I apparently failed to test locally). And looking at the code I don't know what I was thinking - it clearly doesn't handle overflows properly still :/

Comment 27

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

I updated the patch to throw out overflowing rects, added some more hardening against overflow in a few places. Now it passes in my regular local gcc debug build, but it's failing on try with the ASAN build [1]. And not in an ASAN failure, just a test failure. I suspect I've inadvertently wandered into C++ no-man's land where clang and gcc and doing different things.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=88c38788e35c&selectedJob=23744189

Comment 28

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

Comment on attachment 8769755 [details] [diff] [review]
Patch

Dropping uplift request for now.

Comment 29

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

Attachment: Patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=839780ab7038&group_state=expanded is green, finally.

This version of the patches tries to discard anything invalid as early as possible. If mBounds overflows it does a FallbackToBounds which seems to be the safest thing, since then we avoid doing any more operations (other than Union) on the mBounds. Even with all this, the RoundUpToMultiple can overflow, and I added some code to handle that scenario.

Comment 30

[Markus Stange [:mstange]]

I'm not happy about UndoOverflow at all. Can we prevent mTileBounds from overflowing?

Comment 31

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

We can detect it, but I'm not sure how to handle that case. What we would do instead of having mTileBounds overflow? If we clamp it to INT_MAX it's no longer tile-aligned. I could just bail out of ProcessIntersectedTiles with IterationEndReason::STOPPED or something like that. It might give slightly wonky results for the Intersects/Contains functions but I guess it might be cleaner overall, since the results in these cases are pretty much garbage anyway.

Also it looks like the try push above failed on OS X. :/

Comment 32

[Markus Stange [:mstange]]

Yeah, I think bailing out with STOPPED should be fine. As you say, the results are pretty much garbage anyway.

Comment 33

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

I updated the patch, pushed again to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e637e4f7254

It's green except for OS X gtests, which are still failing, and I have no idea why. Running the gtests locally on OS X passes fine, so it must be compiler version differences? Which means painfully debugging this on try :(

Comment 34

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

Attachment: Patch v3

Updated per our IRC discussion. I added a new Union function that detects overflow, and modified TiledRegion so that it never enters any sort of overflow state. This time I think the try is all green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ac8d5d890f&group_state=expanded) although you have to look at the self-serve build api page to see the green OS X gtest run. I'll do another try push shortly, because the infra issues yesterday and today might mean that the results are bogus.

Comment 35

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

(In reply to Al Billings [:abillings] from comment #19)
> sec-approval+ for trunk.

I'm assuming this is still in effect, will land this shortly.

Comment 36

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7985dab6e65e

Comment 37

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

Comment on attachment 8771465 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: bug 1236043
[User impact if declined]: Potential heap overflow when TiledIntRegion is handed screwy rects (which apparently can happen sometimes)
[Describe test coverage new/current, TreeHerder]: added some gtests to cover this and related problems
[Risks and why]: The updated patch now tries to catch any possible overflows and deal with them in a "sane" way. The risk here is that the sane way might cause visual glitches on some pages. Note that without this patch, those pages effectively have undefined behaviour, so I would argue that even though the users might now see glitches where there were previously none, the new behaviour is more consistent and correct, and will be easier to fix.
[String/UUID change made/needed]: none

Comment 38

[Sylvestre Ledru [:Sylvestre]]

I am uncomfortable taking such changes that late in the 48 cycle.
Can it ride the train from 49?

Comment 39

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

In the end it's your call as the release driver. I don't like shipping known security issues in release, so I lean towards uplifting because of that, even if it causes visual regressions.

We should definitely at least uplift to 49, and let it bake on Nightly and Aurora for a couple of days to see if there's any fallout. If there is, then that will probably force the decision for us.

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have caused a few crashtests to fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=32003985&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=32003869&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4a6326df6e

Comment 41

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

Comment on attachment 8771465 [details] [diff] [review]
Patch v3

Well, I guess that settles the uplift question. Back to the drawing board.

Comment 42

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d810bca6c4e

Comment 43

[Daniel Veditz [:dveditz]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #38)
> I am uncomfortable taking such changes that late in the 48 cycle.
> Can it ride the train from 49?

If we fix this now then Release users never had this vulnerability.
If we wait until Fx49 then Release users will be vulnerable for 6 weeks, and:
 * this patch is already in the tree
 * comments (and type of change) make clear it's fixing an integer overflow
 * patch includes tests that demonstrate int overflows

Comment 44

[Daniel Veditz [:dveditz]]

Is there any easy way to redefine Union and UnionEdges to _be_ the Safe... forms for integer rect types? having to go change specific spots to call them explicitly is likely to lead to an incomplete fix and new problems in the future.

Comment 45

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #42)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d810bca6c4e

This try push has a bunch of failures but as far as I can tell they're all intermittents or otherwise known issues. There's a bunch of M-gl jobs which are failing with "UNEXPECTED-TEST-PASS" which according to Mason is a thing happening on other pushes as well. So I *think* this push is clean but I really can't say for sure.

Wes, before I try landing this again, would you mind taking a look at the push above and let me know if you think any of those failures are my fault?

Comment 46

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

(In reply to Daniel Veditz [:dveditz] from comment #44)
> Is there any easy way to redefine Union and UnionEdges to _be_ the Safe...
> forms for integer rect types? having to go change specific spots to call
> them explicitly is likely to lead to an incomplete fix and new problems in
> the future.

Not really, the return type of the new Safe functions is a Maybe<...> and so each call site will have to be updated to handle the Maybe in a site-specific way.

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks good to me. The gl bustage is actual bustage from your base revision, which has a backout sitting on inbound at the moment. No guarantees you haven't broken something hiding behind that bustage, though.

Comment 48

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

Thanks. I did another push earlier today, it's looking better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c8ccba187c5&group_state=expanded

The only consistent failure there is the Android debug R17, and it looks like that was happening on m-c as well, so I'm going to say "not mine" and land this again.

Comment 49

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

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

(The difference between this version and the one in the last attempt is that I added another CheckedInt to catch overflows on the rects.Length() + emptyTiles.Length() >= kMaxTiles check).

Comment 50

[Carsten Book [:Tomcat]]

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

Comment 51

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

Comment on attachment 8771465 [details] [diff] [review]
Patch v3

Approval Request Comment - see comment 37

I'm still in favor of uplifting this to beta, for the reasons that Dan said in comment 43.

Comment 52

[Sylvestre Ledru [:Sylvestre]]

I understand but this is extremely late in the cycle with some risk. Are you ready to work during this weekend and next week to fix potential regressions? If you are not available, who else could help with this?

Comment 53

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #52)
> I understand but this is extremely late in the cycle with some risk. Are you
> ready to work during this weekend and next week to fix potential
> regressions? If you are not available, who else could help with this?

I'm available this weekend and for bits of next week, but I'll be in Seattle at a F2F and so won't be available on Tuesday/Wednesday. Markus, would you also be available to help fix potential regressions?

Comment 54

[Markus Stange [:mstange]]

Sure.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8771465 [details] [diff] [review]
Patch v3

OK, thanks guys.

If we are causing too many regressions, we can always backout the change.
Kats, Markus, could you explain what kind of regressions QE should look for?

Andrei, could you help here?

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/419d32e09203


This doesn't apply cleanly to beta, though:
$ hg graft -r 419d32e09203
grafting 343175:419d32e09203 "Bug 1283826. r=mstange,Bas a=sylvestre"
merging gfx/2d/BaseRect.h
merging gfx/tests/gtest/TestRegion.cpp
warning: conflicts while merging gfx/2d/BaseRect.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Could you rebase it asap for 48 ?

Comment 58

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

Rebased and landed:
https://hg.mozilla.org/releases/mozilla-beta/rev/ccf0f9620bdc

Comment 59

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #55)
> Kats, Markus, could you explain what kind of regressions QE should look for?

I would mostly just watch out for areas of the screen not getting updated during animations and scrolling. If we hit the overflow checking and fail to add areas to the TiledRegion the most likely result is screen contents won't get invalidated and repainted.

Comment 60

[Andrei Vaida [:avaida]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #55)
> Andrei, could you help here?

I'll make sure this gets looked at.

Comment 61

[Mihai Boldan, Desktop QA [:mboldan]]

I've tested around this bug on Firefox 48.0b10, Firefox 49.0a2 (2016-07-24) and Firefox 50.0a1 (2016-07-24), without encountering any relevant issues.

Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64 were used to target various animated websites. 

If there are any other specific tests that need to be performed, please let me know.

Comment 62

[Sylvestre Ledru [:Sylvestre]]

Thanks Mihai!