If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Should JS support octal escape sequences in regexps?

VERIFIED FIXED in mozilla1.2alpha

Status

()

Core
JavaScript Engine
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Phil Schwartau, Assigned: rogerl (gone))

Tracking

({js1.5})

Trunk
mozilla1.2alpha
x86
All
js1.5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Have filed bug 158159 against Rhino for same issue])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
This arose from the HTML in bug 137458.

The site there was generated by Microsoft FrontPage v5.0.
Apparently Microsoft still supports the use of octal escape sequences
such as \240 within regular expressions. That is, sequences of form \OOO,
where each 'O' is a octal digit. This was accepted in ECMA-262 Edition 2.


The bug 137458 site uses constructs such as this:

              "#FF1111".replace(/\240/g, " ")

i.e. replace the character given by octal escape sequence \240
with a single space. This is accepted in both NN4.7 and IE6,
as you can see by trying

        javascript: alert("#FF1111".replace(/\240/g, ' '));


In Mozilla, however, you get this error:

Error: back-reference exceeds number of capturing parentheses
Source File: javascript: alert("#FF1111".replace(/\240/g, ' '));
Line: 1



I spoke with Waldemar about this. The ECMA-262 Edition 3 standard
deprecated the use of octal escape sequences in regular expressions;
but we may want to revisit this for backward compatibility.

Filing this bug for a decision on that -
(Assignee)

Comment 1

15 years ago
I need an opinion on this, we can

- back out the ECMA deprecation 
- mark as WONTFIX
- reassign to Evangelism
Compatibility is king, perl set precedent.  I see why ECMA deprecated, but it
has a different purview (future specs and impls); meanwhile, in the real world,
the Mozilla implementation wants to gain market share.  Evangelizing may help a
bit, but with frontpage spewing octal regexp char codes, it seems to me it won't
win enough.  I say back out the ECMA deprecation.

This matters for js1.5, so for mozilla1.1 and mozilla1.0.1.

/be
Blocks: 149801
Keywords: js1.5, mozilla1.0.1, mozilla1.1
(Assignee)

Comment 3

15 years ago
Created attachment 91232 [details] [diff] [review]
Allow old style octal references.

I switched to emitting the error only if the '-strict' option is in effect,
does that seem like the right thing? Or should it be no more than a warning
even in that case?
Comment on attachment 91232 [details] [diff] [review]
Allow old style octal references.

In general, you shouldn't test JS_HAS_STRICT_OPTION -- instead, call 
js_ReportCompileErrorNumber(..., JSREPORT_STRICT | JSREPORT_WARNING...) and if
it returns false, someone set option.werror, so you should fail immediately
(propagate the error via false or null return code).  Otherwise, do the bad old
thing.

Here, I see why you want to preserve the if-else control flow, but I hope the
strict user gets a nice strict warning (not an error, as I thought users get
now given \377).  Where is the error report for overlarge backref?  Can it
change into a strict warning and be issued iff the else clause of the if
condition shown in this patch is taken?

/be

Updated

15 years ago
Blocks: 126627
(Assignee)

Comment 5

15 years ago
Created attachment 91540 [details] [diff] [review]
Revert to earlier octal handling, but with 'strict' imposing ECMA3 rules.

Here's another attempt. I think I need to go with the strict flag testing
because this is simply a parsing choice - either \007 is an octal value or a
NUL followed by '07', \12 is an octal value or a decimal back-reference.
Neither is an error - except that the back-reference may turn out to be so,
later, in which case it's a real error, not a warning.
(Reporter)

Comment 6

15 years ago
Testcase added to JS testsuite:

        mozilla/js/tests/ecma_3/RegExp/octal-001.js


I have filed bug 158159 against Rhino for this same issue -
Whiteboard: [Have filed bug 158159 against Rhino for same issue]
(Assignee)

Comment 7

15 years ago
Created attachment 91986 [details] [diff] [review]
Fixed tabs.

Fixed whitespace issues, previous patch was -wu, I'm not sure which is more
readable.
Attachment #91232 - Attachment is obsolete: true
(Reporter)

Comment 8

15 years ago
Another testcase for this added to JS testsuite:

        mozilla/js/tests/ecma_3/RegExp/octal-002.js
Rogerl, how about an up-to-date diff -u and a diff -wu (the latter for reviewing
purposes)?  Also, let's target this at 1.2alpha (timeless prodding works) and
I'll help get it reviewed. Thanks.

/be
(Assignee)

Comment 10

15 years ago
Created attachment 97538 [details] [diff] [review]
Updated patch
Attachment #91540 - Attachment is obsolete: true
Attachment #91986 - Attachment is obsolete: true
(Assignee)

Comment 11

15 years ago
Created attachment 97539 [details] [diff] [review]
Updated patch with -wu
I'll review and checkin before the deadline tonight.

/be
Keywords: mozilla1.1 → mozilla1.2
Target Milestone: --- → mozilla1.2alpha
I checked the fix into the trunk (I tweaked a multiline && expression in an if
condition to put the && consistently at the end of each line).

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

Comment 14

15 years ago
Marking Verified Fixed.

Both testcases above now pass, in the debug/optimized JS shell:

        mozilla/js/tests/ecma_3/RegExp/octal-001.js
        mozilla/js/tests/ecma_3/RegExp/octal-002.js
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.