Skip to content

Commit

Permalink
Bug 1658986 Part 3: Remove NativelayerCA checks of invalid regions. r…
Browse files Browse the repository at this point in the history
…=mstange

These checks were designed to identify if we have a logic error in the
display rects or update rects we get from our callers to
HandlePartialUpdate. We've gathered enough evidence that there is such an
error and we've opened Bug 1818540 to address it. There's no longer a good
reason to keep these checks around, as they just crash the browser for
users rather than risk showing them invalid pixels (which we're fairly
sure are valid pixels).

Differential Revision: https://phabricator.services.mozilla.com/D172287
  • Loading branch information
bradwerth committed Mar 10, 2023
1 parent b80d0fa commit c558918
Showing 1 changed file with 0 additions and 73 deletions.
73 changes: 0 additions & 73 deletions gfx/layers/NativeLayerCA.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1217,49 +1217,6 @@ CGColorRef CGColorCreateForDeviceColor(gfx::DeviceColor aColor) {
MOZ_RELEASE_ASSERT(!mInProgressUpdateRegion);
MOZ_RELEASE_ASSERT(!mInProgressDisplayRect);

#ifdef NIGHTLY_BUILD
// Check if this update will leave the in progress surface with invalid pixels. This is
// only possible if the display rect is changing. That is true because definitionally the
// front surface has valid pixels for the entire current display rect. Given that, the in
// progress surface will copy the valid pixels from the front surface to cover any
// invalid regions in the in progress surface. It is only when the display rect changes
// that the in progress surface is relying on aUpdateRegion to cover the revealed area,
// which might also be partly or completely covered by valid pixels in the front surface.
//
// To check this, we check that the area exposed by the new display rect is covered by
// some combination of the update region and the front surface's valid region. Because
// this check is complex, we only do it in Nightly.
//
// As a special case, we'll tolerate a remaining area that is only one pixel tall or wide,
// which is also tolerated in NotifySurfaceReady (until Bug 1818540 is fixed).
//
// Also, since this condition will hit a later release assert in NotifySurfaceReady, we
// choose to crash now.

if (!mDisplayRect.IsEqualInterior(aDisplayRect)) {
gfx::IntRegion exposedRegion(aDisplayRect);

if (mFrontSurface) {
// If there's a front surface, we'll fill in any valid pixels in the exposed region.
// We express this by intersecting the exposed region with the invalid region of the
// front surface.
exposedRegion.AndWith(mFrontSurface->mInvalidRegion);
}

gfx::IntRegion invalidRegion(exposedRegion);
invalidRegion.SubOut(aUpdateRegion);
IntRect invalidBounds = invalidRegion.GetBounds();
if (invalidBounds.width > 1 && invalidBounds.height > 1) {
// Let's crash now instead of later.
std::ostringstream reason;
reason << "The update region " << aUpdateRegion << " must cover the invalid region "
<< exposedRegion << ".";
gfxCriticalError() << reason.str();
MOZ_CRASH();
}
}
#endif

mInProgressUpdateRegion = Some(aUpdateRegion);
mInProgressDisplayRect = Some(aDisplayRect);

Expand Down Expand Up @@ -1369,21 +1326,6 @@ CGColorRef CGColorCreateForDeviceColor(gfx::DeviceColor aColor) {
mFrontSurface = std::move(mInProgressSurface);
mFrontSurface->mInvalidRegion.SubOut(mInProgressUpdateRegion.extract());

// Bug 1818540: We have a rounding error in our invalid region calculation
// somewhere. It's either in the update region that we receive from WebRender,
// or somewhere in this class's invalid region computation. Until we solve
// that, we might be left with a one-pixel tall or wide invalid region on the
// front surface, which will hit either the assert at the end of this function,
// or the assert in HandlePartialUpdate. There is no evidence that we are
// actually missing one pixel tall or wide strips from our updates and filling
// them with bad pixels. So to solve this problem for now, we check for a
// degenerate invalid region and clear it. Fixing Bug 1818540 will remove this
// cleanup and the asserts in this function and in HandlePartialUpdate.
IntRect frontSurfaceInvalidBounds = mFrontSurface->mInvalidRegion.GetBounds();
if (frontSurfaceInvalidBounds.width <= 1 || frontSurfaceInvalidBounds.height <= 1) {
mFrontSurface->mInvalidRegion.SetEmpty();
}

ForAllRepresentations([&](Representation& r) { r.mMutatedFrontSurface = true; });

MOZ_RELEASE_ASSERT(mInProgressDisplayRect);
Expand All @@ -1392,21 +1334,6 @@ CGColorRef CGColorCreateForDeviceColor(gfx::DeviceColor aColor) {
ForAllRepresentations([&](Representation& r) { r.mMutatedDisplayRect = true; });
}
mInProgressDisplayRect = Nothing();

if (!mFrontSurface->mInvalidRegion.Intersect(mDisplayRect).IsEmpty()) {
// We have invalid pixels within the display rect. Let's report the regions as
// a gfxCriticalError so we have something to review in the crash we're about
// to generate from the release assert below.
std::ostringstream reason;
reason << "The front surface invalid region " << mFrontSurface->mInvalidRegion
<< " must not intersect the display rect " << mDisplayRect << ".";
if (mClipRect) {
reason << " And clip rect is " << *mClipRect << ".";
}
gfxCriticalError() << reason.str();
}
MOZ_RELEASE_ASSERT(mFrontSurface->mInvalidRegion.Intersect(mDisplayRect).IsEmpty(),
"Parts of the display rect are invalid! This shouldn't happen.");
}

void NativeLayerCA::DiscardBackbuffers() {
Expand Down

0 comments on commit c558918

Please sign in to comment.