Last Comment Bug 772079 - invalidThebesContent is immediately destroyed after calling ApplyThebesLayerInvalidation
: invalidThebesContent is immediately destroyed after calling ApplyThebesLayerI...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Chris Lord [:cwiiis]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 765677 769541 771529 775163 775592 777152 779404
  Show dependency treegraph
 
Reported: 2012-07-09 08:21 PDT by Chris Lord [:cwiiis]
Modified: 2012-08-02 00:33 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon (10.87 KB, patch)
2012-07-09 12:09 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon (10.71 KB, patch)
2012-07-10 06:53 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon (10.76 KB, patch)
2012-07-10 06:59 PDT, Chris Lord [:cwiiis]
roc: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Fix crasher (2.36 KB, patch)
2012-07-12 10:01 PDT, Chris Lord [:cwiiis]
roc: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
And fix the oranges (5.63 KB, patch)
2012-07-13 10:16 PDT, Chris Lord [:cwiiis]
roc: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2012-07-09 08:21:55 PDT
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] 2012-07-09 11:18:56 PDT
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] 2012-07-09 12:09:31 PDT
Created attachment 640317 [details] [diff] [review]
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) 2012-07-09 18:13:12 PDT
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] 2012-07-10 06:53:21 PDT
Created attachment 640581 [details] [diff] [review]
Don't reset/clear the ThebesLayerInvalidRegion property on frames too soon

Done as suggested - much smaller now :)
Comment 5 Chris Lord [:cwiiis] 2012-07-10 06:59:37 PDT
Created attachment 640584 [details] [diff] [review]
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] 2012-07-11 04:08:36 PDT
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] 2012-07-11 13:14:39 PDT
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] 2012-07-12 06:57:39 PDT
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] 2012-07-12 09:59:29 PDT
I've fixed the red, just hunting down the orange now.
Comment 10 Chris Lord [:cwiiis] 2012-07-12 10:01:11 PDT
Created attachment 641515 [details] [diff] [review]
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) 2012-07-12 16:57:10 PDT
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] 2012-07-13 10:16:35 PDT
Created attachment 641943 [details] [diff] [review]
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] 2012-07-14 00:50:33 PDT
Folded and pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-07-14 10:01:59 PDT
https://hg.mozilla.org/mozilla-central/rev/a7f80f6408ed
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-18 20:18:04 PDT
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) 2012-07-18 20:18:11 PDT
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) 2012-07-18 20:18:19 PDT
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] 2012-07-18 22:41:42 PDT
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) 2012-07-18 23:08:19 PDT
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) 2012-07-18 23:08:24 PDT
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) 2012-07-18 23:08:30 PDT
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 2012-07-20 15:14:01 PDT
(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) 2012-07-20 18:41:25 PDT
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] 2012-07-23 11:22:41 PDT
(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) 2012-07-23 17:03:42 PDT
Bug 775592 and bug 775163.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-23 19:58:29 PDT
Actually this is already on Aurora since it landed before the uplift. Beta approval still wanted.
Comment 27 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-24 12:00:36 PDT
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] 2012-07-25 00:40:31 PDT
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] 2012-07-25 01:10:32 PDT
Pushed to beta: http://hg.mozilla.org/releases/mozilla-beta/rev/afbe9366cccc

Note You need to log in before you can comment on or make changes to this bug.