A comment in ApplyThebesLayerInvalidation states: // We have to preserve the current contents of invalidThebesContent // because there might be multiple container layers for the same // frame and we need to invalidate the ThebesLayer children of all // of them. Also, multiple calls to ApplyThebesLayerInvalidation for the // same layer can share the same region. But after each call to ApplyThebesLayerInvalidation, SetHasContainerLayer is called, which empties the invalid region. Currently, this is manifesting in bug #769541, where invalidations to fixed position elements are getting lost because they are merged in the root scroll layer item, as well as having their own layer which is processed later.

Been having a think about this... How about this in FrameLayerBuilder: - SetHasContainerLayer is altered to not modify the invalid region - During BuildContainerLayerFor: - we use the DisplayItemDataEntry to mark container frames as needing to be set empty - we store the relevant RefCountedRegion to be set on merged frames - During UpdateDisplayItemDataForFrame, we look at the DisplayItemDataEntry and set the ThebesLayerInvalidRegionProperty empty for the container frames / set the RefCountedRegion in the merged frames That should allow for the layer-building to process all invalidation data before setting it to empty immediately afterwards (which should, I think, give us the behaviour we want?)

Here's a patch that does as I described in comment #1 and seems to fix the issue for me. It could probably do with some tidying, or perhaps being done in a more elegant way, r?'ing in case this is good enough (given it'll be obsoleted by dlbi anyway).

Comment on attachment 640317 [details] [diff] [review] Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon Review of attachment 640317 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +755,5 @@ > + // Reset the invalid region now so we can start collecting new dirty areas. > + if (aEntry->mClearInvalidRegion) { > + if (aEntry->mInvalidRegion) { > + aEntry->mInvalidRegion->mRegion.SetEmpty(); > + } I think we can always clear mInvalidRegion here. No need for mInvalidRegion. ::: layout/base/FrameLayerBuilder.h @@ +462,5 @@ > nsAutoTArray<DisplayItemData, 1> mData; > bool mIsSharingContainerLayer; > > + bool mClearInvalidRegion; > + RefCountedRegion* mInvalidRegion; nsRefPtr<RefCountedRegion>, that will simplify quite a bit of this patch. And put this below mData for better packing.

Done as suggested - much smaller now :)

Sorry for the churn, noticed a comment that was inaccurate and decided to correct/clarify it.

https://tbpl.mozilla.org/?tree=Try&rev=4840336358ee There are some oranges on Linux that I assume are showing up now because extra layers are introducing rounding errors and those extra layers are now actually being invalidated properly... I think it might be sensible to check when making the fixed-position displaylist items whether there's a displaylist set and only do it in that situation.

Just to note, even with https://hg.mozilla.org/try/rev/cfb5ac280da3 applied, there are still some oranges: https://tbpl.mozilla.org/?tree=Try&rev=be0743c1e90e Looking into them - A couple of reftests may need fuzzing for a few pixels.

This is blocking on the oranges and a crasher as shown in this test run: https://tbpl.mozilla.org/?tree=Try&rev=aee702b0392c I'm running talos locally, but can't reproduce the crash and the stacktrace in the log isn't useful... The orange mochitest is also worrying - test_transformed_scrolling_repaints(_2) are failing, I guess because fixing this bug has uncovered another invalidation bug where too much invalidation is being done. I fail to see how this patch would cause this behaviour, except by uncovering invalidations that should have been happening. The reftest oranges don't worry me, they're only a matter of a couple of pixels being indistinguisably different, so these can be fuzzed. I'm still looking into this, but any help or suggestions are very much appreciated.

I've fixed the red, just hunting down the orange now.

This will be rolled into the original patch, I'm including it separately for ease of reviewing.

Comment on attachment 641515 [details] [diff] [review] Fix crasher Review of attachment 641515 [details] [diff] [review]: ----------------------------------------------------------------- You're the man.

And this one fixes the oranges (don't know if it fixes the reftests yet, waiting on a try build). It was entirely my fault, so apologies (again) for pointing the finger at other bits of code - I hadn't fully understood the iterations that happen in WillEndTransaction, and so I ended up just not clearing the invalidations in some cases.

Folded and pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed

Comment on attachment 640584 [details] [diff] [review] Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon Review of attachment 640584 [details] [diff] [review]: ----------------------------------------------------------------- Need this on Beta to fix a nasty Web-facing regression.

Comment on attachment 641515 [details] [diff] [review] Fix crasher Review of attachment 641515 [details] [diff] [review]: ----------------------------------------------------------------- Need this on Beta to fix a nasty Web-facing regression.

Comment on attachment 641943 [details] [diff] [review] And fix the oranges Review of attachment 641943 [details] [diff] [review]: ----------------------------------------------------------------- Need this on Beta to fix a nasty Web-facing regression.

Does beta approval also count for aurora approval? We'll need this on aurora too, as bug 758620 has landed there and can cause visible rendering errors without this and bug 769541.

Comment on attachment 641943 [details] [diff] [review] And fix the oranges Review of attachment 641943 [details] [diff] [review]: ----------------------------------------------------------------- Yes, we need this on Aurora too.

Comment on attachment 641515 [details] [diff] [review] Fix crasher Review of attachment 641515 [details] [diff] [review]: ----------------------------------------------------------------- Yes, we need this on Aurora too.

Comment on attachment 640584 [details] [diff] [review] Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon Review of attachment 640584 [details] [diff] [review]: ----------------------------------------------------------------- Yes, we need this on Aurora too.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21) > Comment on attachment 640584 [details] [diff] [review] > Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon > > Review of attachment 640584 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes, we need this on Aurora too. Would you mind providing a risk assessment?

There's a risk of regressing invalidation in some other way, but the bugs already here are quite bad so I think the benefit outweighs the risk.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > There's a risk of regressing invalidation in some other way, but the bugs > already here are quite bad so I think the benefit outweighs the risk. Thanks. Comment 18 and bug 769541 suggests bug 758620 as the main driver for taking this patch, but that only appears to have landed in FN16. What other user facing regressions have been seen (in 15)?

Bug 775592 and bug 775163.

Actually this is already on Aurora since it landed before the uplift. Beta approval still wanted.

Comment on attachment 641943 [details] [diff] [review] And fix the oranges Approving for Beta so we can get fixes for the user-facing regressions in comment 25.

I've rebased this (trivial) and just doing a local build to make sure it's alright before pushing to beta.

Pushed to beta: http://hg.mozilla.org/releases/mozilla-beta/rev/afbe9366cccc