This is a followup on bug 319705 (see bug 319705 comment #9, 2nd paragraph). If, using script, you create a DOM text node consisting of a single CR (0x0d), moving the caret backwords over this text crashes the browser (at least on OS X. Since this is a case of uninitialized memory used as an array subscript, other OSs might not crash). I'll attach a testcase immediately. Here's my analysis of the problem (based bug 319705 comment #6 and #7): What happens is that PrepareUnicodeText() in this case leaves indexBuffer.mBuffer uninitialized (the single character in the content is not mapped to anything). Later, when nsTextFrame::PeekOffset() examines ip[i] (which is really indexBuffer.mBuffer[0]), it finds junk there, and from there on all kinds of things can happen. The reason PrepareUnicodeText() does not init indexBuffer.mBuffer[0] is that the call to aTX.GetNextWord() returns nsnull, which then causes an immediate break out of the loop. I can't say whether GetNextWord() really should return nsnull in this case (and then it should just be handled better by PrepareUnicodeText()), or whether the problem is in GetNextWord() itself.

Load the testcase in the browser, then: - Turn on caret browsing mode (F7) - Place the caret in the word "World" - Use left-arrow to move into the word "Hello" You should crash as you try to move over the (invisible) CR between the two words. The crash is in the following line: if ((ip[i] < ip[aPos->mStartOffset - mContentOffset]) && (clusterBuffer.mBuffer[ip[i] - mContentOffset]) && (! IS_LOW_SURROGATE(paintBuffer.mBuffer[ip[i]-mContentOffset]))) i=0. ip[0] is junk, so what actually crashes is probably |clusterBuffer.mBuffer[ip[i] - mContentOffset]|

All/All because the underlying problem is clearly cross-platform (regardless whether other platforms actually crash).

Is this the sort of thing that could cause jumps into random memory? Or just reads of random memory?

(In reply to comment #3) > Is this the sort of thing that could cause jumps into random memory? Or just > reads of random memory? It seems to me that this can not cause random-memory jumps, only reads. BTW, I had a chance to test the attached testcase on Windows, and it did not crash (not that this matters much).

So which case do we end up hitting in PrepareUnicodeText? Do we get isWhitespace equal to true? Or no?

(In reply to comment #5) > So which case do we end up hitting in PrepareUnicodeText? Do we get > isWhitespace equal to true? Or no? No, isWhitespace is false - but we never get to the point where we ask about it. We get into |if (nsnull == bp)| (line 2118), and thus break out of the loop (line 2227). From there on, nothing sets aIndexBuffer.mBuffer[0]. Note that empirically this appears to be a regression: I'm not crashing with builds from 2005-04-30 and before (first crashing build that I found so far is 2005-05-03).

Hmm. Does GetNextWord() return non-null for a single space, say?

(In reply to comment #7) > Hmm. Does GetNextWord() return non-null for a single space, say? Yes, it returns a non-null for a single space.

What causes \r to behave differently (than e.g. a space) in nsTextTransformer::GetNextWord() is that it's explicitly specified in the IS_DISCARDED macro. This is also true for &shy; - this testcase demonstartes the same crash, with &shy; which does not require script to be inserted. Instructions for crashing are as in the previous testcase.

OK. And the same would happen with BiDi control chars. So what do we need to do in PrepareUnicodeDimensions to deal with a sequence of discarded chars and nothing else? rbs, any ideas?

Apparently, there is already a solution (or at least a "solution") for this in the bidi case. So here I'm just adopting that solution for the general case. This does fix my two testcases, but I don't really know the code well enough to explain why initializing the index buffer this way is (or isn't) correct (or even acceptable) in this case.

(In reply to comment #9) > Created an attachment (id=206805) [edit] > testcase using &shy; Probably related: bug 319914.

Yeah that patch fixes the &shy; issue from Bug 319914. Note pasting &shy; (and moving cursor over it in a debug build) actually worked once (meaning did not crash), this has regressed in 2004, for some rough regression range see the Bug.

Notice bug 244876 which would explain why this did not crash before 2004-05-31 (but not why it started crashing only about a year later). It's possible that whoever wrote this code actually relied on newly-initialized buffers to be zero-filled (as they were before bug 244876 was fixed).

Do we want this on branches? Seems like we would...

Comment on attachment 206812 [details] [diff] [review] patch? Looks reasonable.

-#ifdef IBMBIDI - if (indexp && (mState & NS_FRAME_IS_BIDI) ) { + if (indexp) { while (--n >= 0) { *indexp++ = strInx++; } } -#endif // IBMBIDI I wonder why Bidi is doing what it is doing. For normal cases, it seems to me that strInx shouldn't be incremented because it is a bit at odd with the spirit of that code. For future refences, here is what that code is doing. Assume for simplicity mContentOffset = 0, and suppose the corresponding DOM content of the nsTextFrame has these four characters: 0 1 2 3 4 mContentLength = 5 a|&szlig;|&shy;|&shy;|b Now, assume that it has the CSS "text-transform: uppercase". Hence the German &szlig; "expands" to SS and &shy; "shrinks" to nothing. PrepareUnicodeText is meant to deal with both situations: expansion and contraction. In the example, the prepared output becomes: 0 12 3 <--- textlength = 4 A|SS|B What PrepareUnicodeText does is to encode this transformation. To do so, indexp will run up to mContentLength and record the _beginning_ of the mapped "segment" correspond to each DOM character. Thus: indexp[]: runs from 0 to 5, and maps a|&szlig;|&shy;|&shy;|b 0| 1 | 2 | 3 |4|5 <--- this is indexp ^ extra to: 0| 1 | 3 | 3 |3|4 <--- this is strInx ^ textlength In other words, the i-th DOM character is representable with the peparedText[ indexp[i] : indexp[i + 1] - 1 ]. You can see how the extra is set from this last bit of code: if (aIndexBuffer) { PRInt32* ip = aIndexBuffer->mBuffer; ip[mContentLength] = ip[mContentLength-1]; if ((ip[mContentLength] - mContentOffset) < textLength) { // Must set up last one for selection beyond edge if in boundary ip[mContentLength] = textLength + mContentOffset; } } Note how it does not increment ip[mContentLength] if the content has indeed collapsed to nothingness. Given that the spirit of the code is to map the i-th DOM character to the corresponding peparedText[ indexp[i] ... indexp[i + 1] - 1 ], incrementing is creating fictional (unset) references. Can you just try: + if (indexp) { while (--n >= 0) { - *indexp++ = strInx++; *indexp++ = strInx; } }

Yes, this works as well, and makes more sense. Thanks for the explanation in comment #17 - I now understand this better.

Comment on attachment 206907 [details] [diff] [review] patch v2 r=rbs

Patch checked in, 2005-12-27 07:15.

Comment on attachment 206907 [details] [diff] [review] patch v2 a=dveditz for drivers

Checked in to 1.8 and 1.8.0 branches: Checking in layout/generic/nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.513.4.7; previous revision: 1.513.4.6 done Checking in layout/generic/nsTextFrame.cpp; /cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v <-- nsTextFrame.cpp new revision: 1.513.4.6.2.1; previous revision: 1.513.4.6 done

v.fixed on 1.8.0.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060109 Firefox/1.5.0.1, no crash with uri's testcase.