Last Comment Bug 310267 - crash [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]
: crash [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.8, verified1.8.1
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: 299065
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2005-09-27 21:48 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
7 users (show)
asa: blocking1.8b5-
darin.moz: blocking1.8.1+
dveditz: blocking1.8.0.8+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reduced testcase (603 bytes, text/xml)
2005-09-27 21:50 PDT, Jesse Ruderman
no flags Details
apple crash report with stack trace (22.35 KB, text/plain)
2005-09-27 22:04 PDT, Jesse Ruderman
no flags Details
alternative patch #1: extend the condition in nsFrameManager::RemoveFrame (1.54 KB, patch)
2005-10-24 03:55 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
alternative patch #2: make the continuation of a nextBidi a nextBidi (1.20 KB, patch)
2005-10-24 03:56 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
alternative patch #3: look for nextBidis when inserting frames (1.67 KB, patch)
2005-10-26 08:04 PDT, Simon Montagu :smontagu
uriber: review+
roc: superreview+
roc: approval‑branch‑1.8.1+
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review
Patch for 1.7 (1.04 KB, patch)
2006-11-12 13:05 PST, Mike Hommey [:glandium]
uriber: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2005-09-27 21:48:59 PDT
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.)
Comment 1 Jesse Ruderman 2005-09-27 21:50:30 PDT
Created attachment 197660 [details]
reduced testcase

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050926
Firefox/1.6a1

Crashes within a second of loading the testcase.
Comment 2 Jesse Ruderman 2005-09-27 22:04:55 PDT
Created attachment 197662 [details]
apple crash report with stack trace
Comment 3 Asa Dotzler [:asa] 2005-09-29 18:35:06 PDT
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.
Comment 4 Simon Montagu :smontagu 2005-10-22 12:25:58 PDT
TB10975241E
Comment 5 Simon Montagu :smontagu 2005-10-23 01:09:35 PDT
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
Comment 6 Boris Zbarsky [:bz] 2005-10-23 14:30:29 PDT
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?
Comment 7 Uri Bernstein (Google) 2005-10-23 15:06:52 PDT
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).
Comment 8 Simon Montagu :smontagu 2005-10-24 03:49:20 PDT
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?
Comment 9 Simon Montagu :smontagu 2005-10-24 03:55:00 PDT
Created attachment 200601 [details] [diff] [review]
alternative patch #1: extend the condition in nsFrameManager::RemoveFrame
Comment 10 Simon Montagu :smontagu 2005-10-24 03:56:38 PDT
Created attachment 200602 [details] [diff] [review]
alternative patch #2: make the continuation of a nextBidi a nextBidi
Comment 11 Simon Montagu :smontagu 2005-10-24 04:02:37 PDT
This could do with testing with variations on the original them, but a local copy of the testcase doesn't crash for me.
Comment 12 Boris Zbarsky [:bz] 2005-10-24 05:44:00 PDT
So why does this actually break as things stand?  Is the nsContinuingTextFrame for the nextBidi on a line before the nextBidi itself?
Comment 13 Simon Montagu :smontagu 2005-10-24 06:49:49 PDT
No, it's 2 lines after it. Changing http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#5774 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.
Comment 14 Boris Zbarsky [:bz] 2005-10-24 07:31:50 PDT
Hmm.  Do we have any documentation around that actually describes the frame structures bidi creates?
Comment 15 Simon Montagu :smontagu 2005-10-25 12:54:37 PDT
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.
Comment 16 Boris Zbarsky [:bz] 2005-10-25 14:35:12 PDT
I think the closest we have to that is http://wiki.mozilla.org/Gecko:Continuation_Model
Comment 17 Simon Montagu :smontagu 2005-10-25 14:59:36 PDT
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.
Comment 18 Boris Zbarsky [:bz] 2005-10-25 15:01:31 PDT
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?
Comment 19 Uri Bernstein (Google) 2005-10-25 15:20:43 PDT
(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).
Comment 20 Simon Montagu :smontagu 2005-10-25 15:22:02 PDT
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.
Comment 21 Simon Montagu :smontagu 2005-10-26 07:42:35 PDT
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.
Comment 22 Simon Montagu :smontagu 2005-10-26 08:04:59 PDT
Created attachment 200876 [details] [diff] [review]
alternative patch #3: look for nextBidis when inserting frames

Maybe this should be rolled into GetLastInFlow() instead?
Comment 23 Boris Zbarsky [:bz] 2005-10-26 11:44:19 PDT
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...
Comment 24 Simon Montagu :smontagu 2005-10-27 07:59:43 PDT
I'm also unsure whether GetNextInFlow() should return a nextBidi if there is a nextBidi and no nextInFlow.
Comment 25 Boris Zbarsky [:bz] 2005-10-27 08:02:59 PDT
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.
Comment 26 Simon Montagu :smontagu 2005-10-27 08:25:36 PDT
ipso facto I'm clearly the wrong person to do that. Uri, are you interested?
Comment 27 Uri Bernstein (Google) 2005-10-27 08:55:04 PDT
(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.
Comment 28 Daniel Veditz [:dveditz] 2005-12-15 12:17:54 PST
Could not reproduce the crash in 1.0.7 fwiw
Comment 29 Uri Bernstein (Google) 2006-01-03 16:10:17 PST
"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).
Comment 30 Uri Bernstein (Google) 2006-01-28 13:44:01 PST
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.
Comment 31 Uri Bernstein (Google) 2006-02-22 01:13:45 PST
This is fixed by the fix for bug 299065.
Comment 32 Uri Bernstein (Google) 2006-04-09 08:55:19 PDT
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 mozilla.org.il 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.
Comment 33 Uri Bernstein (Google) 2006-06-17 00:48:54 PDT
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.
Comment 34 Simon Montagu :smontagu 2006-06-19 00:32:21 PDT
Checked in to MOZILLA_1_8_BRANCH
Comment 35 Bob Clary [:bc:] 2006-08-21 20:55:59 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=197660
ff2b2 winxp, linux no crash

verified fixed 1.8
Comment 36 Daniel Veditz [:dveditz] 2006-09-26 14:54:18 PDT
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
Comment 37 Simon Montagu :smontagu 2006-10-03 03:34:10 PDT
Checked in to MOZILLA_1_8_0_BRANCH
Comment 38 Jay Patel [:jay] 2006-10-20 14:35:18 PDT
v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.8pre) Gecko/20061020 Firefox/1.5.0.8pre, no crash with reduced testcase.
Comment 39 Mike Hommey [:glandium] 2006-11-12 13:05:23 PST
Created attachment 245404 [details] [diff] [review]
Patch for 1.7

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...
Comment 40 Uri Bernstein (Google) 2006-11-13 09:23:40 PST
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.
Comment 41 Jesse Ruderman 2007-12-15 20:28:37 PST
Crashtest checked in.

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