If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

()

Core
Networking
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: timeless, Assigned: Biesinger)

Tracking

({testcase})

Trunk
x86
All
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
 
(Reporter)

Comment 1

16 years ago
Created attachment 79694 [details] [diff] [review]
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...
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....
(Reporter)

Comment 3

16 years ago
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.
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?
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 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
Attachment #79694 - Flags: review+
darin, could you super-review?

Comment 8

16 years ago
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)
Attachment #79694 - Flags: superreview+
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
*** Bug 142523 has been marked as a duplicate of this bug. ***

Comment 11

15 years ago
I click on the link, and nothing happens. What should happen? (Mozilla 1.2., Win98)
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

15 years ago
No response when pasting into Location (Mozilla 1.1, Win98).
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

15 years ago
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.
benc, for your testing pleasure, a testcase that shows a "this works" message :)
(click the changed URL)

Comment 17

15 years ago
VERIFIED: Mozilla 1.3a, Win98+MacOS X
Keywords: testcase

Comment 18

15 years ago
VERIFIED: Mozilla 1.3a, Linux
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.