Loading the testcase triggers: ###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 409 followed by a null deref [@ nsCSSFrameConstructor::CreateFloatingLetterFrame].

We end up with a floating first letter frame whose child textframe's content is not actually in the tree. I guess we don't remove the textframe properly during the removeChild. That's bad.... very bad.

Crashes on Linux as well. I'm gonna take a crack at this. OS --> All Assignee --> Me

Patch v1 fixes the crash & assertion -- not sure it's exactly correct, though. Basically, the main issue is that in nsCSSFrameConstructor::RemoveFloatingFirstLetterFrames, in the section labelled "Destroy the old text frame's continuations", we were getting its NextInFlow, which does NOT include Bidi continuations.[1] So Bidi continuations weren't getting destroyed. Also, I don't think the "nsSplittableFrame::BreakFromPrevFlow" method is doing what we'd like it to do here -- more on that after I post another testcase to demonstrate what's going wrong with that. [1] See http://mxr.mozilla.org/seamonkey/source/layout/generic/nsIFrame.h#158 for definition of "Fluid" vs "Hard" continuations. GetNextInFlow only returns Fluid (i.e. non-Bidi) continuations.

Do we actually want to get the first continuation that doesn't have the letter frame as its parent?

(In reply to comment #5) > Do we actually want to get the first continuation that doesn't have the letter > frame as its parent? So, this concern assumes that we could run into a situation with a floated letter frame that contains multiple textFrame kids (which are continuations of each other). AFAIK, that situation couldn't possibly happen right now, because: A) The letter frame would only split its textual content into multiple kids in cases where it has multiple characters (in particular, both RTL and LTR characters) B) The letter frame could only have multiple characters if one of them is an initial punctuation mark C) Bug 393985 (specifically the 1st comment, not the 0th one) indicates that first-letter style isn't applied to the RTL character in cases with an initial punctuation mark. So, the letter frame can't currently have both RTL & LTR characters, AFAIK, so it shouldn't get split. Also, note that Bug 393985 isn't necessarily a bug -- smontagu said in IRC that the spec is vague in that situation, so we may keep that behavior as-is. However, for safety's sake, maybe we want to handle this situation anyway, in case Bug 393985 is fixed. This patch handles this situation by doing what BZ suggested -- it finds the first continuation with a *different* parent.

(fixed typo in a comment and in an assert message)

I think I can make this bug cause a debug build to dereference 0xddddddf5, but that happens in debug-only code.

(In reply to comment #9) > I think I can make this bug cause a debug build to dereference 0xddddddf5, but > that happens in debug-only code. > Yeah -- currently, I can only reproduce the crash in a debug build. It derefs null for me, using the attached testcase (attachment 297918 [details]).

This has become WFM on mozilla-central -- no crash or SetMayHaveFrame assertion on the first testcase, and no SetMayHaveFrame assertion on the second testcase. However, it's still broken on the 1.9.1 branch.

(In reply to comment #11) > This has become WFM on mozilla-central -- no crash or SetMayHaveFrame assertion Mozilla-central gives me 3 copies of this assertion-pair when loading testcase 1, though: ###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file nsTextFrameThebes.cpp, line 619 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file nsLineBreaker.cpp, line 51

Those assertions are bug 448615. WFM, yay!

1.9.0 !exploitable report. I didn't see a crash on 1.9.1 winxp, not sure about other platforms. PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsCSSFrameConstructor::CreateFloatingLetterFrame

Can we track down a fix range in mozilla-central, so we know which patch to backport as a fix for this on 1.9.0?

Since this is still WFM on trunk, why was the bug reopened?

I reopened it and targeted the 1.9.0 version rather than filing a new bug since it was a WFM rather than a FIXED on trunk. If it had been FIXED on trunk I would have just added a private comment. If that was wrong, sorry. Please tell me and I will instead file a new bug.

No need to reopen this or open a new bug -- adding "[needs 1.9.0.x fix]" to the whiteboard (and ticking the blocking? flag, which you did) is good, I think.

I think this may have been fixed by bug 466763, from looking at code changes to nsCSSFrameConstructor in the same region as are touched by this bug's posted (but un-landed) patch. (The testcase on that bug looks similar to this bug's testcase, too.) I'm doing targeted checkouts on either side of that bug's 1.9.1 landing to determine if this is correct...

Yeah -- apparently I was mistaken in comment 11 ("it's still broken on the 1.9.1 branch" in feb 2009). It looks like this actually became fixed on 1.9.1 when bug 466763 landed, on Jan 1st 2009. The parent revision (cda265c626be) crashes on this bug's testcase, whereas the revision for bug 466763 (073269e99a2a) doesn't crash.

To fix this on 1.9.0.x, I've posted a backported patch on bug 466763, with an approval request. That patch causes one known regression -- bug 468491 -- so I've posted a backported version of that bug's fix, as well, with an approval request. I've confirmed that these backported patches fix their respective testcases' crashes on the 1.9.0.x branch (and that bug 466763's patch also fixes the crash on this bug here).

(In reply to comment #21) > To fix this on 1.9.0.x, I've posted a backported patch on bug 466763, with an > approval request. Patch landed. --> Adding fixed1.9.0.12 keyword.

Is there a testcase that works for bug verification on 1.9.0 for this?

To be clear, testcase 1 doesn't crash on either XP or OS X when I try.

This is a debug-only crash. (see comment 10) It should crash in unpatched 1.9.0 debug builds.

Using my debug OS X build from April 9, I cannot get this to crash with either testcase. I do see the assertions with testcase 2 in my debug build from 4/9. In the current build, I get the following assertions when I open testcase 2: ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/abillings/mozilla/content/base/src/nsLineBreaker.cpp, line 51 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/abillings/mozilla/content/base/src/nsLineBreaker.cpp, line 51 ###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/abillings/mozilla/content/base/src/nsLineBreaker.cpp, line 51 but I don't see the original assertions anymore. So, we could call that fixed but I don't know why I don't crash on OS X with the 4/9 build. I don't have access to Windows or Linux debug builds to test this.

IIRC, I never had issues reproducing this in unpatched 1.9.0 debug builds on my Linux boxes... I've fired off a 1.9.0 build from April 9 2009 at 4am PDT, as a sanity check.

Yeah, I just crashed immediately when loading testcase 1, with a build freshly checked out from CVS with MOZ_CO_DATE="9 Apr 2009 19:00 PDT". (on Linux) Apparently Jesse could reproduce this on Mac when he originally filed it, too... though maybe we inadvertantly worked around it on Mac on 1.9.0 somehow? Seems unlikely...

I suspect my Mac debug build is messed up somehow. I moved it and symbols so I could compile a more recent build and I may not have moved everything necessary (not being a debug expert).

Changing resolution to FIXED & adding fixed1.9.1 keyword, since comment 20 showed that this was fixed by bug 466763's patch.

Marking this as verified for 1.9.0.12 based on my current debug build on OS X.

Does not affect 1.8.

verified FIXED using testcases (no rcash and no assertion) on debug builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2pre) Gecko/20090722 Shiretoko/3.5.2pre ID:20090722035056 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090723 Minefield/3.6a1pre ID:20090723134652