110508 - URL: doing SetSpec() with a string with a query doesn't do the right thing

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

doing SetSpec() with a string with a query doesn't do the right thing

call set spec with a valid url that has a query string.

calls to the url after that (like GetFilePath()) will return the filepath, plus 
the escaped query string.

this caused http://bugzilla.mozilla.org/show_bug.cgi?id=110478

Comment 1

[Jean-Francois Ducarroz]

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

Comment 2

[Bradley Baetz (:bbaetz)]

Copying my comment from bug 110478:

This is actually correct behaviour. ? isn't special in a file url. Before this,
it was impossible to open a file url containing a ? character.

This was bug 102603 (also see bug 59002). I mentioned it to darin, but I thought
he'd decided not to fix this the first time arround.

How did mailnews previously work with a local folder containing ? in the name?

The escaping is probably cosmetic. Note that spaces have been escaped, too. I'm
not sure whether that counts as correct behaviour or not, but it is correct that
the filepath includes the ?. File urls do not have a query term.

Comment 3

[Jean-Francois Ducarroz]

Bradley, mailnews is not using file url but mailbox url for which we know how to 
deal with the query part. With the new url parser, we seems not able anymore to 
correctly differentiate the path part from the query part! When you query the 
url path, you get the query with it!

Comment 4

[Darin Fisher]

see my comments + patch:
http://bugzilla.mozilla.org/show_bug.cgi?id=110478#c31

Comment 5

[Darin Fisher]

Attachment: v1.0 patch

query string should start with first '?' in the path, not the last :/

Comment 6

[Darin Fisher]

i'll also add a testcase to urlparse.dat for regression testing.

Comment 7

[Darin Fisher]

bbaetz, seth: can you guys review this patch?  thx!

Comment 8

[Jean-Francois Ducarroz]

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

Comment 9

[Jean-Francois Ducarroz]

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

Comment 10

[(not reading, please use [e-mail] instead)]

bugs 110502 and 110582 are not dups of this bug.

the reason fwd inline and edit as new are failing is because IsValidScheme() is 
returning false when our spec has '_' in the scheme.

for example, here's the uri we're passing to CreateStartupUrl():

"mailbox_message://[e-mail]/Inbox#9210143fetchCompleteMessage=true
"

I've got a fix, I'll attach it for darin and bbaetz to review.

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: patch, includes darin's original patch.

Comment 12

[(not reading, please use [e-mail] instead)]

hmm, looking at http://www.ietf.org/rfc/rfc2396.txt

scheme        = alpha *( alpha | digit | "+" | "-" | "." )

and http://www.w3.org/Addressing/rfc1738.txt

; the scheme is in lower case; interpreters should use case-ignore
scheme         = 1*[ lowalpha | digit | "+" | "-" | "." ]

'_' is not a legal character in a URI or a URL scheme.

I'll go look into fixing the mailnews schemes.

Comment 13

[Darin Fisher]

interesting... RFC2396 says:

scheme        = alpha *( alpha | digit | "+" | "-" | "." )

meaning that '_' isn't a valid element of an URI scheme.  portions of the old
code didn't even support '_' as an URI scheme (see
nsURLHelpers.cpp:ExtractURLScheme).

however, i can't think of any problems that would arise if we did allow '_' as
part of an URI scheme, so i think the patch is fine.

cc'ing andreas

Comment 14

[(not reading, please use [e-mail] instead)]

I'll hold off on converting from "foo_message://" to "foo-message://" until the 
jury (bbaetz, andreas, darin) make a decision.

Comment 15

[Bradley Baetz (:bbaetz)]

The spec says its bad, and I presume its just a couple of deinfes to change?

Whilst this isn't exposed externally, its probably not a bad idea to change the
scheme.

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: change foo_message -> foo-message

this will get edit as new, and fwd inline working again.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 58310 [details] [diff] [review]
change foo_message -> foo-message

obsolete, moved to bug #110502

let's let his bug just cover the original issue, so only darin's patch is
relevant.

Comment 18

[(not reading, please use [e-mail] instead)]

Comment on attachment 58190 [details] [diff] [review]
v1.0 patch

r=sspitzer on darin's patch.

Comment 19

[Bradley Baetz (:bbaetz)]

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

Comment 20

[Ashley Bischoff (blog at handcoding.com)]

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

Comment 21

[Darin Fisher]

Comment on attachment 58190 [details] [diff] [review]
v1.0 patch

bbaetz r='d this patch in bug 110478.

Comment 22

[Matthias Versen [:Matti]]

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

Comment 23

[Bradley Baetz (:bbaetz)]

Judging from the dupes, just because strings like this are invalid doesn't mean
that people don't rely on it....

Comment 24

[Andreas Otte]

One change between the old and new urlparser is that we are now checking the
scheme for vaild chars while parsing it. Previously we only did that while
resolving a relative url for example to figure out if we got an absolute or
relative url to resolve. I see no imminent danger in having _ allowed in schemes
but on the other side we have some bugs begging us to do more checks on urls, to
help deceiding which text in mail-messages to url-ify. I think, to help these
bugs we should stay with the check as it is now and change the schemes according
to the rfc.

Comment 25

[Neil Pryde]

This is so stupid, I can't even check my e-mail on netscape servers, so just fix
this thing a.s.a.p! 

Comment 26

[Ashley Bischoff (blog at handcoding.com)]

Found another testcase; setting as URL.
(http://msdn.microsoft.com/downloads/default.asp?url=/downloads/sample.asp?url=/msdn-files/027/001/777/msdncompositedoc.xml)

Neil: Enthusiasm is appreciated, but pestering the Mozilla developers is not
going to get this fixed any faster.

Comment 27

[Darin Fisher]

fixed-on-trunk

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Neil Pryde]

Alex, it just seems that the patch wasn't tested, before it was added to the
mozilla trunk, and this wasn't the first time that happened, nor will it be the
last :)

Comment 31

[Matthias Versen [:Matti]]

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

Comment 32

[Ashley Bischoff (blog at handcoding.com)]

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

Comment 33

[Matthias Versen [:Matti]]

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

Comment 34

[Matthias Versen [:Matti]]

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

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

Comment 36

[benc]

VERIFIED:
Mozilla 1.1, Mac OS X and Win 98. 

Comment 37

[benc]

VERIFIED: Mozilla 1.4/Linux 
used: /cgi-bin/printenv?query1?query2 on any apache server