Closed Bug 310267 Opened 19 years ago Closed 19 years ago

crash [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]


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

Not set





(Reporter: jruderman, Assigned: smontagu)



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

Crash Data


(6 files)

Filing as security-sensitive because I believe the crash is exploitable. 
(Before I reduced the testcase, I got similar stack traces with large non-zero
numbers on top instead of zero on top.)
Attached file reduced testcase
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050926

Crashes within a second of loading the testcase.
Flags: blocking1.8b5?
Whiteboard: [sg:fix]
Flags: blocking1.8b5? → blocking1.8b5+
If you come up with a very safe fix in the next couple of days, please request
approval for the patch and we'll evaluate.
Flags: blocking1.8b5+ → blocking1.8b5-
OS: MacOS X → All
Hardware: Macintosh → All
In a debug build I get:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file c:/ho
me/simon/projects/mozwork/debugtree/mozilla/layout/generic/nsBlockFrame.cpp, lin
e 5681

followed by: 

First-chance exception in seamonkey.exe (GKLAYOUT.DLL): 0xC0000005: Access Violation.

with this at the top of the stack:
nsCachedStyleData::GetStyleData(const nsStyleStructID & eStyleStruct_Display) line 210 + 3 bytes
nsStyleContext::GetStyleData(nsStyleStructID eStyleStruct_Display) line 248 + 15 bytes
nsIFrame::GetStyleData(nsStyleStructID eStyleStruct_Display) line 612
nsIFrame::GetStyleDisplay() line 90 + 17 bytes
nsBidiPresUtils::InitLogicalArray(nsPresContext * 0x0490e170, nsIFrame * 0x04994a10, nsIFrame * 0x00000000, int 0x00000001) line 375 + 8 bytes
nsBidiPresUtils::Resolve(nsPresContext * 0x0490e170, nsIFrame * 0x049923ac, nsIFrame * 0x04994a10, int & 0x00000000, int 0x00000000) line 197 + 23 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x049923ac, nsPresContext * 0x0490e170, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 794 + 66 bytes

and at nsIFrame::GetStyleDisplay(), the frame's members are all 0xdddddddd
Summary: crash [@ nsBidiPresUtils::Resolve] → crash [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]
That first assert is key, I think.  We're hitting it when we're removing an nsTextFrame.  The text frame has a nextBidi, so we remove that first (in nsFrameManager::RemoveFrame).  We hit the assert while removing said nextBidi.  I have no idea what that code is really doing, so Simon or Uri, could you check out what's happening there?
As much as I'd like to help, I don't really know this code. But I'll try to have a look tomorrow anyway (unless Simon figures it out by then).
Thanks for the analysis, Boris. The situation is actually a little more complicated: the original frame acquires a nextBidi in Bidi resolution, and then the nextBidi acquires an nsContinuingTextFrame in ReflowLine (probably because of the  newline character in the text). This third frame is the one we get the assert on and which crashes later.

I think the way to fix this is to make nsTextManager::RemoveFrame treat an nsContinuingTextFrame of a nextBidi the same way it treats a nextBidi, and I can see two ways to achieve this. Patches to follow.

Do we still think this is exploitable?
Assignee: mozilla → smontagu
This could do with testing with variations on the original them, but a local copy of the testcase doesn't crash for me.
So why does this actually break as things stand?  Is the nsContinuingTextFrame for the nextBidi on a line before the nextBidi itself?
No, it's 2 lines after it. Changing to loop until |line->Contains(deletedNextInFlow)| is true prevents the assertion, but it still crashes (in a different place) because the next time through the loop aDeletedFrame is not the nextSibling of prevSibling.
Hmm.  Do we have any documentation around that actually describes the frame structures bidi creates?
Not really, no. I'm willing to write some, especially if you can point me to some documentation about the frame structures that regular reflow creates that I can use as a starting point.
I think the closest we have to that is
OK, suppose I add a bullet to that page after "nsBlockFrame creates multiple inline children...", something like:

When an inline frame contains bidirectional text, nsBlockFrame also creates multiple inline frames associated with the same inline element, so that each frame contains a run of text with the same directionality. In this case, all of the frames associated with the inline element will also have the status bit NS_FRAME_IS_BIDI set, and the frames will be linked via the nextBidi property.
OK... And how does that interact with the flow?  That is, it appears that with bidi an immediate next-in-flow may be two lines down from where we are.  That can't happen without bidi, right?  So how does that happen?
(In reply to comment #17)
> When an inline frame contains bidirectional text, nsBlockFrame also creates
> multiple inline frames associated with the same inline element,

Shouldn't that more accurately say "text frames", rather than "inline frames"? AFAIK only text frames are currently handled this way (and bug 299065 is about doing this for inline frames as well).
I will need to do more debugging to answer comment 18, i.e. it won't happen before tomorrow morning. In principle Bidi should not have any effect on the line breaking.
It seems that the root of the bug is not the deletion but the insertion. In the non-bidi case the <div> is being inserted after the nextInFlow of a text frame, but in the bidi case (where the text frame has a nextBidi) it is being inserted after the nextBidi and before the nextInFlow, forcing the nextInFlow down by one line.
Maybe this should be rolled into GetLastInFlow() instead?
So before the insertion happens, what does the frame model look like?  We have various places that walk the in-flow chain, get last-in-flows, etc, and it sounds to me like a lot of them need to become bidi-aware either implicitly (change GetLastInFlow()) or explicitly.  Just changing GetLastInFlow() is not so great, since we probably have code that depends on this being the same result as just walking the flow chain...
I'm also unsure whether GetNextInFlow() should return a nextBidi if there is a nextBidi and no nextInFlow.
Which brings me back to my question.  Could we please, please document what sort of frame structures bidi creates?  Right now I can't usefully think about this problem, because I don't know what I'm working with.
ipso facto I'm clearly the wrong person to do that. Uri, are you interested?
(In reply to comment #26)
> ipso facto I'm clearly the wrong person to do that. Uri, are you interested?

I was actually looking forward to *reading* this documentation when it's ready, so that I'll finally understand what's going on...

I can perhaps try reading the code and writing what I can make of it (asking you lots of questions in the process). Still, I would think that it would be better for someone who already knows the system to be the one documenting it. I'm not sure what factum you're basing your assertion upon.
Assignee: smontagu → dbaron
Could not reproduce the crash in 1.0.7 fwiw
Whiteboard: [sg:fix] → [sg:critical?]
"alternative patch #1" will be effectively incorporated into my patch for bug 299065, if and when that patch would reach a complete and stable state.

Getting the "nextBidi" property is replaced by a call to aOldFrame->GetNextContinuation() (which returns both bidi continuations and NiFs).
Depends on: 299065
Actually, the latest version of my patch for bug 299065 also effectively implements "alternative solution #3" (by using GetLastContinuation rather than GetLastInFlow).

...which leavs me confused as to what nsFrameManager::RemoveFrame() and nsFrameManager::InsertFrames() are really expected to do with next-in-flows and bidi continuations.
This is fixed by the fix for bug 299065.
Closed: 19 years ago
Resolution: --- → FIXED
I think we should consider taking one of these patches for the 1.8 branch.

In addition to fixing this crash, there was a report on the forum about RTL text being inverted in several situations (specifically, when using "highlight all" in the search bar in Firefox, where the searched text appears in a bidi wrapping line). These problems are fixed by the patch for bug 299065, but I also verified they are fixed by alternative patch #3 (I didn't check the others), so that might be another reason to take it for 1.8.
Flags: blocking1.8.1?
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 200876 [details] [diff] [review]
alternative patch #3: look for nextBidis when inserting frames

I'm taking the liberty of r+'ing Simon's patch #3 (for the branch), and requesting sr and approval form ROC.
Attachment #200876 - Flags: superreview?(roc)
Attachment #200876 - Flags: review+
Attachment #200876 - Flags: approval-branch-1.8.1?(roc)
Attachment #200876 - Flags: superreview?(roc)
Attachment #200876 - Flags: superreview+
Attachment #200876 - Flags: approval-branch-1.8.1?(roc)
Attachment #200876 - Flags: approval-branch-1.8.1+
Checked in to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
ff2b2 winxp, linux no crash

verified fixed 1.8
Flags: blocking1.8.0.8+
Comment on attachment 200876 [details] [diff] [review]
alternative patch #3: look for nextBidis when inserting frames

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #200876 - Flags: approval1.8.0.8+
Assignee: dbaron → smontagu
Checked in to MOZILLA_1_8_0_BRANCH
Keywords: fixed1.8.0.8
v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/20061020 Firefox/, no crash with reduced testcase.
Group: security
Attached patch Patch for 1.7Splinter Review
Here is an adapted patch for the 1.7 branch. Could you confirm that it is okay ? I'm unable to reproduce the crash on debian with both 1.7 and 1.8 branch with the pango or the xft backend, so I'm kind of blind here...
Attachment #245404 - Flags: review?
Attachment #245404 - Flags: review? → review?(uriber)
Comment on attachment 245404 [details] [diff] [review]
Patch for 1.7

Rubber-stamping, since this is essentially the same as the 1.8 patch I reviewed. I don't have the means to test this in 1.7.
Attachment #245404 - Flags: review?(uriber) → review+
Crashtest checked in.
Flags: in-testsuite+
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
Crash Signature: [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]
You need to log in before you can comment on or make changes to this bug.