119335 - nsXMLContentSerializer::AppendTextData() causes assertion in nsDependentString::Rebind()

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect, P3)

Description

[kinmoz]

I've been seeing this the last few weeks with a trunk build ...

Sometimes when saving an HTML doc in composer I see several assertions like:

###!!! ASSERTION: nsDependentString must wrap only null-terminated strings: '!*a
EndPtr', file ..\..\dist/include/string\nsDependentString.h, line 86
###!!! Break: at file ..\..\dist/include/string\nsDependentString.h, line 86

The assertion seems to be triggered by the following code in
nsXMLContentSerializer::AppendTextData():


    if (frag->Is2b()) {
      AppendToString(nsDependentString(frag->Get2b()+aStartOffset, length),
                     aStr,
                     aTranslateEntities,
                     aIncrColumn);
    }
    else {
      AppendToString(NS_ConvertASCIItoUCS2(frag->Get1b()+aStartOffset, length),
                     aStr,
                     aTranslateEntities,
                     aIncrColumn);
    }

The 1b and 2b pointers in the text fragment are not null terminated, so you may
want to use something other than a Dependent string, or make a local copy of the
fragment contents and null terminate it before using the dependent string.

Here's the partial stack trace that shows how I'm hitting the assertion:


NTDLL! 77f7629c()
nsDebug::Assertion(const char * 0x1010ce38
??_C@_0DJ@PCFA@nsDependentString?5must?5wrap?5only@, const char * 0x1010ce7c
??_C@_09OKBI@?$CB?$CKaEndPtr?$AA@, const char * 0x1010ce00
??_C@_0CO@HHMI@?4?4?2?4?4?2dist?1include?1string?2nsDepe@, int 86) line 290 + 13
bytes
nsDependentString::Rebind(const unsigned short * 0x0613d060, const unsigned
short * 0x0613d1ba) line 86 + 34 bytes
nsDependentString::Rebind(const unsigned short * 0x0613d060, unsigned int 173)
line 96
nsDependentString::nsDependentString(const unsigned short * 0x0613d060, unsigned
int 173) line 99 + 51 bytes
nsXMLContentSerializer::AppendTextData(nsIDOMNode * 0x0613d37c, int 0, int -1,
nsAString & {...}, int 1, int 0) line 120 + 40 bytes
nsHTMLContentSerializer::AppendText(nsHTMLContentSerializer * const 0x06749670,
nsIDOMText * 0x0613d37c, int 0, int -1, nsAString & {...}) line 161 + 31 bytes
nsDocumentEncoder::SerializeNodeStart(nsIDOMNode * 0x0613d37c, int 0, int -1,
nsAString & {...}) line 318
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613d37c, nsAString
& {...}) line 373 + 20 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613d43c, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613d5a0, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613da5c, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613cefc, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0613b05c, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x0606406c, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x05f9f21c, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::SerializeToStringRecursive(nsIDOMNode * 0x06052d54, nsAString
& {...}) line 394 + 21 bytes
nsDocumentEncoder::EncodeToString(nsDocumentEncoder * const 0x06749760,
nsAString & {...}) line 934 + 21 bytes
nsDocumentEncoder::EncodeToStream(nsDocumentEncoder * const 0x06749760,
nsIOutputStream * 0x06749bb0) line 975 + 19 bytes
nsDocument::SaveFile(nsDocument * const 0x06052d78, nsIURI * 0x06748440, int 1,
int 0, const unsigned short * 0x0674c9f0, const unsigned short * 0x016c0f84,
unsigned int 258, unsigned int 4294967295) line 3450 + 44 bytes
nsEditor::SaveFile(nsEditor * const 0x05fe0d40, nsIURI * 0x06748440, int 1, int
0, const nsAString & {...}) line 1009 + 104 bytes

Comment 1

[Sitsofe Wheeler]

This looks similar to bug 109813 and bug 115075.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Use Substring

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

jst suggested Substring(). I did that, and my own bug 121897 seemed to work
after this change. So I am pretty confident all 3 of those other bugs are dupes
of this. Taking and marking the dupes.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 109813 has been marked as a duplicate of this bug. ***

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 115075 has been marked as a duplicate of this bug. ***

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 121897 has been marked as a duplicate of this bug. ***

Comment 7

[jag (Peter Annema)]

Comment on attachment 67801 [details] [diff] [review]
Use Substring

sr=jag

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 67801 [details] [diff] [review]
Use Substring

r=jst

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.

Comment 10

[Akkana Peck]

Not fixed.  Go to:
http://www.mozilla.org/quality/browser/front-end/testcases/xpapps-gui/find-on-page/find-in-page.html
Scroll down to "Long almost-match" and doubleclick on "Test" in italics right
beneath it, and you'll see the assert (at least on linux in a build from today).

Comment 11

[Akkana Peck]

I've been asked to reassign this to Tanu.

Comment 12

[Tanu Mutreja]

Repeating the same steps as Akkana mentioned above, I'm not getting the 
assertions on Linux with Feb24'02 build.

Comment 13

[Tanu Mutreja]

In fact on Linux I'm getting following ASSERTION failure message:

NS_ASSERTION(aStartOffset == 0,"a start offset is beyond the end of the text 
fragment!");
at nsXMLContentSerializer.cpp line 110. 

I feel this is not related to the original bug mentioned there.

Comment 14

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 15

[Tanu Mutreja]

Attachment: Patch to fix "Assertion Failure" mentioned in comment#13.

On Linux, double clicking on the HTML content generates a copy event. On double
clicking a word, here selection range is including end of the last tag too (I
don't understand the intent of that! ). In the test case mentioned by Akkana,
last tag here is an <h2> containing "Long almost-match:".Hence aStartOffset is
pointing to the last position of this text and has the value greater than 0
i.e. 18.

In existing code I could not see any reason for asserting "aStartOffset == 0".
It may be greater than 0 but should be less than ((aEndOffset == -1) ?
frag->GetLength() : aEndOffset).  Here I'm including generic assertions that I
find reasonable and removing the earlier one.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 72777 [details] [diff] [review]
Patch to fix "Assertion Failure" mentioned in comment#13. 

sr=jst

Comment 17

[Akkana Peck]

Comment on attachment 72777 [details] [diff] [review]
Patch to fix "Assertion Failure" mentioned in comment#13. 

r=akkana

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 72777 [details] [diff] [review]
Patch to fix "Assertion Failure" mentioned in comment#13. 

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 19

[Vinay Badami]

Checkin in for tmutreja
Fixed with checkin
D:\mozilla\content\base\src>cvs commit
cvs commit: Examining .
Checking in nsXMLContentSerializer.cpp;
/cvsroot/mozilla/content/base/src/nsXMLContentSerializer.cpp,v  <--  nsXMLConten
tSerializer.cpp
new revision: 1.22; previous revision: 1.21
done

Comment 20

[sujay]

Reporter: please verify and mark verified-fixed...thanks!