785333 - Keep track of display-item data with merged frames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Lord [:cwiiis]]

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

Attachment: Store container layers against merged frames of container items

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Track merged frames of all display items

And this applies over the last one and stores layers against *all* items with merged frames.

Comment 3

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/79345542f853
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1756976e61d

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79345542f853
https://hg.mozilla.org/mozilla-central/rev/a1756976e61d

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Backed out due to bug 785626:
http://hg.mozilla.org/mozilla-central/rev/ee5484eb9f4a

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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

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

Attachment: 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]]

Attachment: Part 2 - Add ContainerLayer::RepositionChild

Add a reposition function to ContainerLayer to save from having to remove/add.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: 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]]

Attachment: 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]]

Attachment: Part 2 - Add ContainerLayer::RepositionChild

Here's a version that should actually build on Windows.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: 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]]

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

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

Attachment: 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]]

Attachment: 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]]

Attachment: 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]]

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

Attachment: 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 21

[Chris Lord [:cwiiis]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c06b09e067d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9023cfa7721
https://hg.mozilla.org/integration/mozilla-inbound/rev/2466826796a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d39a94c966a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ab93cb1541
https://hg.mozilla.org/integration/mozilla-inbound/rev/a36606002731

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c06b09e067d7
https://hg.mozilla.org/mozilla-central/rev/a9023cfa7721
https://hg.mozilla.org/mozilla-central/rev/2466826796a9
https://hg.mozilla.org/mozilla-central/rev/7d39a94c966a
https://hg.mozilla.org/mozilla-central/rev/d1ab93cb1541
https://hg.mozilla.org/mozilla-central/rev/a36606002731

Comment 23

[Virgil Dicu [:virgil] [QA]]

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

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

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

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

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

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

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

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

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

Comment 32

[Chris Lord [:cwiiis]]

Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc1cd7420bf4
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b6753834713
https://hg.mozilla.org/releases/mozilla-aurora/rev/9ab54a081255
https://hg.mozilla.org/releases/mozilla-aurora/rev/c587c7a7c669
https://hg.mozilla.org/releases/mozilla-aurora/rev/8192ce427a5d
https://hg.mozilla.org/releases/mozilla-aurora/rev/2753c3553b4a