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

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

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.

(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?

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.

(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?

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.

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).

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 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.

(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 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.

Hopefully the right patch this time. https://tbpl.mozilla.org/?tree=Try&rev=159b60005794

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 == ?

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...

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

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

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

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.

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

(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.

[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 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.

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

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

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

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

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