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

RESOLVED FIXED in mozilla1.7final

Status

()

P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: jan.ruzicka, Assigned: bzbarsky)

Tracking

({fixed1.7})

Trunk
mozilla1.7final
fixed1.7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

15 years ago
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?
(Reporter)

Updated

15 years ago
Blocks: 194220

Comment 1

15 years ago
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.
(Assignee)

Comment 2

15 years ago
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....
OS: MacOS X → All
Hardware: Macintosh → All
(Assignee)

Comment 3

15 years ago
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.
(Assignee)

Comment 4

15 years ago
Created attachment 146393 [details] [diff] [review]
Patch
Assignee: darin → bzbarsky
Status: UNCONFIRMED → ASSIGNED
(Assignee)

Comment 5

15 years ago
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....
Attachment #146393 - Flags: superreview?(darin)
Attachment #146393 - Flags: review?(darin)
(Assignee)

Updated

15 years ago
Priority: -- → P1
Summary: strange text ' ">P ' inserted into retrieved text. → [FIX]strange text ' ">P ' inserted into retrieved text.
Target Milestone: --- → mozilla1.8alpha
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.
(Assignee)

Comment 7

15 years ago
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

15 years ago
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

15 years ago
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.
(Assignee)

Comment 10

15 years ago
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

15 years ago
Created attachment 146519 [details] [diff] [review]
patch for TestStrings.cpp

Comment 12

15 years ago
Comment on attachment 146393 [details] [diff] [review]
Patch

r+sr=darin
Attachment #146393 - Flags: superreview?(darin)
Attachment #146393 - Flags: superreview+
Attachment #146393 - Flags: review?(darin)
Attachment #146393 - Flags: review+

Updated

15 years ago
Attachment #146393 - Flags: approval1.7?

Comment 13

15 years ago
i've checked in the patch for TestStrings.cpp on the trunk
(Assignee)

Comment 14

15 years ago
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.
(Assignee)

Updated

15 years ago
Summary: [FIX]strange text ' ">P ' inserted into retrieved text. → [FIXr]strange text ' ">P ' inserted into retrieved text.

Comment 15

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

Comment 16

15 years ago
Jan, can you check a trunk build now?
(Reporter)

Comment 17

15 years ago
Looks good.

Comment 18

15 years ago
Comment on attachment 146393 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #146393 - Flags: approval1.7? → approval1.7+
(Assignee)

Comment 19

15 years ago
Fixed on 1.7 branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.7
Resolution: --- → FIXED
Target Milestone: mozilla1.8alpha → mozilla1.7final

Comment 20

15 years ago
(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

15 years ago
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

15 years ago
OK, I just found bug 243429 which already reports that xpcom_break_on_load is
busted.
You need to log in before you can comment on or make changes to this bug.