Closed Bug 413085 Opened 16 years ago Closed 15 years ago

"ASSERTION: SetMayHaveFrame failed" and crash [@ nsCSSFrameConstructor::CreateFloatingLetterFrame] with Arabic, floating first-letter

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: dholbert)

References

Details

(5 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(4 files, 1 obsolete file)

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].
Summary: "ASSERTION: SetMayHaveFrame failed" and crash [@ nsCSSFrameConstructor::CreateFloatingLetterFrame] with Arabic, first-letter → "ASSERTION: SetMayHaveFrame failed" and crash [@ nsCSSFrameConstructor::CreateFloatingLetterFrame] with Arabic, floating first-letter
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.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Crashes on Linux as well.  I'm gonna take a crack at this.

OS --> All
Assignee --> Me
Assignee: nobody → dholbert
OS: Mac OS X → All
Assignee: dholbert → nobody
Component: Layout → Layout: BiDi Hebrew & Arabic
QA Contact: layout → layout.bidi
Attached patch patch v1Splinter Review
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)
Attachment #300501 - Attachment is obsolete: true
Assignee: nobody → dholbert
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: tracking1.9+
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]).
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
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!
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WORKSFORME
Flags: in-testsuite+
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
Group: core-security
Status: RESOLVED → REOPENED
Flags: blocking1.9.0.11?
Resolution: WORKSFORME → ---
Version: Trunk → 1.9.0 Branch
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?
Whiteboard: fix-range-wanted
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → WORKSFORME
Whiteboard: fix-range-wanted → fix-range-wanted, [needs 1.9.0.x fix]
Version: 1.9.0 Branch → Trunk
Flags: blocking1.9.0.11? → blocking1.9.0.12?
Whiteboard: fix-range-wanted, [needs 1.9.0.x fix] → [sg:critical?] fix-range-wanted, [needs 1.9.0.x fix]
Flags: blocking1.9.0.12? → blocking1.9.0.12+
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.
Depends on: 466763
Whiteboard: [sg:critical?] fix-range-wanted, [needs 1.9.0.x fix] → [sg:critical?][needs 1.9.0.x fix]
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).
Whiteboard: [sg:critical?][needs 1.9.0.x fix] → [sg:critical?][needs 1.9.0.x approval on 466763,468491]
(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.
Keywords: fixed1.9.0.12
Whiteboard: [sg:critical?][needs 1.9.0.x approval on 466763,468491] → [sg:critical?]
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.
Keywords: fixed1.9.1
Resolution: WORKSFORME → FIXED
Marking this as verified for 1.9.0.12 based on my current debug build on OS X.
Does not affect 1.8.
Flags: wanted1.8.1.x-
Group: core-security
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsCSSFrameConstructor::CreateFloatingLetterFrame]
You need to log in before you can comment on or make changes to this bug.