The default bug view has changed. See this FAQ.

Keep track of display-item data with merged frames

RESOLVED FIXED in Firefox 17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(6 attachments, 6 obsolete attachments)

4.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.18 KB, patch
bas
: review+
Details | Diff | Splinter Review
3.57 KB, patch
roc
: review+
Details | Diff | Splinter Review
32.74 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
7.35 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 654962 [details] [diff] [review]
Store container layers against merged frames of container items
Attachment #654962 - Flags: review?(roc)
(Assignee)

Comment 2

5 years ago
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.
Attachment #654970 - Flags: review?(roc)
Attachment #654962 - Flags: review?(roc) → review+
Attachment #654970 - Flags: review?(roc) → review+
(Assignee)

Comment 3

5 years ago
Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/79345542f853
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1756976e61d
https://hg.mozilla.org/mozilla-central/rev/79345542f853
https://hg.mozilla.org/mozilla-central/rev/a1756976e61d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 785626

Updated

5 years ago
Depends on: 785816
Backed out due to bug 785626:
http://hg.mozilla.org/mozilla-central/rev/ee5484eb9f4a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
No longer depends on: 785816
(also backed out on Aurora, post-merge, so this is backed out everywhere: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/b766865d2857)
(Assignee)

Comment 7

5 years ago
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).
(Assignee)

Comment 8

5 years ago
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.
Attachment #656101 - Flags: review?(bas.schouten)
(Assignee)

Comment 9

5 years ago
Created attachment 656102 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Add a reposition function to ContainerLayer to save from having to remove/add.
Attachment #656102 - Flags: review?(bas.schouten)
(Assignee)

Comment 10

5 years ago
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.
Attachment #656104 - Flags: review?(roc)
(Assignee)

Comment 11

5 years ago
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).
Attachment #656106 - Flags: review?(roc)
(Assignee)

Comment 12

5 years ago
Created attachment 656140 [details] [diff] [review]
Part 2 - Add ContainerLayer::RepositionChild

Here's a version that should actually build on Windows.
Attachment #656102 - Attachment is obsolete: true
Attachment #656102 - Flags: review?(bas.schouten)
Attachment #656140 - Flags: review?(bas.schouten)
Attachment #656101 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 13

5 years ago
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...
Attachment #656140 - Attachment is obsolete: true
Attachment #656140 - Flags: review?(bas.schouten)
Attachment #656151 - Flags: review?(bas.schouten)
(Assignee)

Comment 14

5 years ago
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).
Attachment #656151 - Flags: review?(bas.schouten) → review+
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()
Attachment #656106 - Flags: review?(roc) → review+
(Assignee)

Comment 16

5 years ago
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.
Attachment #656104 - Attachment is obsolete: true
Attachment #656104 - Flags: review?(roc)
Attachment #656362 - Flags: review?(roc)
(Assignee)

Comment 17

5 years ago
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.
Attachment #656106 - Attachment is obsolete: true
Attachment #656364 - Flags: review+
Attachment #656362 - Flags: review?(roc) → review+
(Assignee)

Comment 18

5 years ago
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
Attachment #656151 - Attachment is obsolete: true
Attachment #656379 - Flags: review+
(Assignee)

Comment 19

5 years ago
Heh, so this is now all green, but no longer fixes the changing frames in scroll-layer bug. Hopefully something small.
(Assignee)

Comment 20

5 years ago
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.
Attachment #656364 - Attachment is obsolete: true
Attachment #656391 - Flags: review+
(Assignee)

Comment 21

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 786672
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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?
(Assignee)

Comment 24

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 788193
(Assignee)

Comment 25

5 years ago
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
Attachment #656101 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

5 years ago
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.
Attachment #656379 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

5 years ago
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
Attachment #656362 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

5 years ago
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
Attachment #654962 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

5 years ago
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
Attachment #654970 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 30

5 years ago
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
Attachment #656391 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 31

5 years ago
(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.
Attachment #654962 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #654970 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656101 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656379 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656391 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 786904
Blocks: 776247
(Assignee)

Comment 32

5 years ago
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

Updated

5 years ago
status-firefox17: --- → fixed
Depends on: 795591
You need to log in before you can comment on or make changes to this bug.