Last Comment Bug 748646 - Put all the block's text-overflow markers at the bottom of PositionedDescendants()
: Put all the block's text-overflow markers at the bottom of PositionedDescenda...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla17
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-24 20:20 PDT by Mats Palmgren (:mats)
Modified: 2012-07-24 03:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: put text-overflow markers at bottom of PositionedDescendants() (6.42 KB, patch)
2012-07-11 03:10 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
patch 2: remove aLists parameter (13.45 KB, patch)
2012-07-19 21:15 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 2 v2: remove aLists parameter (4.70 KB, patch)
2012-07-19 21:19 PDT, Daniel Holbert [:dholbert]
mats: review+
Details | Diff | Splinter Review
Fix AppendToBottom bug [spun off as bug 776306] (968 bytes, patch)
2012-07-20 21:35 PDT, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
patch 1 v2 (8.89 KB, patch)
2012-07-22 15:54 PDT, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2012-04-24 20:20:20 PDT
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) (Exited; email my personal email if necessary) 2012-04-24 23:44:45 PDT
I'll let you fix it :-)
Comment 2 Daniel Holbert [:dholbert] 2012-07-10 16:54:52 PDT
*** Bug 772690 has been marked as a duplicate of this bug. ***
Comment 3 Daniel Holbert [:dholbert] 2012-07-10 17:07:56 PDT
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 (:mats) 2012-07-11 03:10:46 PDT
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.
Comment 5 Daniel Holbert [:dholbert] 2012-07-19 21:09:33 PDT
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] 2012-07-19 21:15:14 PDT
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.
Comment 7 Daniel Holbert [:dholbert] 2012-07-19 21:19:42 PDT
Created attachment 644168 [details] [diff] [review]
patch 2 v2: remove aLists parameter

(Here's that patch again, without any accidentally-included crashtest.list tweaks)
Comment 8 Daniel Holbert [:dholbert] 2012-07-20 21:19:30 PDT
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] 2012-07-20 21:35:08 PDT
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.
Comment 10 Daniel Holbert [:dholbert] 2012-07-20 21:37:11 PDT
(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] 2012-07-20 21:45:27 PDT
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) (Exited; email my personal email if necessary) 2012-07-21 07:24:57 PDT
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] 2012-07-21 07:46:06 PDT
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) (Exited; email my personal email if necessary) 2012-07-21 08:18:58 PDT
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) (Exited; email my personal email if necessary) 2012-07-21 08:19:36 PDT
Which means, I guess, that we don't need to call AppendToBottom...
Comment 16 Mats Palmgren (:mats) 2012-07-21 16:13:18 PDT
Good catch finding the leak!
Comment 17 Daniel Holbert [:dholbert] 2012-07-21 19:00:47 PDT
(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] 2012-07-22 15:54:49 PDT
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.)
Comment 19 Daniel Holbert [:dholbert] 2012-07-22 23:12:21 PDT
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)

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