From 0b513c533150ca69ed906432608e79d3f17956b9 Mon Sep 17 00:00:00 2001 From: Jonathan Kew Date: Tue, 14 Sep 2021 10:10:11 +0000 Subject: [PATCH] Bug 1729578 - Adjust overly-strict assertion in nsContinuingTextFrame::FirstContinuation(). r=emilio It turns out this assertion (added as part of bug 1725555) is not necessarily valid. When we're updating the frame tree in response to DOM modifications (in the testcase here, we're under nsCSSFrameConstructor::ContentRemoved), we may call DeleteFrom on the primary textFrame first, which results in calling SetPrevInFlow(nullptr) on its nextContinuation; this in turn clears the mFirstContinuation back-pointers in all the successors. But if we then call DeleteFrom on one of the continuations later in the chain, those nulled-out pointers trigger this assertion. In fact, it's OK for them to be null in this case, so the assertion is over-zealous and we should drop it. I believe the only time it's OK for this to happen is if the chain of nsContinuingTextFrames has been "detached" from the primary frame during frame-tree updating, as has happened in this example. We can still sanity-check the mFirstContinuation pointer here by asserting that it matches the FirstContinuation() reported by our prev-continuation, or is null if there is no prev-continuation. This is a bit expensive, though, so it's a only a debug-mode assertion (for the fuzzers to try and hit). Differential Revision: https://phabricator.services.mozilla.com/D125444 --- layout/base/crashtests/1729578.html | 26 ++++++++++++++++++++++++++ layout/base/crashtests/crashtests.list | 1 + layout/generic/nsTextFrame.cpp | 7 +++++-- 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 layout/base/crashtests/1729578.html diff --git a/layout/base/crashtests/1729578.html b/layout/base/crashtests/1729578.html new file mode 100644 index 000000000000..27dd8a140fe3 --- /dev/null +++ b/layout/base/crashtests/1729578.html @@ -0,0 +1,26 @@ + + + +
    +
  1. +
+x +
+
;MLCI|=oV;nvAP*o7U
+x diff --git a/layout/base/crashtests/crashtests.list b/layout/base/crashtests/crashtests.list index 4e5e8132ba3f..3b031b4469d3 100644 --- a/layout/base/crashtests/crashtests.list +++ b/layout/base/crashtests/crashtests.list @@ -560,3 +560,4 @@ load 1685146.html load 1689912.html load 1690163.html load 1723200.html +load 1729578.html diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp index f2b7ed65f588..100cdd432244 100644 --- a/layout/generic/nsTextFrame.cpp +++ b/layout/generic/nsTextFrame.cpp @@ -4493,8 +4493,11 @@ class nsContinuingTextFrame final : public nsTextFrame { nsIFrame* FirstInFlow() const final; nsTextFrame* FirstContinuation() const final { - MOZ_DIAGNOSTIC_ASSERT(mFirstContinuation || !mPrevContinuation, - "mFirstContinuation unexpectedly null!"); + // If we have a prev-continuation pointer, then our first-continuation + // must be the same as that frame's. + MOZ_ASSERT((!mPrevContinuation && !mFirstContinuation) || + (mPrevContinuation && + mPrevContinuation->FirstContinuation() == mFirstContinuation)); return mFirstContinuation; };