Last Comment Bug 785333 - Keep track of display-item data with merged frames
: Keep track of display-item data with merged frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Chris Lord [:cwiiis]
:
:
Mentors:
Depends on: 785626 786672 795591
Blocks: 776247 786904 788193
  Show dependency treegraph
 
Reported: 2012-08-24 02:01 PDT by Chris Lord [:cwiiis]
Modified: 2012-09-29 17:48 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Store container layers against merged frames of container items (4.54 KB, patch)
2012-08-24 02:04 PDT, Chris Lord [:cwiiis]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Track merged frames of all display items (10.34 KB, patch)
2012-08-24 03:36 PDT, Chris Lord [:cwiiis]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 1 (bonus) - Synchronise Layer Insert/Remove (14.18 KB, patch)
2012-08-28 11:00 PDT, Chris Lord [:cwiiis]
bas: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 2 - Add ContainerLayer::RepositionChild (31.35 KB, patch)
2012-08-28 11:01 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Part 3 - Change how old layers are removed in ContainerState::Finish (2.82 KB, patch)
2012-08-28 11:03 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Part 6 - Track layer ownership during construction (4.87 KB, patch)
2012-08-28 11:06 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review
Part 2 - Add ContainerLayer::RepositionChild (31.99 KB, patch)
2012-08-28 11:59 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Part 2 - Add ContainerLayer::RepositionChild (31.98 KB, patch)
2012-08-28 12:17 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Splinter Review
Part 3 - Change how old layers are removed in ContainerState::Finish (3.57 KB, patch)
2012-08-29 01:14 PDT, Chris Lord [:cwiiis]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 6 - Track layer ownership during construction (6.73 KB, patch)
2012-08-29 01:17 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Splinter Review
Part 2 - Add ContainerLayer::RepositionChild (32.74 KB, patch)
2012-08-29 02:40 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 6 - Track layer ownership during construction (7.35 KB, patch)
2012-08-29 03:50 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2012-08-24 02:01:01 PDT
Sometimes we redraw more than is necessary because the underlying frame of an item has changed. We mitigate against this in nsDisplayScroll, for example, by swapping the underlying frame when merging.

Rather than doing it this way, we could keep track of display-item data for merged frames and when a layer isn't found, check for layers against merged frames as well. This would protect against the underlying frame changing to another merged frame, and from the underlying frame changing to a new frame (but keeping other merged frames). This would also allow us to remove the aforementioned code in nsDisplayScroll (I've tested this).

I'll split this patch up between doing this for container items and then non-container items.
Comment 1 Chris Lord [:cwiiis] 2012-08-24 02:04:26 PDT
Created attachment 654962 [details] [diff] [review]
Store container layers against merged frames of container items
Comment 2 Chris Lord [:cwiiis] 2012-08-24 03:36:05 PDT
Created attachment 654970 [details] [diff] [review]
Track merged frames of all display items

And this applies over the last one and stores layers against *all* items with merged frames.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-27 16:44:20 PDT
Backed out due to bug 785626:
http://hg.mozilla.org/mozilla-central/rev/ee5484eb9f4a
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-27 18:45:57 PDT
(also backed out on Aurora, post-merge, so this is backed out everywhere: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/b766865d2857)
Comment 7 Chris Lord [:cwiiis] 2012-08-28 09:52:30 PDT
This caused crashes due to merged items separating into multiple layers (due to things becoming visible between merged items). Without this code, no layers are retained in this situation, so I think it's worthwhile to fix it, even if the retaining we do in this situation is limited (at the very least, it reduces layer creation/destruction churn).

I have a somewhat lengthy fix incoming (lengthy as it required layer additions to change how ContainerState::Finish worked efficiently - the underlying fix is actually pretty small).
Comment 8 Chris Lord [:cwiiis] 2012-08-28 11:00:15 PDT
Created attachment 656101 [details] [diff] [review]
Part 1 (bonus) - Synchronise Layer Insert/Remove

I needed to change some layer stuff and it made sense to do this at the same time. Currently, D3D/GL layers have inefficient insert/remove functions, this just synchronises them with the implementation in BasicContainerLayer.
Comment 9 Chris Lord [:cwiiis] 2012-08-28 11:01:32 PDT
Created attachment 656102 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Add a reposition function to ContainerLayer to save from having to remove/add.
Comment 10 Chris Lord [:cwiiis] 2012-08-28 11:03:43 PDT
Created attachment 656104 [details] [diff] [review]
Part 3 - Change how old layers are removed in ContainerState::Finish

This changes ContainerState::Finish from assuming that layer ordering is always unchanged to allowing it to change, by using ContainerLayer::Reposition instead of its current iterate+remove loop.
Comment 11 Chris Lord [:cwiiis] 2012-08-28 11:06:47 PDT
Created attachment 656106 [details] [diff] [review]
Part 6 - Track layer ownership during construction

This applies on top of attachment #654962 [details] [diff] [review] and attachment #654970 [details] [diff] [review], and adds the necessary code to avoid the assertion in ProcessDisplayItems.

This users LayerUserData to track when a layer has been associated with a frame during construction, to prevent it being found again by another frame.

This is necessary for the situation where frames that were previously merged are now represented on separate layers due to items becoming visible in-between (this is what caused the crash in bug #785626).
Comment 12 Chris Lord [:cwiiis] 2012-08-28 11:59:12 PDT
Created attachment 656140 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Here's a version that should actually build on Windows.
Comment 13 Chris Lord [:cwiiis] 2012-08-28 12:17:32 PDT
Created attachment 656151 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Sorry for churn, here's a version that should be less crashy :p If this still has problems, I promise I'll sleep on it and wait for try before pushing another...
Comment 14 Chris Lord [:cwiiis] 2012-08-28 12:25:08 PDT
Comment on attachment 656151 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Replacing r=bas, didn't notice the r+ between adding revisions (confirmed on IRC).
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-28 19:15:04 PDT
Comment on attachment 656104 [details] [diff] [review]
Part 3 - Change how old layers are removed in ContainerState::Finish

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2051,5 @@
>  
> +    // Remove layers that have disappeared
> +    if (prevChild) {
> +      Layer* layer;
> +      while((layer = prevChild->GetNextSibling())) {

I think we can move this block out of the loop now instead of having an iteration with i==mNewChildLayers.Length()
Comment 16 Chris Lord [:cwiiis] 2012-08-29 01:14:24 PDT
Created attachment 656362 [details] [diff] [review]
Part 3 - Change how old layers are removed in ContainerState::Finish

This makes the suggested change, but I'm still seeing an orange + crash on M2, so looking at that. Possibly something I've done wrong with RepositionChild.

Would still care for review, I won't push until everything is green of course.
Comment 17 Chris Lord [:cwiiis] 2012-08-29 01:17:39 PDT
Created attachment 656364 [details] [diff] [review]
Part 6 - Track layer ownership during construction

Carrying r+.

This just moves the userdata removal from ContainerState::Finish to FrameLayerBuilder::DidEndTransaction. The previous patch only removed the data of container children, so could conceivably have still broken, I think. This is more reliable and clearer, anyway.
Comment 18 Chris Lord [:cwiiis] 2012-08-29 02:40:59 PDT
Created attachment 656379 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Carrying r=bas - This fixes the Shadow operation implementation that was causing the oranges (you can't pass a NULL argument over ipdl - added RaiseToTopChild to compensate).

Tests are looking green up to this point: https://tbpl.mozilla.org/?tree=Try&rev=12e22dc7a118
Comment 19 Chris Lord [:cwiiis] 2012-08-29 03:14:18 PDT
Heh, so this is now all green, but no longer fixes the changing frames in scroll-layer bug. Hopefully something small.
Comment 20 Chris Lord [:cwiiis] 2012-08-29 03:50:29 PDT
Created attachment 656391 [details] [diff] [review]
Part 6 - Track layer ownership during construction

It was something simple - I didn't notice the outer if statement on the call in DidEndTransaction. Modified the function to take flags and always be called, fixes the issue and still works, carrying r=roc.
Comment 23 Virgil Dicu [:virgil] [QA] 2012-09-10 02:53:51 PDT
These patches fixed the issue reported in bug 788193, but the problem is still present in Aurora. As per comment 5 in that bug, does it make sense to port this one to Aurora? Or should that be tracked separately/untracked?
Comment 24 Chris Lord [:cwiiis] 2012-09-10 03:00:41 PDT
(In reply to Virgil Dicu [:virgil] [QA] from comment #23)
> These patches fixed the issue reported in bug 788193, but the problem is
> still present in Aurora. As per comment 5 in that bug, does it make sense to
> port this one to Aurora? Or should that be tracked separately/untracked?

I think it makes sense to uplift these patches. They only missed the merge by a few days... I didn't request because these weren't solving a solid problem, just a problem I perceived could happen, and I was worried about the risk.

It's had time to bake and I haven't seen any reports directly related to this, so I'll request aurora approval.
Comment 25 Chris Lord [:cwiiis] 2012-09-10 03:03:43 PDT
Comment on attachment 656101 [details] [diff] [review]
Part 1 (bonus) - Synchronise Layer Insert/Remove

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Inefficient layer insertion and removal.
User impact if declined: None, except the other patches in this bug are based on top of this one.
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Practically no risk (the code is already used in the basic layers case).
String or UUID changes made by this patch: None
Comment 26 Chris Lord [:cwiiis] 2012-09-10 03:05:54 PDT
Comment on attachment 656379 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Inability to reposition layers (only remove/add, which involves more work)
User impact if declined: Other patches in this bug require this one.
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): No risk, these functions are unused except by the other patches in this bug.
String or UUID changes made by this patch: None.
Comment 27 Chris Lord [:cwiiis] 2012-09-10 03:09:46 PDT
Comment on attachment 656362 [details] [diff] [review]
Part 3 - Change how old layers are removed in ContainerState::Finish

[Approval Request Comment]
Bug caused by (feature/regressing bug #): If layers somehow change order, a crash will likely occur (there is an assertion that will trigger in this situation)
User impact if declined: None, except the other patches in this bug are based on top of this one.
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Low risk. Slightly less efficient than the old code in some situations, but more versatile/safer.
String or UUID changes made by this patch: None
Comment 28 Chris Lord [:cwiiis] 2012-09-10 03:16:06 PDT
Comment on attachment 654962 [details] [diff] [review]
Store container layers against merged frames of container items

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Full-layer invalidation may occur when container layers are associated with different frames (performance issue). Also possible bad layer association when invalidating areas (bug 788193)
User impact if declined: Possible performance regression, more visible on mobile.
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Requires part 6 of the patches in this bug. With part 6, small risk of exposed invalidation issues (without part 6, high risk of various elements disappearing + assertions)
String or UUID changes made by this patch: None
Comment 29 Chris Lord [:cwiiis] 2012-09-10 03:17:55 PDT
Comment on attachment 654970 [details] [diff] [review]
Track merged frames of all display items

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Full-layer invalidation may occur when non-container layers are associated with different frames (performance issue).
User impact if declined: Possible performance regression, more visible on mobile.
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Small risk of exposed invalidation issues.
String or UUID changes made by this patch: None
Comment 30 Chris Lord [:cwiiis] 2012-09-10 03:21:15 PDT
Comment on attachment 656391 [details] [diff] [review]
Part 6 - Track layer ownership during construction

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Layers associating with multiple frames during layer-building
User impact if declined: Attachment #654962 [details] [diff] may cause elements to disappear, or possibly other bad side-effects
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None
Comment 31 Chris Lord [:cwiiis] 2012-09-10 03:22:18 PDT
(In reply to Chris Lord [:cwiiis] from comment #28)
> Comment on attachment 654962 [details] [diff] [review]
> Store container layers against merged frames of container items
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Full-layer invalidation may occur
> when container layers are associated with different frames (performance
> issue). Also possible bad layer association when invalidating areas (bug
> 788193)
> User impact if declined: Possible performance regression, more visible on
> mobile.
> Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
> Risk to taking this patch (and alternatives if risky): Requires part 6 of
> the patches in this bug. With part 6, small risk of exposed invalidation
> issues (without part 6, high risk of various elements disappearing +
> assertions)
> String or UUID changes made by this patch: None

Meant to say for the user impact here that you may end up with invalidation-related issues, like those in bug 788193.

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