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

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll let you fix it :-)

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2: remove aLists parameter

(Here's that patch again, without any accidentally-included crashtest.list tweaks)

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Daniel Holbert [:dholbert]]

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 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 644588 [details] [diff] [review]
Fix AppendToBottom bug [spun off as bug 776306]

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

Good catch!

Comment 13

[Daniel Holbert [:dholbert]]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which means, I guess, that we don't need to call AppendToBottom...

Comment 16

[Mats Palmgren (inactive)]

Good catch finding the leak!

Comment 17

[Daniel Holbert [: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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: 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.)

Comment 19

[Daniel Holbert [:dholbert]]

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)

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e4040216dff9
https://hg.mozilla.org/mozilla-central/rev/7295d036a37b