rethink contents of escape matrix

RESOLVED FIXED in mozilla0.9.8



17 years ago
17 years ago


(Reporter: Andreas Otte, Assigned: Andreas Otte)



Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



17 years ago
I'm rereading rfc2396 to verify or change the contents of the escape matrix in

Comment 1

17 years ago
Created attachment 56592 [details] [diff] [review]
first patch on escape matrix

Comment 2

17 years ago

@ 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 ...


17 years ago
Priority: -- → P3
Target Milestone: --- → mozilla0.9.7

Comment 3

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

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

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

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

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

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

Comment 9

17 years ago
That's what I thought :(

Comment 10

17 years ago
Created attachment 60032 [details] [diff] [review]
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.
Attachment #56592 - Attachment is obsolete: true

Comment 11

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

Comment 12

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

17 years ago
sounds reasonable to me.

Comment 14

17 years ago
Created attachment 60223 [details] [diff] [review]
new patch, direct change to escape matrix, but only for @'~


17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 15

17 years ago
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
Keywords: review


17 years ago
Attachment #60223 - Flags: superreview+

Comment 16

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



17 years ago
Attachment #60223 - Flags: review+

Comment 17

17 years ago
fix checked in, dropped the ; stuff
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.