775823 - TextOverflow::CreateMarkers() leaks if ClipMarker() fails due to OOM

Status: RESOLVED INVALID

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Holbert [:dholbert]]

I'm pretty sure TextOverflow::CreateMarkers() leaks in mozilla-central right now, if we happen to hit OOM at just the right time. (which is relevant because this code has OOM checks)

Currently, this method has 2 clauses like this:

693     nsDisplayItem* marker = new (mBuilder)
694       nsDisplayTextOverflowMarker(mBuilder, mBlock, markerRect,
695                                   aLine->GetAscent(), mLeft.mMarkerString, 0);
696     if (marker) {
697       marker = ClipMarker(mBuilder, mBlock, marker,
698                           mContentArea + mBuilder->ToReferenceFrame(mBlock),
699                           &markerRect);
700     }
701     mMarkerList->AppendNewToTop(marker);

where ClipMarker() returns
  new (aBuilder) nsDisplayClip(..., marker, ...)


So hypothetically, suppose our nsDisplayTextOverflowMarker allocation succeeds -- so marker starts out non-null.  Then we hit this line...
> 697       marker = ClipMarker(mBuilder, mBlock, marker,
...and suppose ClipMarker() fails to allocate its nsDisplayClip.  So it returns null, stomping on our |marker| value.  This will stomp on the only pointer to our "new nsDisplayTextOverflowMarker" that we just allocated, so we'll never be able to free it, and we'll leak. (unless there's some display-list allocation black magic that I'm unaware of that magically manages this somehow...)

(I ran across this when playing around with mats' patch on bug 748646, which touches this code)

Comment 1

[Daniel Holbert [:dholbert]]

Nevermind, it looks like aBuilder uses an arena for allocation, so the I think the "leaked" |marker| memory will get freed when aBuilder is destructed.

Resolving as INVALID.