815666 - CSS 3D transform visibility is broken after landing 806256

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Alice0775 White]

This is spin off from bug 807563 

Screen shot;
https://bugzilla.mozilla.org/attachment.cgi?id=678183

Steps to reproduce:
1. Open http://desandro.github.com/3dtransforms/examples/carousel-01.html

Actual results:
Some planes(#3 and #8) are missing at the time of initial load of the page

Expected results:
All planes should draw

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/be0d3f1de9c2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121031180512
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/01123067aa58
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121031182652
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=be0d3f1de9c2&tochange=01123067aa58

Comment 1

[Alex Keybl [:akeybl]]

Since we know of major web properties that use 3d transforms for galleries, it makes sense to track this FF19 regression.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Alright, think I understand this now.

The regression happened when we stopped calling ComputePreserve3DChildrenOverflow every time we reflowed a preserve-3d frame, and only called it should actually come up with something new.

Unfortuntately, because of the way that we handle overflow rects within a preserve-3d chain, other code that collates the overflow areas of child frames (nsBlockFrame::ComputeOverflowAreas, ConsiderChildOverflow at least, not sure how many others there are) don't actually compute these correctly.

We previously let these compute the wrong values, and then always overwrote them with the right ones.

Finding all the places we do this and making them correct would fix this.

Another alternatives is to change overflow rects to be 3d. This handles the cases where an intermediate frame in a preserve-3d chain has a singular transform, and thus has an empty overflow rect, but the combined chain isn't singular. We currently handle this by lying about the overflow rects, and storing them transformed up to the root of the preserve-3d chain instead of relative to the current frame's transform.

The reason the change was made initially was because when we have a preserve-3d parent with a large number of children, and we change *all* the child transforms, we do them one by one, and recurse all children updating overflows each time. We could try find a way to coalesce these and avoid the O(n^2) behaviour that way instead.

roc: Any thoughts on the above ideas? If we go for the first option, then I might need help finding all the code that needs changing.

Comment 3

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Unfortuntately, because of the way that we handle overflow rects within a
> preserve-3d chain, other code that collates the overflow areas of child
> frames (nsBlockFrame::ComputeOverflowAreas, ConsiderChildOverflow at least,
> not sure how many others there are) don't actually compute these correctly.
> 
> We previously let these compute the wrong values, and then always overwrote
> them with the right ones.

Can you explain further why this isn't adequate?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Overwriting no longer works, because we no longer call it all the time.

As for accumulating the overflow, for normal frames we accumulate the combined overflow areas of all children, and then transform that by the frame's transform (if there is any).

For a preserve-3d frame, some children might have overflow rects already in our coordinate space, and others not. So we need to transform (or not transform) each of the child overflow rects individually and accumulate the post-transform regions instead.

Comment 5

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Overwriting no longer works, because we no longer call it all the time.

How about ensuring that ComputePreserve3DChildrenOverflow runs whenever any preserve-3d descendant's overflow area changes?

Comment 6

[Matt Woodrow (:mattwoodrow)]

That has O(N^2) behaviour when we change the transform on N descendants. This was the original reason for terrible performance on the periodic table demo.

Comment 7

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

That's because we have a lot of UpdateTranformLayer hints and a lot of UpdateOverflow calls but no reflow, right?

> We could try find a way to coalesce these and avoid the O(n^2) behaviour that way
> instead.

So I guess we should try this then.

How about having nsCSSFrameConstructor::ProcessRestyledFrames collect a set of frames that need UpdateOverflow called on them (and their ancestors updated), and then process that set in the "right" order?

One way to do that would be to use a worklist of (frame, depth in frame tree), keeping it sorted by depth in frame tree. Then you can repeatedly pull a deepest frame from the worklist, call UpdateOverflow on it, and if that returns true add its parent to the worklist (if it's not already there).

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Add a helper class to coalesce frames that need their overflow updated

GetDepthInFrame tree could potentially be slow, not sure if it's worth computing this and caching it though.

nsTArray also isn't the best for storage, but priority queue/heap doesn't provide a good way to implement Contains(), and our linked list implementation (from mfbt) doesn't implement sorted insertions. I guess that could be changed though.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times

Comment 10

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

Comment on attachment 688993 [details] [diff] [review]
Add a helper class to coalesce frames that need their overflow updated

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

::: layout/base/RestyleTracker.h
@@ +70,5 @@
> +      
> +      // If the overflow changed, then we want to also update the parent's
> +      // overflow
> +      if (overflowChanged || entry.mFrame->UpdateOverflow()) {
> +        AddFrameInternal(entry.mFrame->GetParent(), false);

If you add the depth as a parameter to AddFrameInternal, then you can pass entry.mDepth - 1 here and avoid a lot of calls to GetDepthInFrameTree.

@@ +116,5 @@
> +    }
> +
> +    Entry entry(aFrame, aInitial);
> +    if (mEntryList.BinaryIndexOf(entry) == mEntryList.NoIndex) {
> +      mEntryList.InsertElementSorted(entry);

Wouldn't we need to set an existing element's mInitial in some cases?

@@ +121,5 @@
> +    }
> +  }
> +
> +  /* A list of frames to process, sorted by their depth in the frame tree */
> +  nsTArray<Entry> mEntryList;

Make this a max-heap. Then insertion, and removing a maximal element, can be done in log(N) time. nsTArray has helpers for this.

Comment 11

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> nsTArray also isn't the best for storage, but priority queue/heap doesn't
> provide a good way to implement Contains(),

Oh, I missed that. Hmm.

I think you can use a max-heap ordered by depth and then frame pointer. Then you can simply not do a Contains() check and allow the same frame to be inserted multiple times. When you remove the max element (which is always at element 0 in the nsTArray), you can check if the new max element is the same frame, and keep removing the max element while it is the same frame.

An alternative would be to use some kind of balanced binary tree, but I don't know of one in our code already.

Comment 12

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

Comment on attachment 688994 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times

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

I think you uploaded the wrong patch here.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Add a helper class to coalesce frames that need their overflow updated v2

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times v2

Hopefully the right patch this time.

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

Comment 15

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

Comment on attachment 689129 [details] [diff] [review]
Add a helper class to coalesce frames that need their overflow updated v2

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

Looks good but that !=/== issue is worrying...

::: layout/base/RestyleTracker.h
@@ +30,5 @@
> +{
> +public:
> +
> +  /**
> +   * Add a frame that has had a style changes, and needs its

a style change

@@ +54,5 @@
> +  void Flush() {
> +    while (!mEntryList.IsEmpty()) {
> +      Entry entry = mEntryList.Pop();
> +
> +

Remove one of these blank lines

@@ +58,5 @@
> +
> +      // Pop off any duplicate entries and copy back mInitial
> +      // if any have it set.
> +      while (!mEntryList.IsEmpty() &&
> +             mEntryList.Top().mFrame != entry.mFrame) {

Shouldn't this line be == ?

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Add a helper class to coalesce frames that need their overflow updated v3

Yes, it definitely should be ==. I moved some code around and forgot to reverse it. Interestingly that didn't ruin performance on the periodic table demo...

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/767accc64cf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0319dd845d53

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/767accc64cf8
https://hg.mozilla.org/mozilla-central/rev/0319dd845d53

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Looking for an aurora uplift nom here so we don't ship this regression ever.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 689130 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times v2

This approval request is for both patches on this bug.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 806256
User impact if declined: Flicking / missing content on pages with 3d transforms
Testing completed (on m-c, etc.): Been on m-c for over a week.
Risk to taking this patch (and alternatives if risky): Low risk, just coalesces unnecessarily repeated function calls.
String or UUID changes made by this patch: None.

Comment 22

[Matt Woodrow (:mattwoodrow)]

We should probably wait until bug 822906 is resolved before uplifting this.

Comment 23

[Alex Keybl [:akeybl]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> We should probably wait until bug 822906 is resolved before uplifting this.

We're still waiting on this for the uplift to FF19. Note that FF20 (now on Aurora) is impacted by bug 822906 though.

Comment 24

[bhavana bajaj [:bajaj]]

[Relman]We sill need to land patch in Bug 822906 (just landed on mozilla-inbound) and the patch in this bug, at the same time.

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 689130 [details] [diff] [review]
Use OverflowUpdateTracker to avoid calling UpdateOverflow on the same frame multiple times v2

Approving for Beta alongside bug 822906. Please land asap today to make it into Beta 3.

Comment 26

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/releases/mozilla-beta/rev/97e0102c59c0
https://hg.mozilla.org/releases/mozilla-beta/rev/6e334c229091

Comment 27

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/releases/mozilla-beta/rev/24dc8d9b0a3a

Comment 28

[Alex Keybl [:akeybl]]

We're going to be backing out bug 806256 and bug 807563 from FF19, so this bug won't be necessary.

Comment 29

[Mihai Morar, (:MihaiMorar)]

I'd reproduce this issue on Nightly it was reported:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0(20121127030907)

I couldn't reproduce this issue on FF 19b4 and Latest Aurora 20 on Windows 7 x64, Ubuntu x64 and Mac OS 10.8 x64 as following attachments shows:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0(20130130080006)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130130 Firefox/20.0(20130130042019)

Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0(20130130080006)
Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130130 Firefox/20.0
(20130130042019)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0   20130130080006
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130130 Firefox/20.0   20130130042019

Comment 30

[Mihai Morar, (:MihaiMorar)]

Attachment: Screenshot Windows

Comment 31

[Mihai Morar, (:MihaiMorar)]

Attachment: Linux

Comment 32

[Mihai Morar, (:MihaiMorar)]

Attachment: Mac Screenshot

Comment 33

[Mihai Morar, (:MihaiMorar)]

Based on my verifications done in Comment 29 30 31 and 32 I confirm this fix is verified.