108575 - rethink contents of escape matrix

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Andreas Otte]

I'm rereading rfc2396 to verify or change the contents of the escape matrix in
nsStdEscape.

Comment 1

[Andreas Otte]

Attachment: first patch on escape matrix

Comment 2

[Andreas Otte]

changes:

@ now also allowed in directory
' now allowed everywhere
~ now allowed everywhere
; now also allowed in scheme, username, password and host

this needs some more testing of course ...

Comment 3

[Andreas Otte]

Some more explanations:

' and ~ 

Both characters belong into the unreserverd charset (section 2.3). No
restrictions, so no escaping should be necessary at all.

@ and ; 

Both characters belong to the reserved charset. According to section 3.2 ";" is
reserved in the authority component for registry-based authoritys and can be
used in server-based authoritys. "@" is not reserved in the directory component
according to section 3.3, but it is also explicitly allowed, so we can remove
the current escaping there.

Comment 4

[Doug Turner (:dougt)]

maybe the best way to go about getting this in the builds is to conditionally
use this new matrix based on a preference or env var.  andreas, are you up for
adding the code which will allow QA to flip a switch and use this new matrix?

Comment 5

[Andreas Otte]

Sure, I will try that. Having two matrixes is a little bloat, but okay I think.
I would like to use a pref to switch between the two matrixes, how much would
that hurt performance?

Comment 6

[Darin Fisher]

just check the pref once at startup and cache the result, or if you want to get
fancy, add a pref change callback :)

Comment 7

[Andreas Otte]

Hmmm ... for now I have it working with a pref check at each nsStdEscape call
and it works fine, but makes xpcom requires pref. Looking into the caching stuff
... 

Comment 8

[Darin Fisher]

actually, an environment variable would be better.  no ones going to approve
making xpcom depend on libpref BTW.

Comment 9

[Andreas Otte]

That's what I thought :(

Comment 10

[Andreas Otte]

Attachment: switch between two escape matrix based on env-variable ALT_ESC_MATRIX

switch between the matrixes based on env variable ALT_ESC_MATRIX. No caching
yet, can't find a suitable place to store the global variable.

Comment 11

[Darin Fisher]

andreas: i think you only check the env variable once... calling getenv
everytime is unnecessary.

Comment 12

[Andreas Otte]

Yes I know, but I can't find a suitable place to cache the env variable. On the
other hand I don't think it is necessary to do this env-stuff at all.

' and ~ are not used in the urlparser anywhere, changing it's escaping value is
without any danger.

We look at @ in ParseURL and ParseAuthority, in ParseURL we first look for /, so
there is no danger mistaking a @ as part of the path with the @ inside the
authority. No danger here too.

; in scheme, username, password and host is a bit more complicated. There could
be a problem with the urlparser. I have to dig deeper there. So what about
dropping the changes for ; and put the other changes in (unconditionally) after
the tree opens for 0.9.8?

Comment 13

[Darin Fisher]

sounds reasonable to me.

Comment 14

[Andreas Otte]

Attachment: new patch, direct change to escape matrix, but only for @'~

Comment 15

[Andreas Otte]

Doug, I moved the ; part (which could have caused problems) out of the patch,
the other changes are trivial without any impact I can think of to url parsing.
So, I think we can put this directly into the matrix without any pref or env
variable.

Comment 16

[Darin Fisher]

Comment on attachment 60223 [details] [diff] [review]
new patch, direct change to escape matrix, but only for @'~

sr=darin

Comment 17

[Andreas Otte]

fix checked in, dropped the ; stuff