rethink contents of escape matrix

RESOLVED FIXED in mozilla0.9.8

Status

()

Core
Networking
P3
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Andreas Otte, Assigned: Andreas Otte)

Tracking

Trunk
mozilla0.9.8
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

17 years ago
I'm rereading rfc2396 to verify or change the contents of the escape matrix in
nsStdEscape.
(Assignee)

Comment 1

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

Comment 2

17 years ago
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 ...
Status: NEW → ASSIGNED
(Assignee)

Updated

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

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?
(Assignee)

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 :)
(Assignee)

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.
(Assignee)

Comment 9

17 years ago
That's what I thought :(
(Assignee)

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.
(Assignee)

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.
(Assignee)

Comment 14

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

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

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
variable.
Keywords: review

Updated

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 @'~

sr=darin

Updated

17 years ago
Attachment #60223 - Flags: review+
(Assignee)

Comment 17

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