toString/toSource of RegExp does not escape slashes

VERIFIED FIXED in mozilla1.8alpha2

Status

()

P3
normal
VERIFIED FIXED
15 years ago
12 years ago

People

(Reporter: timeless, Assigned: shaver)

Tracking

({fixed-aviary1.0, fixed1.7, js1.5})

Trunk
mozilla1.8alpha2
fixed-aviary1.0, fixed1.7, js1.5
Points:
---
Bug Flags:
blocking-aviary1.0 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
js> eval(""+RegExp("[^/]+$"))
typein:1: SyntaxError: unterminated character class ^:
typein:1: /[^/]+$/
typein:1: ^
js> build()
built on Jun 21 2004 at 18:03:35
(boffo)
also tested: mozilla1.4.1 (viper), mozilla1.2.1 (boffo), 1.7b (lab-xp-tp1)

afaik RegExp("[^/]+$") should be approximately equivalent to /[^\/]+$/

The testing example I have is:

"somepath/file.ext".match(regexpobject)[0]

expected output is: "file.ext".
since I'm using the regular expression twice, I figured I'd construct and store
the object instead of inlining it. Since I'm not particularly fond of escaping
things, I decided to use the RegExp() form. The form works, but xpcshell/js and
venkman all showed sketchy string representations. Note that the non RegExp
constructor /[^\/]+$/ form does correctly unescape.

The nature of the error message for my testcase will vary depending on whether
the build you're using is from before or after the regexp landing (e.g. 1.2.1
output):
js> eval(""+RegExp("[^/]+$"))
typein:1: SyntaxError: unterminated character class [:
typein:1: /[^/]+$/
typein:1: ^
js> build()
built on Nov 30 2002 at 14:36:14

Things to test:
/[^\/]+$/
RegExp("[^\\\/]+$")
RegExp("[^\\/]+$")
RegExp("[^\/]+$")
RegExp("[^/]+$")

Personally, I expect all of the preceding to behave the same (both as regular
expressions and as things convertable to strings).
(Reporter)

Updated

15 years ago
Summary: Unescape of character class [^/] is broken → Unescape of character class [^/] constructed with RegExp constructor is broken
Ah, sweet RegExp.  I think we're up to about 800 cuts now.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

15 years ago
Keywords: js1.5
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.8alpha2
(Reporter)

Comment 2

15 years ago
js> RegExp("/")
///
It's true that the toString/toSource representation isn't lot of use as
eval-fodder, but IE(6) does the same thing, so I'm not sure we can change
toString.  Maybe toSource?  Teetering towards WONTFIX.

Note that the RegExp(RegExp("/").source) works, though.

Assignee: general → shaver
Summary: Unescape of character class [^/] constructed with RegExp constructor is broken → toString/toSource of RegExp does not escape slashes
Not js1.5 -- breaking compat isn't a release requirement, usually.

(Wonder what 4.x does.)
Status: NEW → ASSIGNED
Keywords: js1.5
(Reporter)

Comment 5

15 years ago
Mozilla/4.8 [en] (Windows NT 5.0; U): (RegExp("/")).toSource() => /// 
Mozilla/4.8 [en] (Windows NT 5.0; U): (RegExp("/")).toString() => /// 
Created attachment 152453 [details] [diff] [review]
proposed fix

Per ECMA-262 Edition 3, 15.10.6.4.

/be

Updated

15 years ago
Attachment #152453 - Flags: review?(shaver)
See the NOTE in 15.10.6.4 -- it allows "///" to be the result of
RegExp("/").toString().  The spec also allows what the patch here does, which
seems better enough to be worth doing.

/be
Comment on attachment 152453 [details] [diff] [review]
proposed fix

Seems kinda a shame that we have this match_or_replace thing that can do _one_
text replacement for us, but not a global string replacement.  Still...
Attachment #152453 - Flags: review?(shaver) → review+
match_or_replace can work globally.  However, calling it from the engine to
replace / with \/ is tricky: you need a regexp with a negative lookbehind
asserting that \ does not precede / (either / is at the front of the string, or
there's no \ char before the /).  Seems better to open-code it (and not to worry
about avoiding reallocs).

/be
Created attachment 152477 [details] [diff] [review]
proposed fix, v2

Oops -- first patch sucked, forgot to scale length by sizeof(jschar).

/be

Updated

15 years ago
Attachment #152477 - Flags: review?(shaver)

Updated

15 years ago
Attachment #152453 - Attachment is obsolete: true
Attachment #152453 - Flags: review+
Comment on attachment 152477 [details] [diff] [review]
proposed fix, v2

Even better. r=shaver
Attachment #152477 - Flags: review?(shaver) → review+
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Updated

15 years ago
Attachment #152477 - Flags: approval1.7.1?
Attachment #152477 - Flags: approval1.4.3?
1.4.3?  Whatever.

Timeless, you can do the work.  I actually would rather *not* make this change
in any branch, since we were ECMA-conformant, and backward compatible, till this
went in for 1.8a2.  What's your requirement, exactly?

/be
Would like to sync up aviary 1.0 with current trunk in this regard.

/be
Flags: blocking-aviary1.0+
(Reporter)

Comment 15

15 years ago
i'm not likely to use 1.4.3, but i am likely to use 1.7.x. I'm fine with not
doing 1.4.3.
Flags: blocking1.7.1?
I am against this landing on the 1.4 branch.
(Reporter)

Updated

15 years ago
Attachment #152477 - Flags: approval1.4.3?
Comment on attachment 152477 [details] [diff] [review]
proposed fix, v2

Per drivers' edict that 1.7x and aviary1.0 back-end modules match, I'm plusing
for 1.7.3.

/be
Attachment #152477 - Flags: approval1.7.3? → approval1.7.3+
Fixed in 1.7 branch.

/be
Keywords: fixed-aviary1.0, fixed1.7, js1.5

Updated

14 years ago
Flags: blocking1.7.5?

Comment 19

14 years ago
Created attachment 174930 [details]
js1_5/Regress/regress-248444.js

thanks to timeless.

Comment 20

14 years ago
js1_5/Regress/regress-248444.js checked in.

Updated

14 years ago
Flags: testcase+

Comment 21

12 years ago
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.