1022612 - Remove ComputeVisibility pass when painting

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have a large set of patches that do this. They basically work. The goal is to eliminate the cost of the ComputeVisibility pass we do now, since most of the work is currently done again via RecomputeVisibilty after we've determined the visible regions for ThebesLayers. We can also simplify some code.

The basic idea is that we compute an mVisibilityRect for display items at construction, based on the current dirty rect. FrameLayerBuilder then constructs layers, and ProcessDisplayItems takes over responsibility for merging and flattening display lists. Retained layer visible regions are computed without any occlusion culling. For each retained ThebesLayer, we keep doing our current RecomputeVisiblity occlusion culling for the items within that layer.

But we do need some cross-retained-layer occlusion culling. Bug 988511 at least requires it. I'm not sure how much to do. It has to be done carefully because any async transformations need to block it.

I'm thinking of doing something simple where if a ContainerLayer C has a cliprect and has a layer child L which is opaque, fills C's cliprect, and has the same animated geometry root, we set the visible region of all child layers below L to empty. I think this would suffice for bug 988511.

Matt, Markus, can you guys think of any other important cases where cross-retained-layer occlusion culling is needed?

Comment 1

[Markus Stange [:mstange]]

In Desktop Firefox we'll want to make sure that the ThebesLayer that contains the window chrome doesn't contain a background for the viewport because that would waste memory.

In general, it looks like there are two different reasons why we would want cross-retained-layer occlusion culling:
 (1) We don't want ThebesLayers to paint more than needed (memory + DrawThebesLayer perf concern) and
 (2) we don't want to composite Layers that are occluded (compositor perf concern).

I think (2) should be handled entirely on the Compositor side by doing an occlusion pass prior to every composition. In this bug we should only need to worry about (1).

Comment 2

[Markus Stange [:mstange]]

Compositor-side occlusion culling would also be useful for bug 1021845.

Comment 3

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)

> 
> I'm thinking of doing something simple where if a ContainerLayer C has a
> cliprect and has a layer child L which is opaque, fills C's cliprect, and
> has the same animated geometry root, we set the visible region of all child
> layers below L to empty. I think this would suffice for bug 988511.
> 
> Matt, Markus, can you guys think of any other important cases where
> cross-retained-layer occlusion culling is needed?

Can we also do a version that doesn't involve the ContainerLayer? Sibling layers that belong to the same active geometry root should be able to occlude each other, even if they don't cover the container.

I think we'll want that to avoid painting into scrollable ThebesLayers that are partially occluded by active layer content.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=24f23982cb54

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This may fix the reftest failures: https://tbpl.mozilla.org/?tree=Try&rev=d790b0a86d88

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=31ad270c5dab

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=dd42b3b8a348

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=c89be62a010f

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=4d54e349d6a1

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're about green modulo some test fuzz. https://tbpl.mozilla.org/?tree=Try&rev=a7a153a02827

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> We're about green modulo some test fuzz.
> https://tbpl.mozilla.org/?tree=Try&rev=a7a153a02827

That was a bit over-optimistic --- there was a serious bug that manifested on B2G that took me way too long to figure out :-(. But here we go:
https://tbpl.mozilla.org/?tree=Try&rev=0c2f1e87fd11

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Always pass a frame to AutoBuildingDisplayList

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove unused nsDisplayWrapList constructor

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Rename "cached frame" to "current frame" in nsDisplayListBuilder and take advantage of the fact it's always set

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Track current dirty rect in nsDisplayListBuilder

We need this to set the initial visible rect during display list construction.
Eventually we'll also be able to get rid of the dirty rect parameter to
nsIFrame::BuildDisplayList.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: BuildDisplayListForExtraPage needs to pass the correct dirty rect in

When printing, every page has the same origin. So doing this change naively
would result in the first page having all the display items for every page
added to it, and all but the first page's display items being pruned
away by PruneDisplayListForExtraPage. This would making printing long documents
very slow. We avoid that problem with the new check for
NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO, so the only pages other than the
current page we descend into are the ones with placeholders for abs-pos content
on the current page.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Set the initial mVisibleRect for each display item to the dirty rect when we create the item

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 7: Enable APZC for IPC reftests

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 8: nsDisplayWrapList (but not subclasses) should return true for ShouldFlattenAway

Also the assertion in TryMerge is going away because we're going to do TryMerge
first in FrameLayerBuilder.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 9: nsDisplayScrollInfoLayer destructor does not need to destroy ScrollLayerCount

Getting the timing of this right without processing display items in reverse
order is hard. But it doesn't matter if this property sticks around anyway.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 10: Implement merging and flattening in ProcessDisplayItems

BuildContainerLayerFor now has to be able to mutate the passed-in display item
list.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 11: Set opaque flag on nsDisplayList if we find an opaque item that covers the whole list

This is less general than what nsDisplayItem::ComputeVisibility does. This means
if multiple opaque items together cover the list bounds, but not individually,
we won't mark the list as opaque. I think that should be OK.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 12: Propagate NeedsTransparentSurface in ProcessDisplayItems

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 13: Set mDidComputeVisibility in ProcessDisplayItems

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 14: Don't compute a final transparent region anymore

Computing this via FrameLayerBuilder is some work and we don't really have to.

SuppressComponentAlpha will be false in more cases. This will be OK as long as
text in the chrome window is over opaque content in the same ThebesLayer. We
will miss some edge cases such as text in 'opacity' with no opaque background.
This should be OK.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 16: No need to exclude final transparent region from window opaque region

The removed code should be a no-op as long as the window opaque region is
accurate enough.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 17: RecordFrameMetrics should not set layer visible regions

This is unnecessary. FrameLayerBuilder sets the correct region.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 18: When ComputeVisibility returns false, RecomputeVisibility should avoid painting the item

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 19: Test that merged display item lists merge their contents in the correct z-order

Prior to this patch, the only tests that caught this were a couple of obscure
cases on B2G. This test tests it on all platforms.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 20: Do the business

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 21: Remove DidComputeVisibility checking

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 22: Add MOZ_COUNT_CTOR to nsDisplayWrapList

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 23: Remove nsDisplayItem::IsVaryingRelativeToMovingFrame

It's obsolete and no-one calls it.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 24: Simplify nsDisplayList::ComputeVisibilityForSublist now that FrameLayerBuilder does most of the work

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 25: We don't need to explicitly worry about displayports anymore when computing occlusion

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 26: nsDisplayList::mVisibleRect is no longer used

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 27: Make FrameLayerBuilder responsible for setting all layer visible regions

Calling Layer::SetVisibleRegion multiple times in a transaction can result in
unnecessary IPC traffic.

XXX This patch removes Intersect(childGfxBounds). Is this OK? It seems to me
it should be, with the changes to ProjectBounds to exclude zero/negative W
coordinates.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 28: Make nsLayoutUtils::GetScrollableFrameFor return null for non-scrolled-frames

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 29: Expose GetAnimatedGeometryRootForFrame

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 30: Expose IsConstructingScrollLayerForScrolledFrame and IsDisplayPortOpaque on nsDisplayScrollLayer

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 31: Perform layer-level occlusion culling in FrameLayerBuilder

We need this to avoid constructing and painting unncecessarily large
ThebesLayers.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 32: Remove nsDisplayItem::SetVisibleRegionOnLayer

It is no longer called because FrameLayerBuilder always sets the visible
regions on layers now.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 33: Remove InFixedPos code

This hasn't been used for a while I guess.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 34: Skip RecomputeVisibilityForItems in inactive layers

RecomputeVisibilityForItems for the retained ThebesLayer already recomputes
visibility for all items in that layer, including items nested in other items.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 35: nsDisplayPluginReadback doesn't need to hack visible regions anymore

One nice bit of fallout from this bug is that handling plugin background
readback is simplified. We no longer have to fiddle with display item
visibility calculations; only layer occlusion culling has to know about
readback.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 36: Avoid redundant calls to ShouldPrerenderTransformedContent

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 37: Remove aAllowVisibleRegionExpansion

This is no longer needed thanks to the readback simplification.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 38: Avoid test failure due to antialiased pixel leakage

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 39: Add a little bit of reftest fuzzing

Comment 50

[Matt Woodrow (:mattwoodrow)]

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

Comment 51

[Matt Woodrow (:mattwoodrow)]

Wrong bug, ignore the previous comment.

Comment 52

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452712 [details] [diff] [review]
Part 3: Rename "cached frame" to "current frame" in nsDisplayListBuilder and take advantage of the fact it's always set

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

::: layout/base/nsDisplayList.h
@@ +238,5 @@
>     * @return a point pt such that adding pt to a coordinate relative to aFrame
>     * makes it relative to ReferenceFrame(), i.e., returns 
>     * aFrame->GetOffsetToCrossDoc(ReferenceFrame()). The returned point is in
>     * the appunits of aFrame. It may be optimized to be faster than
>     * aFrame->GetOffsetToCrossDoc(ReferenceFrame()) (but currently isn't).

This comment is a bit out of date

@@ +756,5 @@
>    nsAutoTArray<ThemeGeometry,2>  mThemeGeometries;
>    nsDisplayTableItem*            mCurrentTableItem;
>    DisplayListClipState           mClipState;
>    const nsRegion*                mFinalTransparentRegion;
> +  // When mCurrentFrame is non-null, mCachedOffset is the offset from

Isn't it always non-null now?

@@ +757,5 @@
>    nsDisplayTableItem*            mCurrentTableItem;
>    DisplayListClipState           mClipState;
>    const nsRegion*                mFinalTransparentRegion;
> +  // When mCurrentFrame is non-null, mCachedOffset is the offset from
> +  // mCurrentFrame to mReferenceFrame.

mCurrentReferenceFrame

Comment 53

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452715 [details] [diff] [review]
Part 6: Set the initial mVisibleRect for each display item to the dirty rect when we create the item

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

::: layout/base/nsDisplayList.h
@@ -2680,5 @@
>  
>  protected:
>    nsDisplayWrapList() {}
>  
> -  void MergeFrom(nsDisplayWrapList* aOther)

nsDisplayTransform::TryMerge has a caller of this function which isn't removed here. I assume it is later on somewhere.

Comment 54

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452721 [details] [diff] [review]
Part 11: Set opaque flag on nsDisplayList if we find an opaque item that covers the whole list

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2732,5 @@
> +      for (const nsRect* r = iter.Next(); r; r = iter.Next()) {
> +        opaqueClipped.Or(opaqueClipped, itemClip.ApproximateIntersectInward(*r));
> +      }
> +      if (opaqueClipped.Contains(mContainerBounds)) {
> +        aList->SetIsOpaque();

Couldn't we accumulate the opaque clipped regions for all the items and get the same behaviour as before? It probably doesn't matter much though.

Comment 55

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452724 [details] [diff] [review]
Part 14: Don't compute a final transparent region anymore

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

SuppressComponentAlpha will be true more often, not false, right?

::: layout/base/FrameLayerBuilder.cpp
@@ +2190,5 @@
>        tmp.Or(mOpaqueRegion, *r);
>         // Opaque display items in chrome documents whose window is partially
>         // transparent are always added to the opaque region. This helps ensure
>         // that we get as much subpixel-AA as possible in the chrome.
> +       if (tmp.GetNumRects() <= 4 || aItem->Frame()->PresContext()->IsChrome()) {

Do we even need to care about the number of rects any more?

@@ +2202,5 @@
>      if (!componentAlpha.IsEmpty()) {
>        nsIntRect componentAlphaRect =
>          aState->ScaleToOutsidePixels(componentAlpha, false).Intersect(aVisibleRect);
>        if (!mOpaqueRegion.Contains(componentAlphaRect)) {
> +        if (SuppressComponentAlpha(aState->mBuilder, aItem)) {

Can we skip this if this is an inactive layer tree?

Comment 56

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452738 [details] [diff] [review]
Part 27: Make FrameLayerBuilder responsible for setting all layer visible regions

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

::: layout/base/FrameLayerBuilder.cpp
@@ -1627,5 @@
> -  nsIntRect childBounds = aLayerVisibleRegion.GetBounds();
> -  gfxRect childGfxBounds(childBounds.x, childBounds.y,
> -                         childBounds.width, childBounds.height);
> -  gfxRect layerVisible = transform.Inverse().ProjectRectBounds(itemVisible);
> -  layerVisible = layerVisible.Intersect(childGfxBounds);

ProjectRectBounds does exclude points with w <= 0, but it replaces them with interpolated points at w = 0.000001. That's pretty close to the asymptote, so we can get some really large values out of this function. Even with the input rect being the post-transform size, I expect we still might get big enough numbers to fail the checks in GfxRectToIntRect.

Can we get the 'inner' size of a display item (getting the bounds of the child list?) to use just to restrict the results to sane coordinates?

Comment 57

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452739 [details] [diff] [review]
Part 28: Make nsLayoutUtils::GetScrollableFrameFor return null for non-scrolled-frames

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

::: layout/base/nsLayoutUtils.cpp
@@ +1482,5 @@
>  nsLayoutUtils::GetScrollableFrameFor(const nsIFrame *aScrolledFrame)
>  {
>    nsIFrame *frame = aScrolledFrame->GetParent();
>    nsIScrollableFrame *sf = do_QueryFrame(frame);
> +  return sf && sf->GetScrolledFrame() == aScrolledFrame ? sf : nullptr;

Parens around the condition might make this clearer.

Comment 58

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8452742 [details] [diff] [review]
Part 31: Perform layer-level occlusion culling in FrameLayerBuilder

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3416,5 @@
>    NS_ASSERTION(mContainerBounds.IsEqualInterior(mAccumulatedChildBounds),
>                 "Bounds computation mismatch");
>    uint32_t textContentFlags = 0;
>  
> +  if (mLayerBuilder->IsBuildingRetainedLayers()) {

Why do we only do occlusion culling for retained layers? Inactive layer trees can have multiple layers and we want to avoid painting pixels multiple times.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #54)
> Couldn't we accumulate the opaque clipped regions for all the items and get
> the same behaviour as before? It probably doesn't matter much though.

We could. I just don't want to do that if we don't need to.

(In reply to Matt Woodrow (:mattwoodrow) from comment #55)
> Review of attachment 8452724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> SuppressComponentAlpha will be true more often, not false, right?

Yes. I'll fix that.

> ::: layout/base/FrameLayerBuilder.cpp
> @@ +2190,5 @@
> >        tmp.Or(mOpaqueRegion, *r);
> >         // Opaque display items in chrome documents whose window is partially
> >         // transparent are always added to the opaque region. This helps ensure
> >         // that we get as much subpixel-AA as possible in the chrome.
> > +       if (tmp.GetNumRects() <= 4 || aItem->Frame()->PresContext()->IsChrome()) {
> 
> Do we even need to care about the number of rects any more?

I think we probably do. If you mean because pixman regions handle complex regions better, that probably has improved the situation, but we still have to keep the complexity bounded.

> @@ +2202,5 @@
> >      if (!componentAlpha.IsEmpty()) {
> >        nsIntRect componentAlphaRect =
> >          aState->ScaleToOutsidePixels(componentAlpha, false).Intersect(aVisibleRect);
> >        if (!mOpaqueRegion.Contains(componentAlphaRect)) {
> > +        if (SuppressComponentAlpha(aState->mBuilder, aItem)) {
> 
> Can we skip this if this is an inactive layer tree?

I suppose so. I'm a bit worried about items in inactive layers in chrome windows getting component alpha disabled, though.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #56)
> ProjectRectBounds does exclude points with w <= 0, but it replaces them with
> interpolated points at w = 0.000001. That's pretty close to the asymptote,
> so we can get some really large values out of this function. Even with the
> input rect being the post-transform size, I expect we still might get big
> enough numbers to fail the checks in GfxRectToIntRect.
> 
> Can we get the 'inner' size of a display item (getting the bounds of the
> child list?) to use just to restrict the results to sane coordinates?

Good point. I'll have to look into this further.

(In reply to Matt Woodrow (:mattwoodrow) from comment #58)
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +3416,5 @@
> >    NS_ASSERTION(mContainerBounds.IsEqualInterior(mAccumulatedChildBounds),
> >                 "Bounds computation mismatch");
> >    uint32_t textContentFlags = 0;
> >  
> > +  if (mLayerBuilder->IsBuildingRetainedLayers()) {
> 
> Why do we only do occlusion culling for retained layers? Inactive layer
> trees can have multiple layers and we want to avoid painting pixels multiple
> times.

Because RecomputeVisibility in the retained ThebesLayer will perform occlusion culling for all the inactive content in that layer.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 27: Make FrameLayerBuilder responsible for setting all layer visible regions

Calling Layer::SetVisibleRegion multiple times in a transaction can result in
unnecessary IPC traffic.

This patch removes Intersect(childGfxBounds). This is only needed to
restrict the visible region to something sane for 3D transforms, and this will
be fixed up in a later patch.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 40: Restrict visible rect of 3D-transformed layers before converting to nsIntRect

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 41: Use itemType instead of calling GetType() again

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On Windows, the changes I made to glass region calculation mean that the height of the glass region at the top of the Firefox window has increased, because now we're only taking into account the "excluded glass" region of the window and ignoring the fact that the bottom half of the ThebesLayer at the top of the chrome window is opaque. This is probably worth fixing since on slow Windows 7 machines glass compositing is slow.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 42: Add opaque regions of ThebesLayer content to the "exclude glass" region

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 43: Fix up SuppressComponentAlpha to allow component alpha in inactive layers over opaque parts of the chrome window

Comment 67

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8455127 [details] [diff] [review]
Part 43: Fix up SuppressComponentAlpha to allow component alpha in inactive layers over opaque parts of the chrome window

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2220,5 @@
> +  if (aItem->ReferenceFrame() != aBuilder->RootReferenceFrame()) {
> +    // aItem is probably in some transformed subtree.
> +    // We're not going to bother figuring out where this landed, we're just
> +    // going to assume it might have landed over a transparent part of
> +    // the window.

Maybe worth adding an NS_WARNING here in case someone adds text within a transform in the chrome document and wonders why they're not getting subpixel-AA?

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #67)
> Maybe worth adding an NS_WARNING here in case someone adds text within a
> transform in the chrome document and wonders why they're not getting
> subpixel-AA?

Unfortunately I don't think anyone doing that is likely to notice an NS_WARNING.

Latest try attempt:
https://tbpl.mozilla.org/?tree=Try&rev=bd263fd33053
Hopefully this will be green :-)

Comment 69

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #67)
> > Maybe worth adding an NS_WARNING here in case someone adds text within a
> > transform in the chrome document and wonders why they're not getting
> > subpixel-AA?
> 
> Unfortunately I don't think anyone doing that is likely to notice an
> NS_WARNING.

No, but we might once we start debugging the gfx bug that gets filed :)

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We'd probably be better off annotating the display list dump for text items to indicate whether and how subpixel AA was disabled.

This try push is looking good.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5bacdd4594c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e502d50b8e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/18ffe0bb4d42
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdaa916f6851
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2fc524e961
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad096055f1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b81f9a774059
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ebd2f6b65a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4e2a1dcadc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf33a32bfe0
https://hg.mozilla.org/integration/mozilla-inbound/rev/75fffcce9c4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc140abf7b17
https://hg.mozilla.org/integration/mozilla-inbound/rev/121bf69509b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e95971991caa
https://hg.mozilla.org/integration/mozilla-inbound/rev/c59ee68b1917
https://hg.mozilla.org/integration/mozilla-inbound/rev/549e3cb9e111
https://hg.mozilla.org/integration/mozilla-inbound/rev/376c45f4d905
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b6049145dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6cb0f412f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/032ba47c6684
https://hg.mozilla.org/integration/mozilla-inbound/rev/c081917e5626
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67228df9ae0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ec6bcf251f
https://hg.mozilla.org/integration/mozilla-inbound/rev/79e0ce466219
https://hg.mozilla.org/integration/mozilla-inbound/rev/f89950384bf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9564f9f4648
https://hg.mozilla.org/integration/mozilla-inbound/rev/c413b946dbc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8625f91b68
https://hg.mozilla.org/integration/mozilla-inbound/rev/233345d8ffc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b488c389e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d141d01c97
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9751c1302a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e190d419511
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd246b1436c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e97b4516cbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de27f6dcd31
https://hg.mozilla.org/integration/mozilla-inbound/rev/3773718e4870
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f857407b64e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6be65e455a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1f3340c45f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f12d7d68b685
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1776b2606d

Comment 72

[Carsten Book [:Tomcat]]

sorry had to backout all the csets above for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=43805026&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=43804253&tree=Mozilla-Inbound

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This got backed out.

Bug 1034247 landed just before this. It makes ChooseScaleAndSetTransform dependent on the visible rect of the aChildren display list, which isn't computed anymore with these patches.

I think I can fix it though.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15: Add nsDisplayItem::GetVisibleRectForChildren()

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 15.5: Use GetVisibleRectForChildren instead of GetVisibleRect

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=9b9847cff2cb

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 44: Make nsSimplePageSequence only paint the pages that are actually in the dirty rect

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=20f6ff561a49

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 45: Increase region complexity limit for mDrawAboveRegion/mVisibleAboveRegion to fix scroll-rounding.html test failure

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc8d30ff7c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eadc5fee43d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd594236388f
https://hg.mozilla.org/integration/mozilla-inbound/rev/42fa2c97e989
https://hg.mozilla.org/integration/mozilla-inbound/rev/055dd1921e8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b18dedd1505
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc4a4a7aa73
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3e99a92ff0
https://hg.mozilla.org/integration/mozilla-inbound/rev/797058a09ad2
https://hg.mozilla.org/integration/mozilla-inbound/rev/58abf5b0e148
https://hg.mozilla.org/integration/mozilla-inbound/rev/da9152b6ea29
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ab1bcd4c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa04a071e60
https://hg.mozilla.org/integration/mozilla-inbound/rev/75f7dbb5a2a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8974b74b0eb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d1a3e8b39d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2c5397ca8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7134400f08
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c48c6ee5dc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ae9cb22edb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0758d2a06002
https://hg.mozilla.org/integration/mozilla-inbound/rev/8369d9ad7ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/489f6a7c0c03
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f4e9da0e4b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b17cf0f806
https://hg.mozilla.org/integration/mozilla-inbound/rev/3388856a80ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/0268a46f4880
https://hg.mozilla.org/integration/mozilla-inbound/rev/959917c9027d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e2c6a72043
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23f1081afb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b72413ecc385
https://hg.mozilla.org/integration/mozilla-inbound/rev/2763c4878de5
https://hg.mozilla.org/integration/mozilla-inbound/rev/18eecc5b1ef7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff86ced4d46
https://hg.mozilla.org/integration/mozilla-inbound/rev/66de53183a22
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b837686bdf
https://hg.mozilla.org/integration/mozilla-inbound/rev/f14ada64fe1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/667793b21194
https://hg.mozilla.org/integration/mozilla-inbound/rev/b42054800020
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0df1c9d68b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac64141a00a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c57c620c898
https://hg.mozilla.org/integration/mozilla-inbound/rev/caab10bf6ca3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f3749c95eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae9316fd909
https://hg.mozilla.org/integration/mozilla-inbound/rev/34b3014a3112

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Latest try push is here: https://tbpl.mozilla.org/?tree=Try&rev=102b4c36535c

Comment 83

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest perma-fail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bcded4e3b4a

Opt mochitest-5: https://tbpl.mozilla.org/php/getParsedLog.php?id=44030428&tree=Mozilla-Inbound
Debug mochitest-9: https://tbpl.mozilla.org/php/getParsedLog.php?id=44029565&tree=Mozilla-Inbound

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sigh.

I can reproduce the debug crash in pickle.cc in the emulator and get a good stack trace:

#0  0x4270f366 in mozalloc_abort (msg=<value optimized out>)
    at /home/roc/mozilla-inbound/memory/mozalloc/mozalloc_abort.cpp:30
#1  0x40d036ca in Abort (aSeverity=<value optimized out>, aStr=0x0, 
    aExpr=<value optimized out>, aFile=<value optimized out>, aLine=173)
    at /home/roc/mozilla-inbound/xpcom/base/nsDebugImpl.cpp:472
#2  NS_DebugBreak (aSeverity=<value optimized out>, aStr=0x0, 
    aExpr=<value optimized out>, aFile=<value optimized out>, aLine=173)
    at /home/roc/mozilla-inbound/xpcom/base/nsDebugImpl.cpp:429
#3  0x40f138c4 in ~Logger (this=0x45f7f584, __in_chrg=<value optimized out>)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/logging.cc:47
#4  0x40f18d10 in ~LogWrapper (this=0x45f7fc34, iter=0x45f7fa84, 
    result=0x46bcf9ec)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/logging.h:60
#5  Pickle::ReadBool (this=0x45f7fc34, iter=0x45f7fa84, result=0x46bcf9ec)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/pickle.cc:173
#6  0x40ff179e in IPC::ParamTraitsFundamental<bool>::Read (this=0x46b944c0, 
    __v=0x46bcf960, __msg=0x45f7fc30, __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/ipc/chromium/src/chrome/common/ipc_message_utils.h:141
#7  ReadParam<bool> (this=0x46b944c0, __v=0x46bcf960, __msg=0x45f7fc30, 
    __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/ipc/chromium/src/chrome/common/ipc_message_utils.h:121
#8  Read<bool> (this=0x46b944c0, __v=0x46bcf960, __msg=0x45f7fc30, 
    __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/_ipdlheaders/mozilla/layers/PLayerTransactionParent.h:482
#9  mozilla::layers::PLayerTransactionParent::Read (this=0x46b944c0, 
    __v=0x46bcf960, __msg=0x45f7fc30, __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:3142
#10 0x40ff19b0 in mozilla::layers::PLayerTransactionParent::Read (this=0xad, 
    __v=0x4998e148, __msg=0x0, __iter=0x0)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:1335
#11 0x40ff1a26 in mozilla::layers::PLayerTransactionParent::Read (
    this=0x46b944c0, __v=0x46bcf958, __msg=0x45f7fc30, __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:2930
#12 0x40ff1bdc in mozilla::layers::PLayerTransactionParent::Read (
    this=0x46b944c0, __v=0x46bcf958, __msg=0x45f7fc30, __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:1603
#13 0x40ff1e2c in mozilla::layers::PLayerTransactionParent::Read (
    this=0x46b944c0, __v=0x45f7fa90, __msg=0x45f7fc30, __iter=0x45f7fa84)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:3730
#14 0x40ff3376 in mozilla::layers::PLayerTransactionParent::OnMessageReceived (
    this=0x46b944c0, __msg=...)
    at /home/roc/mozilla-inbound/obj-gonk-debug/ipc/ipdl/PLayerTransactionParent.cpp:440

We're getting an invalid value in useClipRect. Not sure what the value is yet.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The value, in one case at least, is 1065353216. 0x3f800000, which looks more like a pointer.

The IPDL message does look corrupt. I've decoded it manually here (each element is a 4-byte int):

{0x14, /*cset.length*/
 0x2, 0xffffffd7, /*TOpCreateContainerLayer*/
 0xc, 0xfffffffc, 0xfffffff9, /*TOpRemoveChild(container,child)*/
 0xc, 0xfffffffc, 0xffffffe4, /*TOpRemoveChild*/
 0xb, 0xffffffd7, 0xfffffff9, /*TOpPrependChild*/
 0xa, 0xffffffd7, 0xffffffe4, 0xfffffff9, /*TOpInsertAfter*/
 0xa, 0xfffffffc, 0xffffffd7, 0xfffffffb, /*TOpInsertAfter*/
 0xd, 0xfffffffc, 0xfffffff7, 0xffffffd7, /*TOpRepositionChild*/
 0xc, 0xfffffffc, 0xfffffffa, /*TOpRemoveChild*/
 0xc, 0xfffffffc, 0xfffffff6, /*TOpRemoveChild*/
 0xc, 0xfffffffc, 0xfffffff3, /*TOpRemoveChild*/
 0xc, 0xfffffffc, 0xfffffff0, /*TOpRemoveChild*/
 0xc, 0xfffffffc, 0xffffffee, /*TOpRemoveChild*/
 0xf, 0xfffffffb, 0xffffffd6, /*TOpAttachCompositable*/

 /*TOpSetLayerAttributes*/ 
 0x7, 0xffffffd7,
  /*CommonLayerAttributes*/
  0x26, 0x94, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
  0x3f800000, 0x0, 0x0, 0x0, 0x0, 0x3f800000, 0x0, 0x0, 0x0, 0x0, 0x3f800000, 0x0, 0x0, 0x0, 0x0, 0x3f800000, 0x3f800000}

The last line looks like the transform; an identity matrix of floats. The previous line should be
nsIntRegion visibleRegion and EventRegions eventRegions. An EventRegions is just two nsIntRegions. An nsIntRegion is serialized as a list of rectangles followed by an empty rect sentinel. This requires that nsRegionRectIterator never return an empty rect.

What appears to have happened is that visibleRegion was stored as a single rectangle with origin 0x26,0x94 and size 0x0. This should never happen. But if it were to happen (due to a pixman bug or whatever), we get this bug: the region gets serialized as the empty rect followed by another empty rect, and then when deserializing it, we read the first empty rect and then think we're done. This off-by-4-ints error will eventually trigger this exact crash.

Now I'm trying to pin down how the child process generates this bad region, but I'm having difficulty with the B2G debugger.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's the stack trace when we first get into this invalid state. Validate() is what I added to catch the bug. Unsurprisingly it's in SetOuterVisibleRegion which I added.

#2  0x40b007c2 in nsRegion::Validate (this=<value optimized out>)
    at /home/roc/mozilla-inbound/gfx/src/nsRegion.cpp:20
#3  0x40b00acc in nsRegion::And (this=0xbee8d3c4, aRegion=..., 
    aRect=<value optimized out>)
    at /home/roc/mozilla-inbound/gfx/src/nsRegion.h:94
#4  nsIntRegion::And (this=0xbee8d3c4, aRegion=..., 
    aRect=<value optimized out>)
    at /home/roc/mozilla-inbound/gfx/src/nsRegion.h:410
#5  0x4163a806 in SetOuterVisibleRegion (aLayer=0x44b5b800, 
    aOuterVisibleRegion=0xbee8d3c4, aLayerContentsVisibleRect=0x44b06ecc)
    at /home/roc/mozilla-inbound/layout/base/FrameLayerBuilder.cpp:1720

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're intersecting the region given by extents = {x1 = 23, y1 = 133, x2 = 123, y2 = 233} with the rectangle {x = 38, y = 148, width = 0, height = 0} using pixman_region32_intersect_rect and it's giving us extents = {x1 = 38, y1 = 148, x2 = 38, y2 = 148}.

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, obvious pixman issue. The relevant code:

    else if (!reg1->data && !reg2->data)
    {
        /* Covers about 80% of cases that aren't trivially rejected */
        new_reg->extents.x1 = MAX (reg1->extents.x1, reg2->extents.x1);
        new_reg->extents.y1 = MAX (reg1->extents.y1, reg2->extents.y1);
        new_reg->extents.x2 = MIN (reg1->extents.x2, reg2->extents.x2);
        new_reg->extents.y2 = MIN (reg1->extents.y2, reg2->extents.y2);

        FREE_DATA (new_reg);

	new_reg->data = (region_data_type_t *)NULL;

There's nothing here to switch to pixman_region_empty_data.

So the question is whether to patch pixman or work around it in nsRegion. Since I'm not sure the pixman people would consider it a pixman bug, I incline to the latter, but I'm happy to make a pixman patch if someone tells me how to upstream it.

Comment 89

[Benoit Girard (:BenWa)]

Jeff what do you think about Comment 88?

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 46: Work around pixman bug to make sure nsRegionRectIterator never returns an empty rect

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=bfb74ddaeb64

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a870613a0e5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/378f3027448d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3ec7478483
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6ea9910204
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb48f5e961b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/117b5a04dbad
https://hg.mozilla.org/integration/mozilla-inbound/rev/da750c203884
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c529430c64
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d4a91fd70d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf91c8ca6fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e6a1d488a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb02e31b35a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d674b220a180
https://hg.mozilla.org/integration/mozilla-inbound/rev/054fe93eeb87
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c03d02fcf41
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5927e8e86b
https://hg.mozilla.org/integration/mozilla-inbound/rev/462bb749420e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15aca574567
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc64afa1c1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/58d7c85b017e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0d786121d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/058404840786
https://hg.mozilla.org/integration/mozilla-inbound/rev/14af206b64c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac44a3e02f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/61079074f838
https://hg.mozilla.org/integration/mozilla-inbound/rev/df68a4897fc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b03d30bb74
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1df8efc0422
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ced9a3b4e49
https://hg.mozilla.org/integration/mozilla-inbound/rev/308c07ab8697
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87f29dd4cfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0968c1d061
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fe1de532bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2ce158ea05
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4280b1c2eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9997475ee4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e17b8fe2cc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff491e2cf96e
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefee240e917
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bb154599e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7751fa7b29
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad42fadb4829
https://hg.mozilla.org/integration/mozilla-inbound/rev/10a604b0c00d
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a06c38dd43
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fdf302757f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4b268ef593
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a69de91baa

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll probably back this out on Aurora after the uplift. No point in carrying the risk there.

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a870613a0e5a
https://hg.mozilla.org/mozilla-central/rev/378f3027448d
https://hg.mozilla.org/mozilla-central/rev/3d3ec7478483
https://hg.mozilla.org/mozilla-central/rev/0e6ea9910204
https://hg.mozilla.org/mozilla-central/rev/cb48f5e961b8
https://hg.mozilla.org/mozilla-central/rev/117b5a04dbad
https://hg.mozilla.org/mozilla-central/rev/da750c203884
https://hg.mozilla.org/mozilla-central/rev/e8c529430c64
https://hg.mozilla.org/mozilla-central/rev/b0d4a91fd70d
https://hg.mozilla.org/mozilla-central/rev/9bf91c8ca6fc
https://hg.mozilla.org/mozilla-central/rev/a0e6a1d488a8
https://hg.mozilla.org/mozilla-central/rev/bb02e31b35a9
https://hg.mozilla.org/mozilla-central/rev/d674b220a180
https://hg.mozilla.org/mozilla-central/rev/054fe93eeb87
https://hg.mozilla.org/mozilla-central/rev/9c03d02fcf41
https://hg.mozilla.org/mozilla-central/rev/7e5927e8e86b
https://hg.mozilla.org/mozilla-central/rev/462bb749420e
https://hg.mozilla.org/mozilla-central/rev/c15aca574567
https://hg.mozilla.org/mozilla-central/rev/bfc64afa1c1a
https://hg.mozilla.org/mozilla-central/rev/58d7c85b017e
https://hg.mozilla.org/mozilla-central/rev/2b0d786121d4
https://hg.mozilla.org/mozilla-central/rev/058404840786
https://hg.mozilla.org/mozilla-central/rev/14af206b64c1
https://hg.mozilla.org/mozilla-central/rev/3ac44a3e02f4
https://hg.mozilla.org/mozilla-central/rev/61079074f838
https://hg.mozilla.org/mozilla-central/rev/df68a4897fc7
https://hg.mozilla.org/mozilla-central/rev/03b03d30bb74
https://hg.mozilla.org/mozilla-central/rev/d1df8efc0422
https://hg.mozilla.org/mozilla-central/rev/3ced9a3b4e49
https://hg.mozilla.org/mozilla-central/rev/308c07ab8697
https://hg.mozilla.org/mozilla-central/rev/b87f29dd4cfb
https://hg.mozilla.org/mozilla-central/rev/bc0968c1d061
https://hg.mozilla.org/mozilla-central/rev/05fe1de532bc
https://hg.mozilla.org/mozilla-central/rev/fa2ce158ea05
https://hg.mozilla.org/mozilla-central/rev/9c4280b1c2eb
https://hg.mozilla.org/mozilla-central/rev/3d9997475ee4
https://hg.mozilla.org/mozilla-central/rev/8e17b8fe2cc6
https://hg.mozilla.org/mozilla-central/rev/ff491e2cf96e
https://hg.mozilla.org/mozilla-central/rev/eefee240e917
https://hg.mozilla.org/mozilla-central/rev/c1bb154599e2
https://hg.mozilla.org/mozilla-central/rev/cb7751fa7b29
https://hg.mozilla.org/mozilla-central/rev/ad42fadb4829
https://hg.mozilla.org/mozilla-central/rev/10a604b0c00d
https://hg.mozilla.org/mozilla-central/rev/85a06c38dd43
https://hg.mozilla.org/mozilla-central/rev/9fdf302757f5
https://hg.mozilla.org/mozilla-central/rev/cb4b268ef593
https://hg.mozilla.org/mozilla-central/rev/24a69de91baa

Comment 95

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

Part 20 ("Do the business") seems to have caused bug 1041608.

Comment 96

[Jeff Muizelaar [:jrmuizel]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #88)
> Yeah, obvious pixman issue. The relevant code:
> 
>     else if (!reg1->data && !reg2->data)
>     {
>         /* Covers about 80% of cases that aren't trivially rejected */
>         new_reg->extents.x1 = MAX (reg1->extents.x1, reg2->extents.x1);
>         new_reg->extents.y1 = MAX (reg1->extents.y1, reg2->extents.y1);
>         new_reg->extents.x2 = MIN (reg1->extents.x2, reg2->extents.x2);
>         new_reg->extents.y2 = MIN (reg1->extents.y2, reg2->extents.y2);
> 
>         FREE_DATA (new_reg);
> 
> 	new_reg->data = (region_data_type_t *)NULL;
> 
> There's nothing here to switch to pixman_region_empty_data.
> 
> So the question is whether to patch pixman or work around it in nsRegion.
> Since I'm not sure the pixman people would consider it a pixman bug, I
> incline to the latter, but I'm happy to make a pixman patch if someone tells
> me how to upstream it.

It looks like it should actually be _intersect_rects job to create an empty region if you pass in an empty rect.

Comment 97

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8459073 [details] [diff] [review]
Part 46: Work around pixman bug to make sure nsRegionRectIterator never returns an empty rect

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

I think this would be better fixed with a patch like the following:

diff --git a/gfx/src/nsRegion.h b/gfx/src/nsRegion.h
index 455b073..3827ae6 100644
--- a/gfx/src/nsRegion.h
+++ b/gfx/src/nsRegion.h
@@ -82,17 +82,25 @@ public:
     return *this;
   }
   nsRegion& And(const nsRect& aRect, const nsRegion& aRegion)
   {
     return And(aRegion, aRect);
   }
   nsRegion& And(const nsRegion& aRegion, const nsRect& aRect)
   {
-    pixman_region32_intersect_rect(&mImpl, aRegion.Impl(), aRect.x, aRect.y, aRect.width, aRect.height);
+    // pixman has a bug where it doesn't validate the input to intersect_rect
+    // and can end up with regions with empty rects, so take the more round about route
+    // if we're passing in an empty rect.
+    if (aRect.width == 0 || aRect.height == 0) {
+      nsRegion rect(aRect);
+      pixman_region32_intersect(&mImpl, aRegion.Impl(), rect.mImpl);
+    } else {
+      pixman_region32_intersect_rect(&mImpl, aRegion.Impl(), aRect.x, aRect.y, aRect.width, aRect.height);
+    }
     return *this;
   }
   nsRegion& And(const nsRect& aRect1, const nsRect& aRect2)
   {
     nsRect TmpRect;
 
     TmpRect.IntersectRect(aRect1, aRect2);
     return Copy(TmpRect);

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: backout for Aurora

Approval Request Comment
[Feature/regressing bug #]: 1022612
[User impact if declined]: regressions from 1022612 in Aurora without any user benefit (since 1022612 is mainly about laying groundwork for other features which won't land in Aurora)
[Describe test coverage new/current, TBPL]: none
[Risks and why]: practically no risk. Just backing out something that recently landed on central
[String/UUID change made/needed]: none

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a817719d9ea