Put all the block's text-overflow markers at the bottom of PositionedDescendants()

RESOLVED FIXED in mozilla17

Status

()

Core
Layout: Block and Inline
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mats, Assigned: dholbert)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Follow-up from bug 735898 comment 58.

Currently we add them line-by-line on the top which means that unless
someone content-sorts this list, they'll display on top of positioned
children on earlier lines.  We should collect them on a separate list
for the block and add that list to the bottom of PositionedDescendants()
after all the lines are processed for text-overflow.  That way, the
PositionedDescendants() list will be in content-order (which makes
the optimization in bug 735898 part 8 possible, but we should fix this
anyway).

I'll try to fix this within the next week or so, unless someone beats
me to it.
I'll let you fix it :-)
Duplicate of this bug: 772690
mats: are you actively working on this / any updates on this?

(We'll likely need bug 735898 part 8 to get flexbox paint-ordering correct, in certain cases involving the 'order' property, so I'm interested in seeing that patch land & hence interested in fixing this bug. :)  If you're not working on this, mind if I steal it?)
Version: unspecified → Trunk
(Reporter)

Comment 4

5 years ago
Created attachment 640991 [details] [diff] [review]
patch 1: put text-overflow markers at bottom of PositionedDescendants()

Feel free to take the bug.  FWIW, I wrote this patch some time ago,
but Try reported a leak and I couldn't figure it out at the time.
FWIW, the leak is:

> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1632 bytes during test execution
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 12 instances of nsDisplayText with size 64 bytes each (768 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 48 instances of nsRect with size 16 bytes each (768 bytes total)
> TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 12 instances of nsTArray_base with size 8 bytes each (96 bytes total)

I was able to reproduce it about 50% of the time by running
  TEST_PATH=layout/generic/crashtests/crashtests.list  make crashtest
in a local debug build with the patch applied.

However, the leak disappeared when I tried to bisect the manifest file -- if I delete either the first half or the second half, the leak goes away.  (I also tried deleting everything but the text-overflow crashtests, but that didn't reproduce the leak either.)

So there appears to be some sort of race involved.
Created attachment 644167 [details] [diff] [review]
patch 2: remove aLists parameter

In any case, here's a followup to the existing patch that removes a paerameter "aLists" that's no longer needed in a few places.
Created attachment 644168 [details] [diff] [review]
patch 2 v2: remove aLists parameter

(Here's that patch again, without any accidentally-included crashtest.list tweaks)
Attachment #644167 - Attachment is obsolete: true
At least locally, I can fix the leaks by replacing AppendToBottom() with AppendToTop() -- so I think this might just be a bug in AppendToBottom().  (It's not used extensively right now)

That function looks like this right now:

1006   /**
1007    * Removes all items from aList and prepends them to the bottom of this list
1008    */
1009   void AppendToBottom(nsDisplayList* aList) {
1010     if (aList->mSentinel.mAbove) {
1011       aList->mTop->mAbove = mSentinel.mAbove;
1012       mTop = aList->mTop;
1013       mSentinel.mAbove = aList->mSentinel.mAbove;
1014            
1015       aList->mTop = &aList->mSentinel;
1016       aList->mSentinel.mAbove = nsnull;
1017     }
1018   }
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h#984

From some linked-list paper-doodling, I think line 1012 there isn't correct. ("mTop = aList->mTop;")  We want to prepend aList to ourselves, and that normally doesn't affect the *topmost* item in our list -- so mTop should stay unchanged, generally.  (We'd only want to touch it if we were the empty list -- in that case, line 1012 would be correct.)
Attachment #644168 - Flags: review?(matspal)
Created attachment 644588 [details] [diff] [review]
Fix AppendToBottom bug [spun off as bug 776306]

I think this fixes the problem described in the previous comment.

Graphically: if we've got two nonempty lists, like so:

 list1: mSentinel-->A-->B
        mTop------------^

 list2: mSentinel-->C-->D
        mTop------------^

...and we call list1.prepend(list2), then we do NOT want to update list1.mTop.  

We want to end up with
 list1: mSentinel-->C-->D-->A-->B
        mTop--------------------^
with mTop still pointing to B.



HOWEVER, if list1 is empty...
 list1: mSentinel-->[null]
        mTop---^
and we prepend list2 to it, (basically popuplating it with list2's contents), *then* we *do* want to set list1.mTop.
Attachment #644588 - Flags: review?(matspal)
(Note that the code I added basically matches the equivalent clause in the single-nsDisplayItem version of AppendToBottom:
 https://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h?mark=989-991#984
)
Here's a try run with all 3 of this bug's attached patches
(mats' main patch & my patches 2 & 3):
  https://tbpl.mozilla.org/?tree=Try&rev=17667aab6af0

...and with the SortByZOrder speedup (bug 735898 part 8) layered on top of those:
  https://tbpl.mozilla.org/?tree=Try&rev=3c223f25df56
Comment on attachment 644588 [details] [diff] [review]
Fix AppendToBottom bug [spun off as bug 776306]

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

Good catch!
Attachment #644588 - Flags: review?(matspal) → review+
Comment on attachment 640991 [details] [diff] [review]
patch 1: put text-overflow markers at bottom of PositionedDescendants()

Thanks!  Both try runs are green, modulo randomorange -- hooray!

I'm pretty sure mats' first patch was ready for review, too (aside from the fact that it was indirectly causing leaks).  Requesting review on his behalf on that one.
Attachment #640991 - Attachment description: wip → patch 1: put text-overflow markers at bottom of PositionedDescendants()
Attachment #640991 - Flags: review?(roc)
Comment on attachment 640991 [details] [diff] [review]
patch 1: put text-overflow markers at bottom of PositionedDescendants()

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

::: layout/generic/nsBlockFrame.cpp
@@ +6309,5 @@
> +  // Pick up the resulting text-overflow markers.  They are associated with
> +  // the block so we put them at the bottom so that the list stays sorted in
> +  // content order.
> +  if (textOverflow) {
> +    aLists.PositionedDescendants()->AppendToBottom(&textOverflow->GetMarkers());

I don't think this is quite right. aLists may have PositionedDescendants that are below this block. This code would put our markers below those PositionedDescendants.

Instead I think we need to have the DisplayLine calls all append to an nsDisplayListCollection; after that we append the markers to aLists.PositionedDescendants() and then append the new collection.
Which means, I guess, that we don't need to call AppendToBottom...
(Reporter)

Updated

5 years ago
Attachment #644168 - Flags: review?(matspal) → review+
(Reporter)

Comment 16

5 years ago
Good catch finding the leak!
Assignee: matspal → dholbert
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Which means, I guess, that we don't need to call AppendToBottom...

I'll file a separate bug for that, then, and finish up mats' patch here.
Attachment #644588 - Attachment description: patch 3 v1: Fix AppendToBottom bug → Fix AppendToBottom bug [spun off as bug 776306]
Created attachment 644798 [details] [diff] [review]
patch 1 v2

Stole the patch with mats' blessing, and addressed Comment 14's review comments.

(The nsBlockFrame chunks are the only thing changed from the previous revision of this patch.)
Attachment #640991 - Attachment is obsolete: true
Attachment #640991 - Flags: review?(roc)
Attachment #644798 - Flags: review?(roc)
Attachment #644798 - Flags: review?(roc) → review+
Landed patch 1 and patch 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/e4040216dff9
  https://hg.mozilla.org/integration/mozilla-inbound/rev/7295d036a37b

(and the other patch already landed in bug 776306, per comment 17)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/e4040216dff9
https://hg.mozilla.org/mozilla-central/rev/7295d036a37b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.