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

VERIFIED FIXED

Status

()

Core
Layout: Text
P3
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

(Blocks: 2 bugs, 5 keywords)

Trunk
x86
All
assertion, crash, testcase, verified1.9.0.12, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
blocking1.9.0.12 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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].
(Reporter)

Comment 1

10 years ago
Created attachment 297918 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Updated

10 years ago
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
(Assignee)

Comment 3

10 years ago
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)

Updated

10 years ago
Assignee: dholbert → nobody
Component: Layout → Layout: BiDi Hebrew & Arabic
QA Contact: layout → layout.bidi
Priority: P2 → P3
(Assignee)

Comment 4

10 years ago
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.
Do we actually want to get the first continuation that doesn't have the letter frame as its parent?
(Assignee)

Comment 6

10 years ago
Created attachment 300465 [details]
testcase 2 (triggers assertion)
(Assignee)

Comment 7

10 years ago
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.
(Assignee)

Comment 8

10 years ago
Created attachment 300505 [details] [diff] [review]
patch v2a (get 1st cont with diff parent)

(fixed typo in a comment and in an assert message)
Attachment #300501 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Assignee: nobody → dholbert
Flags: wanted1.9.0.x+
Flags: blocking1.9-

Updated

9 years ago
Flags: tracking1.9+
(Reporter)

Comment 9

9 years ago
I think I can make this bug cause a debug build to dereference 0xddddddf5, but that happens in debug-only code.
(Assignee)

Comment 10

9 years ago
(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]).

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
(Assignee)

Comment 11

8 years ago
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.
(Assignee)

Comment 12

8 years ago
(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
(Reporter)

Comment 13

8 years ago
Those assertions are bug 448615.  WFM, yay!
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
(Reporter)

Updated

8 years ago
Flags: in-testsuite+

Comment 14

8 years ago
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
(Assignee)

Comment 15

8 years ago
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?

Comment 17

8 years ago
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.
(Assignee)

Comment 18

8 years ago
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
Last Resolved: 8 years ago8 years ago
Resolution: --- → WORKSFORME
Whiteboard: fix-range-wanted → fix-range-wanted, [needs 1.9.0.x fix]
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 19

8 years ago
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...
(Assignee)

Comment 20

8 years ago
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
(Assignee)

Updated

8 years ago
Whiteboard: [sg:critical?] fix-range-wanted, [needs 1.9.0.x fix] → [sg:critical?][needs 1.9.0.x fix]
(Assignee)

Comment 21

8 years ago
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).
(Assignee)

Updated

8 years ago
Whiteboard: [sg:critical?][needs 1.9.0.x fix] → [sg:critical?][needs 1.9.0.x approval on 466763,468491]
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Comment 25

8 years ago
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.
(Assignee)

Comment 27

8 years ago
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.
(Assignee)

Comment 28

8 years ago
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).
(Assignee)

Comment 30

8 years ago
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.
Keywords: fixed1.9.0.12 → verified1.9.0.12

Comment 32

8 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ nsCSSFrameConstructor::CreateFloatingLetterFrame]
You need to log in before you can comment on or make changes to this bug.