772079 - invalidThebesContent is immediately destroyed after calling ApplyThebesLayerInvalidation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Lord [:cwiiis]]

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.

Comment 1

[Chris Lord [:cwiiis]]

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

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon

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 3

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

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.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon

Done as suggested - much smaller now :)

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon

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

Comment 6

[Chris Lord [:cwiiis]]

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.

Comment 7

[Chris Lord [:cwiiis]]

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.

Comment 8

[Chris Lord [:cwiiis]]

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.

Comment 9

[Chris Lord [:cwiiis]]

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

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Fix crasher

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

Comment 11

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

Comment on attachment 641515 [details] [diff] [review]
Fix crasher

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

You're the man.

Comment 12

[Chris Lord [:cwiiis]]

Attachment: And fix the oranges

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.

Comment 13

[Chris Lord [:cwiiis]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7f80f6408ed

Comment 15

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

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 16

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

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 17

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

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.

Comment 18

[Chris Lord [:cwiiis]]

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 19

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

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 20

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

Comment on attachment 641515 [details] [diff] [review]
Fix crasher

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

Yes, we need this on Aurora too.

Comment 21

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

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.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 23

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

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.

Comment 24

[Alex Keybl [:akeybl]]

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

Comment 25

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

Bug 775592 and bug 775163.

Comment 26

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

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

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 28

[Chris Lord [:cwiiis]]

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

Comment 29

[Chris Lord [:cwiiis]]

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