Closed Bug 1022612 Opened 6 years ago Closed 5 years ago

Remove ComputeVisibility pass when painting

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: roc, Assigned: roc)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(48 files, 1 obsolete file)

2.78 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.36 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.88 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
16.06 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.37 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
9.58 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.66 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.39 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.15 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
21.46 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
14.75 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.17 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.18 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.91 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.38 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.22 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.27 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.85 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.56 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.17 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.22 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.71 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.79 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.20 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.44 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.40 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
53.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.95 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
9.09 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.18 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.49 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.32 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
37.53 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.73 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.87 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
14.80 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.16 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.48 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.87 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.66 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
20.45 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.50 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.69 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.97 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.97 KB, patch
mattwoodrow
: review+
jrmuizel
: review-
Details | Diff | Splinter Review
246.26 KB, patch
Details | Diff | Splinter Review
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?
Flags: needinfo?(mstange)
Flags: needinfo?(matt.woodrow)
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).
Flags: needinfo?(mstange)
Compositor-side occlusion culling would also be useful for bug 1021845.
(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.
Flags: needinfo?(matt.woodrow)
Blocks: 1026271
No longer blocks: 1026271
(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
Assignee: nobody → roc
Status: NEW → ASSIGNED
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.
Attachment #8452713 - Flags: review?(matt.woodrow)
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.
Attachment #8452714 - Flags: review?(matt.woodrow)
Also the assertion in TryMerge is going away because we're going to do TryMerge
first in FrameLayerBuilder.
Attachment #8452717 - Flags: review?(matt.woodrow)
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.
Attachment #8452718 - Flags: review?(matt.woodrow)
BuildContainerLayerFor now has to be able to mutate the passed-in display item
list.
Attachment #8452720 - Flags: review?(matt.woodrow)
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.
Attachment #8452721 - Flags: review?(matt.woodrow)
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.
Attachment #8452724 - Flags: review?(matt.woodrow)
The removed code should be a no-op as long as the window opaque region is
accurate enough.
Attachment #8452725 - Flags: review?(matt.woodrow)
This is unnecessary. FrameLayerBuilder sets the correct region.
Attachment #8452727 - Flags: review?(matt.woodrow)
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.
Attachment #8452729 - Flags: review?(matt.woodrow)
It's obsolete and no-one calls it.
Attachment #8452733 - Flags: review?(matt.woodrow)
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.
Attachment #8452738 - Flags: review?(matt.woodrow)
We need this to avoid constructing and painting unncecessarily large
ThebesLayers.
Attachment #8452742 - Flags: review?(matt.woodrow)
It is no longer called because FrameLayerBuilder always sets the visible
regions on layers now.
Attachment #8452743 - Flags: review?(matt.woodrow)
This hasn't been used for a while I guess.
Attachment #8452744 - Flags: review?(matt.woodrow)
RecomputeVisibilityForItems for the retained ThebesLayer already recomputes
visibility for all items in that layer, including items nested in other items.
Attachment #8452745 - Flags: review?(matt.woodrow)
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.
Attachment #8452746 - Flags: review?(matt.woodrow)
This is no longer needed thanks to the readback simplification.
Attachment #8452748 - Flags: review?(matt.woodrow)
Wrong bug, ignore the previous comment.
Attachment #8452711 - Flags: review?(matt.woodrow) → review+
Attachment #8452703 - Flags: review?(matt.woodrow) → review+
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
Attachment #8452712 - Flags: review?(matt.woodrow) → review+
Attachment #8452713 - Flags: review?(matt.woodrow) → review+
Attachment #8452714 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8452715 - Flags: review?(matt.woodrow) → review+
Attachment #8452717 - Flags: review?(matt.woodrow) → review+
Attachment #8452718 - Flags: review?(matt.woodrow) → review+
Attachment #8452720 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8452721 - Flags: review?(matt.woodrow) → review+
Attachment #8452716 - Flags: review?(matt.woodrow) → review+
Attachment #8452722 - Flags: review?(matt.woodrow) → review+
Attachment #8452723 - Flags: review?(matt.woodrow) → review+
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?
Attachment #8452724 - Flags: review?(matt.woodrow) → review+
Attachment #8452725 - Flags: review?(matt.woodrow) → review+
Attachment #8452727 - Flags: review?(matt.woodrow) → review+
Attachment #8452728 - Flags: review?(matt.woodrow) → review+
Attachment #8452729 - Flags: review?(matt.woodrow) → review+
Attachment #8452730 - Flags: review?(matt.woodrow) → review+
Attachment #8452731 - Flags: review?(matt.woodrow) → review+
Attachment #8452732 - Flags: review?(matt.woodrow) → review+
Attachment #8452733 - Flags: review?(matt.woodrow) → review+
Attachment #8452734 - Flags: review?(matt.woodrow) → review+
Attachment #8452735 - Flags: review?(matt.woodrow) → review+
Attachment #8452736 - Flags: review?(matt.woodrow) → review+
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 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.
Attachment #8452739 - Flags: review?(matt.woodrow) → review+
Attachment #8452740 - Flags: review?(matt.woodrow) → review+
Attachment #8452741 - Flags: review?(matt.woodrow) → review+
Attachment #8452743 - Flags: review?(matt.woodrow) → review+
Attachment #8452744 - Flags: review?(matt.woodrow) → review+
Attachment #8452745 - Flags: review?(matt.woodrow) → review+
Attachment #8452746 - Flags: review?(matt.woodrow) → review+
Attachment #8452747 - Flags: review?(matt.woodrow) → review+
Attachment #8452748 - Flags: review?(matt.woodrow) → review+
Attachment #8452749 - Flags: review?(matt.woodrow) → review+
Attachment #8452750 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8452742 - Flags: review?(matt.woodrow) → review+
Attachment #8452738 - Flags: review?(matt.woodrow)
(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.
(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.
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.
Attachment #8454207 - Flags: review?(matt.woodrow)
Attachment #8454209 - Flags: review?(matt.woodrow) → review+
Attachment #8454225 - Flags: review?(matt.woodrow) → review+
Attachment #8454207 - Flags: review?(matt.woodrow) → review+
Blocks: 1025914
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.
Attachment #8455078 - Flags: review?(matt.woodrow) → review+
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?
Attachment #8455127 - Flags: review?(matt.woodrow) → review+
(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 :-)
(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 :)
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.
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
Duplicate of this bug: 1038579
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.
Attachment #8456093 - Flags: review?(matt.woodrow) → review+
Attachment #8456094 - Flags: review?(matt.woodrow) → review+
Attachment #8456807 - Flags: review?(matt.woodrow) → review+
Attachment #8457765 - Flags: review?(matt.woodrow) → review+
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
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.
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.
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
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}.
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.
Jeff what do you think about Comment 88?
Flags: needinfo?(jmuizelaar)
Attachment #8459073 - Flags: review?(matt.woodrow) → review+
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
Depends on: 1041200
Depends on: 1041207
I'll probably back this out on Aurora after the uplift. No point in carrying the risk there.
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Part 20 ("Do the business") seems to have caused bug 1041608.
Depends on: 1041530
Depends on: 1041510
(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 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);
Attachment #8459073 - Flags: review-
Depends on: 1042096
Depends on: 1042186
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
Attachment #8460604 - Flags: approval-mozilla-aurora?
Depends on: 1042467
Attachment #8460604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1043160
Depends on: 1043163
Depends on: 1042318
Depends on: 1047780
Flags: needinfo?(jmuizelaar)
Blocks: 1052095
Depends on: 1059519
Depends on: 1059498
Blocks: 1072347
Duplicate of this bug: 1072347
Depends on: 1081185
Depends on: 1081560
Depends on: 1062792
Depends on: 1092842
Depends on: 1098266
Depends on: 1107403
Depends on: 1111753
Depends on: 1115237
Depends on: 1109036
Blocks: 1134311
You need to log in before you can comment on or make changes to this bug.