948181 - "ASSERTION: embedding/override underflow" with bidi, :first-line

Status: RESOLVED DUPLICATE of bug 945105

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

mEmbeddingStack.Length() is zero, which has two symptoms:

> NS_ASSERTION(mEmbeddingStack.Length(), "embedding/override underflow");

###!!! ASSERTION: embedding/override underflow: 'mEmbeddingStack.Length()', file ../../../layout/base/nsBidiPresUtils.cpp, line 274

> mEmbeddingStack.TruncateLength(mEmbeddingStack.Length() - 1);

###!!! ABORT: caller should use SetLength instead: 'newLen <= oldLen', file ../../dist/include/nsTArray.h

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Mats Palmgren (inactive)]

I haven't looked at the code, but marking security-sensitive just in case...

Comment 3

[Simon Montagu :smontagu]

Attachment: frame tree

During bidi resolution after removing the dir attribute, the condition at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp#987 is false for the <span id="x"> (frame 0x7f8393e474c8), but it is true for the continuation of that span (frame 0x7f8393e48210). 

This continuation has a prev-continuation but no next-continuation, so we don't do PushBidiControl at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp#996, but we do do PopBidiControl at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp#1187, and so we assert.

Regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d279364a8b&tochange=e85b0372cece. dbaron, bug 828312 and/or bug 898333 in that range look relevant, but I'm not sure whether one of them has a bug, or whether they exposed a bug in bidi resolution.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this was a restyling bug, and should have been fixed by the patches in bug 945105.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, this works for me, and it's basically the same testcase as bug 945105, except Jesse found a way to juice it up and make it trigger bad assertions.

I wonder if this bidi code should be a little more robust, though?  Would it have done anything bad in non-debug builds, or only in debug builds?

Comment 6

[Andrew McCreight [:mccr8]]

Simon, can we close this as works-for-me, or should we morph this into some kind of sec-audit bug about dbaron's ideas in comment 5?  Thanks.