163988 - Crashes at www.macgamer.com (HREF="&#)

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Petteri Kamppuri]

Whenever I try to load http://www.macgamer.com/news/item.php?id=5716 with
Mozilla 1.1b it crashes. I verified this with the latest nightly build I could
grab from mozilla.org (sorry can't get the build id right now, because I can't
run two copies of Mozilla at the same time). Reproducible every time.

Comment 1

[Petteri Kamppuri]

Attachment: Crash log

This is the crash log I get with the official Mozilla 1.1b.

Comment 2

[Jerry Baker]

Confirmed 2002082109 trunk on Windows XP.

Platform/OS -> All

Talkbacks:
TB9679328W
TB9679210W

Comment 3

[Eli Friedman]

Attachment: Testcase (crashes)

Note: all the file contains is '<A HREF="&#65279;http://www.google.com">'.  It
crashes immediately upon loading.  I guess Mozilla is crashing when it tries to
process the entity in URL.

Comment 4

[Torben]

Crash still happens with build 2002102415, TalkBack ID TB13239655X. Updating
summary to be easier to query. 
The test case does not crash for me, [e-mail] could you please
review it?

Comment 5

[Torben]

Attachment: Testcase

I created a new testcase my self; in addition to the <A
HREF="&#65279;http://www.google.com/"> it also must contain a charset
declaration. Crash occurs both with iso-8859-1 and iso-8859-15, but _not_ with
UTF-8.

Comment 6

[Torben]

Attachment: Testcase that does not crash

Same as atachment 104516, but with charset=UTF-8

Comment 7

[Torben]

Strange, the testcase does not crash when loaded directly from the server, it
must be saved to disk and load from there to crash.

Comment 8

[Torben]

Changing component (hope this is the right one).

Comment 9

[Torben]

Updating summery, this happens with recent mozilla builds also.

Comment 10

[Andrew Schultz]

Attachment: valgrind log

tested this a few times, and the only thing that appeared consistently was
malloc at the top of the stack.  tried it in valgrind and got this.

debug build says
###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated:
'Error', file nsString2.cpp, line 1450

this seems related to bug 191965, although that should be fixed on trunk.

Comment 11

[Andrew Schultz]

==> networking

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The assertion seemed to be when count==24 and mLength==23, which may be what I
mentioned in bug 172700 comment 9.

Comment 13

[Doug Turner (:dougt)]

i can confirm the crash going to the above URL (none of the attached testcases
cause the crash).

Comment 14

[Andrew Schultz]

> none of the attached testcases cause the crash

for some reason, you have to download the testcase before it will work

Comment 15

[Mats Palmgren (inactive)]

WFM, 2003-06-25-05 trunk Linux.  Maybe it was fixed by bug 188278?

Comment 16

[Andrew Schultz]

I'm hanging with the same build and a debug build still throws the assertion in
comment 10 (which came after bug 188278)

Comment 17

[Darin Fisher]

i'm able to repro this crash using the 20030624 trunk build of firebird under WinXP.

Comment 18

[Walter Shen]

Attachment: Patch

Removed check on BOM.

Comment 19

[Walter Shen]

Comment on attachment 126785 [details] [diff] [review]
Patch

This was causing the conversion from UTF8 to UCS2 (UTF16) to fail.  It seems
that it was not stripping BOM's properly, so I just removed that check, as you
suggested.  It fixes the crash, because Necko was assuming the conversion was
going to succeed.  (And now it does, I think.)

Comment 20

[Darin Fisher]

-> walter

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 126785 [details] [diff] [review]
Patch

I don't know why waterson originally added this code to ignore the BOM, but I
think it's wrong at this level.  (It's a valid Unicode character, after all.) 
It should be (and is) done in the decoders for the encodings that use the BOM
as a byte-order mark.

jag should superreview.

You should generated a new patch without the "^M"s at the end of the three
lines before this is checked in.  (You should be able to tell you've succeeded
when the braces don't show as part of the +/- lines in the diff.)  You can't
use the VC++ editor if you've set up cygwin with UNIX newlines.

Comment 22

[jag (Peter Annema)]

Comment on attachment 126785 [details] [diff] [review]
Patch

sr=jag

Comment 23

[Walter Shen]

Attachment: new patch without ^M's

Same patch, got rid of the line returns.

Comment 24

[Darin Fisher]

fixed-on-trunk

Comment 25

[Chris Waterson]

IIRC, I just copied and pasted (or rewrote, possibly incorrectly) some code that
frang had done originally.  This bug maybe lurk elsewhere in the code.