240837 - [FIXr]strange text ' ">P ' inserted into retrieved text.

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Jan Ruzicka]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040417
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7b) Gecko/20040417

In the page gopher://gopher.floodgap.com/0/gopher/wbgopher,
Text contains strange characters '">P' .
(that is quotation mark, greater the sign, caplital letter p)
this text appears within retrieved text.




Reproducible: Always
Steps to Reproduce:
1.open location gopher://gopher.floodgap.com/0/gopher/wbgopher

Actual Results:  
[... some other text ...]
It ha">Ps been mentioned to me that Opera needs a proxy server to browse
[... some other text ...]


Expected Results:  
[... some other text ...]
It has been mentioned to me that Opera needs a proxy server to browse
[... some other text ...]


The text shown is also truncated.

Original data can be retrieved by:
1. on command line enter: 
      telnet gopher.floodgap.com 70 
2. in telnet session enter:
      gopher/wbgopher

Can also somebody test other platforms?

Comment 1

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040415

See http://www.armory.com/%7Espectre/cwi/hl/

It ha">s been mentioned to me that Opera needs a proxy server to browse

View Source:

See http://www.armory.com/%7Espectre/cwi/hl/

It ha"></a>s been mentioned to me that Opera needs a proxy server to browse
Gopherspace. This will also work for other gopher-less browsers.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Happens on Linux too.  Note also the garbage at the very bottom:

  href="f="mailto:href="

If I load the whole thing with type T instead, no problem.  So this seems like a
bug in the text-to-HTML converter....

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm seeing nsTXTToHTMLConv::CatHTML called with:

  front: 5982, back: 5059

and

  front: 5991, back: 5981

That seems way wacky.  front should be smaller than back, no?  Darin, I bet
RFindCharInSet got broken by your landing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

We had no tests for this.  Oops.

It looks like the only conumers are the two text-to-html converters, though....

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Index: xpcom/tests/windows/nsStringTest.h

hm, I can't find any user of this file...

I'd suggest patching xpcom/tests/TestStrings.cpp instead.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, jeez.  I patched the test file that had tests for RFindCharInSet.... 
TestStrings.cpp has none, nor tests for FindCharInSet.

I may try to write some, but if someone else (more familiar with the file) has
the time to do it, that would be great....

Comment 8

[Darin Fisher]

yeah, TestStrings.cpp is where these testcases should be added.  i volunteer to
write those testcases since i want to use that as a way to understand and verify
this patch.

Comment 9

[Darin Fisher]

ack, ok... this was me misunderstanding the meaning of RFindCharInSet's offset
parameter :(  i thought offset defined the starting index of a substring of
|this| to search, but in reality that's not at all the case.  you're patch looks
correct to me.  i like the fact that you have modified ::RFindCharInSet to not
dereference |data + dataLen|.  i'm pretty sure we don't care about using this
function to detect the trailing null char, so that should be ok.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, yeah.  The offset is the place to start searching.... This wasn't very
clearly documented in the old string code, I guess....

And thanks for doing the tests, darin!

Comment 11

[Darin Fisher]

Attachment: patch for TestStrings.cpp

Comment 12

[Darin Fisher]

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

r+sr=darin

Comment 13

[Darin Fisher]

i've checked in the patch for TestStrings.cpp on the trunk

Comment 14

[Boris Zbarsky [:bzbarsky]]

Could you add a test for FindCharInSet without an offset param passed in too?

Fix checked in on trunk; leaving open pending 1.7 checkin.

Comment 15

[Neil Fraser]

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

Comment 16

[benc]

Jan, can you check a trunk build now?

Comment 17

[Jan Ruzicka]

Looks good.

Comment 18

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.7

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.7 branch.

Comment 20

[Darin Fisher]

(In reply to comment #14)
> Could you add a test for FindCharInSet without an offset param passed in too?
> 
> Fix checked in on trunk; leaving open pending 1.7 checkin.

done (on trunk)

Comment 21

[Colin Blake]

I don't know if its this patch which caused the problem, but xpcom_break_on_load
doesn't work any more for me (OpenVMS). I've been away since 1.4 so I'm not sure
when or why it broke, but the string test at
http://lxr.mozilla.org/seamonkey/source/xpcom/components/xcDll.cpp#430 doesn't
work for me any more. Is it working for other platforms?

Comment 22

[Colin Blake]

OK, I just found bug 243429 which already reports that xpcom_break_on_load is
busted.