138052 - data: is still too agressive about stripping whitespace XML Parsing Error: not well-formed

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[timeless]

Comment 1

[timeless]

Attachment: check for xml and clearly check for text/ at the beginning of the string

The current code works for any mime type beginning with 'text', and will treat
a mime type of texture/map as if it were text.
It's my belief that we probably don't want that.
Although I'm adding the same sort of bug by checking for xml instead of +xml or
-xml or xml as the last n chars or ...

The other problem with Find("text") is from a readability perspective, it does
a Find without comparing against 0 when it really wants to know if the first n
chars are text. this costs a bit in the failure case and confuses people who
know that sometimes people think that 0 is a failure case and wonder if the
code should use kNotFound...

Anyways. the key fix for this bug is to check for xml because that's what xul,
xbl and svg are.

fwiw biesi wrote this patch first...

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... why are we doing this?  If I understand rfc 2397 and rfc2396 correctly,
spaces in a data: url should be encoded as %20 (rfc 2396, section 2.4.3) and
spaces that are not encoded should be stripped, per RFC 2396, appendix E....

Comment 3

[timeless]

this is parity, we're already doing it for text/* (well text*), i'd like us to 
do it for xml things.  Currently you can't use %20 because of some other bug.

for testing purposes, encoding spaces is a royal pain even if it were to work.

Comment 4

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

I'd probably have used something like Substring(mContentType, 0, 5).Equals("text/")

darin: Why is this whitespace removing done, anyway? A base64 decoder should
ignore whitespace anyway (so we could remove the lBase64 from the if, in any
case); why do we want to remove them from non-text/ mimetypes?

Comment 5

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

eh, nevermind my last comment, I just read bz's comment.

However, I'd still argue that we shouldn't strip them if the data is base64
encoded, because the B64 decoder should strip spaces anyway, so the extra
stripping just takes time.

Comment 6

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

Comment on attachment 79694 [details] [diff] [review]
check for xml and clearly check for text/ at the beginning of the string

can you use strncmp(...) == 0 instead of !strncmp(...), I find that more
readable

r=biesi with that change

Comment 7

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

darin, could you super-review?

Comment 8

[Darin Fisher]

Comment on attachment 79694 [details] [diff] [review]
check for xml and clearly check for text/ at the beginning of the string

sr=darin (sorry for not reviewing this sooner)

Comment 9

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

checked in

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[benc]

I click on the link, and nothing happens. What should happen? (Mozilla 1.2., Win98)

Comment 12

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

benc: Hm, interesting that it doesn't load - what should happen is that an empty
(I think) page should load. What should not happen is that you get an XML
Parsing error; that error would occur because spaces were stripped (which was
what this patch changed).

try pasting the url in the urlbar and pressing enter maybe.

Comment 13

[benc]

No response when pasting into Location (Mozilla 1.1, Win98).

Comment 14

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

benc: that's strange... here, on a recent nightly, I get the expected blank page.
do you get any message in the JS Console?

Comment 15

[benc]

Is that all it's supposed to do? Oh. that is what happens to me.

QA people like testcases that say "this is working great!" if everything works.
Lots of us are blackbox testers, so we want to just click and go.

Comment 16

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

benc, for your testing pleasure, a testcase that shows a "this works" message :)
(click the changed URL)

Comment 17

[benc]

VERIFIED: Mozilla 1.3a, Win98+MacOS X

Comment 18

[benc]

VERIFIED: Mozilla 1.3a, Linux