Last Comment Bug 413085 - "ASSERTION: SetMayHaveFrame failed" and crash [@ nsCSSFrameConstructor::CreateFloatingLetterFrame] with Arabic, floating first-letter
: "ASSERTION: SetMayHaveFrame failed" and crash [@ nsCSSFrameConstructor::Creat...
Status: VERIFIED FIXED
[sg:critical?]
: assertion, crash, testcase, verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 All
: P3 critical (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
Depends on: 466763
Blocks: stirdom textfuzzer
  Show dependency treegraph
 
Reported: 2008-01-18 21:07 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
15 users (show)
roc: blocking1.9-
dveditz: blocking1.9.0.12+
roc: wanted1.9.0.x+
stransky: wanted1.8.1.x-
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (361 bytes, text/html)
2008-01-18 21:08 PST, Jesse Ruderman
no flags Details
patch v1 (1.55 KB, patch)
2008-01-30 12:59 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
testcase 2 (triggers assertion) (288 bytes, text/html)
2008-01-30 13:06 PST, Daniel Holbert [:dholbert]
no flags Details
patch v2 (get 1st cont with diff parent) (2.94 KB, patch)
2008-01-30 15:10 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch v2a (get 1st cont with diff parent) (2.94 KB, patch)
2008-01-30 15:14 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2008-01-18 21:07:27 PST
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].
Comment 1 Jesse Ruderman 2008-01-18 21:08:26 PST
Created attachment 297918 [details]
testcase (crashes Firefox when loaded)
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2008-01-20 18:44:30 PST
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.
Comment 3 Daniel Holbert [:dholbert] 2008-01-29 13:20:28 PST
Crashes on Linux as well.  I'm gonna take a crack at this.

OS --> All
Assignee --> Me
Comment 4 Daniel Holbert [:dholbert] 2008-01-30 12:59:30 PST
Created attachment 300462 [details] [diff] [review]
patch v1

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.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2008-01-30 13:04:12 PST
Do we actually want to get the first continuation that doesn't have the letter frame as its parent?
Comment 6 Daniel Holbert [:dholbert] 2008-01-30 13:06:47 PST
Created attachment 300465 [details]
testcase 2 (triggers assertion)
Comment 7 Daniel Holbert [:dholbert] 2008-01-30 15:10:14 PST
Created attachment 300501 [details] [diff] [review]
patch v2 (get 1st cont with diff 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.
Comment 8 Daniel Holbert [:dholbert] 2008-01-30 15:14:53 PST
Created attachment 300505 [details] [diff] [review]
patch v2a (get 1st cont with diff parent)

(fixed typo in a comment and in an assert message)
Comment 9 Jesse Ruderman 2008-03-08 14:53:04 PST
I think I can make this bug cause a debug build to dereference 0xddddddf5, but that happens in debug-only code.
Comment 10 Daniel Holbert [:dholbert] 2008-05-05 11:16:50 PDT
(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]).
Comment 11 Daniel Holbert [:dholbert] 2009-02-03 17:37:48 PST
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.
Comment 12 Daniel Holbert [:dholbert] 2009-02-03 17:43:18 PST
(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
Comment 13 Jesse Ruderman 2009-02-03 17:51:14 PST
Those assertions are bug 448615.  WFM, yay!
Comment 14 Bob Clary [:bc:] 2009-04-28 15:53:02 PDT
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
Comment 15 Daniel Holbert [:dholbert] 2009-04-28 15:57:33 PDT
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?
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-28 16:06:00 PDT
Since this is still WFM on trunk, why was the bug reopened?
Comment 17 Bob Clary [:bc:] 2009-04-28 16:24:21 PDT
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.
Comment 18 Daniel Holbert [:dholbert] 2009-04-28 16:45:13 PDT
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.
Comment 19 Daniel Holbert [:dholbert] 2009-06-02 12:30:54 PDT
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...
Comment 20 Daniel Holbert [:dholbert] 2009-06-02 13:29:43 PDT
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.
Comment 21 Daniel Holbert [:dholbert] 2009-06-11 16:07:02 PDT
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).
Comment 22 Daniel Holbert [:dholbert] 2009-06-12 12:33:48 PDT
(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.
Comment 23 Al Billings [:abillings] 2009-06-30 17:02:50 PDT
Is there a testcase that works for bug verification on 1.9.0 for this?
Comment 24 Al Billings [:abillings] 2009-06-30 17:12:39 PDT
To be clear, testcase 1 doesn't crash on either XP or OS X when I try.
Comment 25 Daniel Holbert [:dholbert] 2009-06-30 17:15:05 PDT
This is a debug-only crash. (see comment 10)

It should crash in unpatched 1.9.0 debug builds.
Comment 26 Al Billings [:abillings] 2009-06-30 17:26:02 PDT
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.
Comment 27 Daniel Holbert [:dholbert] 2009-06-30 17:46:07 PDT
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.
Comment 28 Daniel Holbert [:dholbert] 2009-07-01 09:09:40 PDT
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...
Comment 29 Al Billings [:abillings] 2009-07-01 09:27:08 PDT
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).
Comment 30 Daniel Holbert [:dholbert] 2009-07-01 10:28:46 PDT
Changing resolution to FIXED & adding fixed1.9.1 keyword, since comment 20 showed that this was fixed by bug 466763's patch.
Comment 31 Al Billings [:abillings] 2009-07-06 16:01:05 PDT
Marking this as verified for 1.9.0.12 based on my current debug build on OS X.
Comment 32 Martin Stránský 2009-07-08 03:17:49 PDT
Does not affect 1.8.
Comment 33 Aakash Desai [:aakashd] 2009-07-23 14:29:58 PDT
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

Note You need to log in before you can comment on or make changes to this bug.