From feb62581459311e79f67a0534b4d0a71834a728f Mon Sep 17 00:00:00 2001 From: leviw Date: Wed, 20 Jan 2016 18:23:55 -0800 Subject: Delete BidiRuns when handling empty lines There is a fast-path for empty lines that failed to clear the BidiRun list in the BidiResolver, resulting in a leak. This test plugs the leak and adds asserts that resolver runs are cleaned up. BUG=527166 Review URL: https://codereview.chromium.org/1602363003 Cr-Commit-Position: refs/heads/master@{#370577} --- .../fast/inline/empty-line-leaking-bidiruns-crash-expected.txt | 3 +++ .../LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash.html | 7 +++++++ third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp | 7 +++++++ third_party/WebKit/Source/platform/text/BidiResolver.h | 1 + 4 files changed, 18 insertions(+) create mode 100644 third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash-expected.txt create mode 100644 third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash.html diff --git a/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash-expected.txt b/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash-expected.txt new file mode 100644 index 0000000..d48051a --- /dev/null +++ b/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash-expected.txt @@ -0,0 +1,3 @@ +Test passes if no assert. + + diff --git a/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash.html b/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash.html new file mode 100644 index 0000000..60b2fb1 --- /dev/null +++ b/third_party/WebKit/LayoutTests/fast/inline/empty-line-leaking-bidiruns-crash.html @@ -0,0 +1,7 @@ + +
Test passes if no assert.
+
+ diff --git a/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp b/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp index 973526c..05b71ef 100644 --- a/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp +++ b/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp @@ -839,6 +839,9 @@ void LayoutBlockFlow::layoutRunsAndFloatsInRange(LineLayoutState& layoutState, while (!endOfLine.atEnd()) { bool logicalWidthIsAvailable = false; + // The runs from the previous line should have been cleaned up. + ASSERT(!resolver.runs().runCount()); + // FIXME: Is this check necessary before the first iteration or can it be moved to the end? if (layoutState.endLine()) { layoutState.setEndLineMatched(layoutState.endLineMatched() || matchedEndLine(layoutState, resolver, cleanLineStart, cleanLineBidiStatus)); @@ -876,6 +879,7 @@ void LayoutBlockFlow::layoutRunsAndFloatsInRange(LineLayoutState& layoutState, if (layoutState.lineInfo().isEmpty()) { if (lastRootBox()) lastRootBox()->setLineBreakInfo(endOfLine.object(), endOfLine.offset(), resolver.status()); + resolver.runs().deleteRuns(); } else { VisualDirectionOverride override = (styleToUse.rtlOrdering() == VisualOrder ? (styleToUse.direction() == LTR ? VisualLeftToRightOverride : VisualRightToLeftOverride) : NoVisualOverride); if (isNewUBAParagraph && styleToUse.unicodeBidi() == Plaintext && !resolver.context()->parent()) { @@ -949,6 +953,9 @@ void LayoutBlockFlow::layoutRunsAndFloatsInRange(LineLayoutState& layoutState, resolver.setPosition(endOfLine, numberOfIsolateAncestors(endOfLine)); } + // The resolver runs should have been cleared, otherwise they're leaking. + ASSERT(!resolver.runs().runCount()); + // In case we already adjusted the line positions during this layout to avoid widows // then we need to ignore the possibility of having a new widows situation. // Otherwise, we risk leaving empty containers which is against the block fragmentation principles. diff --git a/third_party/WebKit/Source/platform/text/BidiResolver.h b/third_party/WebKit/Source/platform/text/BidiResolver.h index db4c901..9e0b8bc 100644 --- a/third_party/WebKit/Source/platform/text/BidiResolver.h +++ b/third_party/WebKit/Source/platform/text/BidiResolver.h @@ -350,6 +350,7 @@ BidiResolver::~BidiResolver() { // The owner of this resolver should have handled the isolated runs. ASSERT(m_isolatedRuns.isEmpty()); + ASSERT(!m_runs.runCount()); } #endif -- cgit v1.1