623435 - Rip out deprecated |RegExp.compile| builtin method

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

See bug 587288 comment 18 and bug 587288 comment 21. Small excerpt:

As a side note, the |RegExp.compile| builtin method is insidious! Swaps out the
guts of a regexp object, so you have to be really careful when holding
js::RegExp temporaries, a la bug 327170, because further code execution can
drop your refcount underneath you.

This is wanted 2.0 so we can have it removed on a nice big release.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

brendan, do you think it's too late in the release timeline to do this? If not, I can whip it up.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. Remove |RegExp.compile|

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. Clean regexp constructor.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. Clean regexp constructor.

Sorry, forgotten qref.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 506343 [details] [diff] [review]
0. Remove |RegExp.compile|

>diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp
>--- a/js/src/jsregexp.cpp
>+++ b/js/src/jsregexp.cpp
>@@ -659,92 +659,28 @@ EscapeNakedForwardSlashes(JSContext *cx,
>         if (!escaped)
>             cx->free(chars);
>         return escaped;
>     }
>     return unescaped;
> }
> 
> static bool
>-regexp_compile_sub_tail(JSContext *cx, JSObject *obj, Value *rval, JSString *str, uint32 flags = 0)
>+SwapRegExpInternals(JSContext *cx, JSObject *obj, Value *rval, JSString *str, uint32 flags = 0)
> {
>     flags |= cx->regExpStatics()->getFlags();
>     AlreadyIncRefed<RegExp> re = RegExp::create(cx, str, flags);
>     if (!re)
>         return false;
>     SwapObjectRegExp(cx, obj, re);
>     *rval = ObjectValue(*obj);
>     return true;
> }
> 
> static JSBool
>-regexp_compile_sub(JSContext *cx, JSObject *obj, uintN argc, Value *argv, Value *rval)
>-{
>-    if (!InstanceOf(cx, obj, &js_RegExpClass, argv))
>-        return false;
>-
>-    if (argc == 0)
>-        return regexp_compile_sub_tail(cx, obj, rval, cx->runtime->emptyString);
>-
>-    Value sourceValue = argv[0];
>-    if (sourceValue.isObject() && sourceValue.toObject().getClass() == &js_RegExpClass) {
>-        /*
>-         * If we get passed in a RegExp object we construct a new
>-         * RegExp that is a duplicate of it by re-compiling the
>-         * original source code. ECMA requires that it be an error
>-         * here if the flags are specified. (We must use the flags
>-         * from the original RegExp also).
>-         */
>-        JSObject &sourceObj = sourceValue.toObject();
>-        if (argc >= 2 && !argv[1].isUndefined()) {
>-            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NEWREGEXP_FLAGGED);
>-            return false;
>-        }
>-        RegExp *re = RegExp::extractFrom(&sourceObj);
>-        if (!re)
>-            return false;
>-        AlreadyIncRefed<RegExp> clone = RegExp::clone(cx, *re);
>-        if (!clone)
>-            return false;
>-        SwapObjectRegExp(cx, obj, clone);
>-        *rval = ObjectValue(*obj);
>-        return true;
>-    }
>-
>-    /* Coerce to string and compile. */
>-    JSString *sourceStr = js_ValueToString(cx, sourceValue);
>-    if (!sourceStr)
>-        return false;
>-    argv[0] = StringValue(sourceStr);
>-    uint32 flags = 0;
>-    if (argc > 1 && !argv[1].isUndefined()) {
>-        JSString *flagStr = js_ValueToString(cx, argv[1]);
>-        if (!flagStr)
>-            return false;
>-        argv[1] = StringValue(flagStr);
>-        if (!RegExp::parseFlags(cx, flagStr, flags))
>-            return false;
>-    }
>-
>-    JSString *escapedSourceStr = EscapeNakedForwardSlashes(cx, sourceStr);
>-    if (!escapedSourceStr)
>-        return false;
>-    argv[0] = StringValue(escapedSourceStr);
>-
>-    return regexp_compile_sub_tail(cx, obj, rval, escapedSourceStr, flags);
>-}
>-
>-static JSBool
>-regexp_compile(JSContext *cx, uintN argc, Value *vp)
>-{
>-    JSObject *obj = JS_THIS_OBJECT(cx, Jsvalify(vp));
>-    return obj && regexp_compile_sub(cx, obj, argc, vp + 2, vp);
>-}
>-
>-static JSBool
> regexp_exec_sub(JSContext *cx, JSObject *obj, uintN argc, Value *argv, JSBool test, Value *rval)
> {
>     if (!InstanceOf(cx, obj, &js_RegExpClass, argv))
>         return false;
> 
>     RegExp *re = RegExp::extractFrom(obj);
>     if (!re)
>         return true;
>@@ -833,49 +769,97 @@ js_regexp_test(JSContext *cx, uintN argc
>     return true;
> }
> 
> static JSFunctionSpec regexp_methods[] = {
> #if JS_HAS_TOSOURCE
>     JS_FN(js_toSource_str,  regexp_toString,    0,0),
> #endif
>     JS_FN(js_toString_str,  regexp_toString,    0,0),
>-    JS_FN("compile",        regexp_compile,     2,0),
>     JS_FN("exec",           js_regexp_exec,     1,0),
>     JS_FN("test",           js_regexp_test,     1,0),
>     JS_FS_END
> };
> 
> static JSBool
> regexp_construct(JSContext *cx, uintN argc, Value *vp)
> {
>     Value *argv = JS_ARGV(cx, vp);
>+    Value *rval = &JS_RVAL(cx, vp);
>+
>     if (!IsConstructing(vp)) {
>         /*
>          * If first arg is regexp and no flags are given, just return the arg.
>-         * (regexp_compile_sub detects the regexp + flags case and throws a
>-         * TypeError.)  See 15.10.3.1.
>+         * Otherwise, delegate to the standard constructor.
>+         * See ECMAv5 15.10.3.1.
>          */
>         if (argc >= 1 && argv[0].isObject() && argv[0].toObject().isRegExp() &&
>-            (argc == 1 || argv[1].isUndefined()))
>-        {
>+            (argc == 1 || argv[1].isUndefined())) {
>             *vp = argv[0];
>             return true;
>         }
>     }
> 
>     /* Otherwise, replace obj with a new RegExp object. */
>     JSObject *obj = NewBuiltinClassInstance(cx, &js_RegExpClass);
>     if (!obj)
>         return false;
> 
>-    return regexp_compile_sub(cx, obj, argc, argv, vp);
>+    if (argc == 0)
>+        return SwapRegExpInternals(cx, obj, rval, cx->runtime->emptyString);
>+
>+    Value sourceValue = argv[0];
>+    if (sourceValue.isObject() && sourceValue.toObject().getClass() == &js_RegExpClass) {
>+        /*
>+         * If we get passed in a RegExp object we construct a new
>+         * RegExp that is a duplicate of it by re-compiling the
>+         * original source code. ECMA requires that it be an error
>+         * here if the flags are specified. (We must use the flags
>+         * from the original RegExp also).
>+         */
>+        JSObject &sourceObj = sourceValue.toObject();
>+        if (argc >= 2 && !argv[1].isUndefined()) {
>+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NEWREGEXP_FLAGGED);
>+            return false;
>+        }
>+        RegExp *re = RegExp::extractFrom(&sourceObj);
>+        if (!re)
>+            return false;
>+        AlreadyIncRefed<RegExp> clone = RegExp::clone(cx, *re);
>+        if (!clone)
>+            return false;
>+        SwapObjectRegExp(cx, obj, clone);
>+        *rval = ObjectValue(*obj);
>+        return true;
>+    }
>+
>+    /* Coerce to string and compile. */
>+    JSString *sourceStr = js_ValueToString(cx, sourceValue);
>+    if (!sourceStr)
>+        return false;
>+    argv[0] = StringValue(sourceStr);
>+    uint32 flags = 0;
>+    if (argc > 1 && !argv[1].isUndefined()) {
>+        JSString *flagStr = js_ValueToString(cx, argv[1]);
>+        if (!flagStr)
>+            return false;
>+        argv[1] = StringValue(flagStr);
>+        if (!RegExp::parseFlags(cx, flagStr, flags))
>+            return false;
>+    }
>+
>+    JSString *escapedSourceStr = EscapeNakedForwardSlashes(cx, sourceStr);
>+    if (!escapedSourceStr)
>+        return false;
>+    argv[0] = StringValue(escapedSourceStr);
>+
>+    return SwapRegExpInternals(cx, obj, rval, escapedSourceStr, flags);
> }
> 
>-/* Similar to regexp_compile_sub_tail. */
>+/* Similar to SwapRegExpInternals. */
> static bool
> InitRegExpClassCompile(JSContext *cx, JSObject *obj)
> {
>     AlreadyIncRefed<RegExp> re = RegExp::create(cx, cx->runtime->emptyString, 0);
>     if (!re)
>         return false;
>     SwapObjectRegExp(cx, obj, re);
>     return true;
>diff --git a/js/src/tests/ecma_3/extensions/jstests.list b/js/src/tests/ecma_3/extensions/jstests.list
>--- a/js/src/tests/ecma_3/extensions/jstests.list
>+++ b/js/src/tests/ecma_3/extensions/jstests.list
>@@ -3,13 +3,13 @@ script 10.1.3-2.js
> script 7.9.1.js
> script regress-103087.js
> script regress-188206-01.js
> script regress-188206-02.js
> script regress-220367-002.js
> script regress-228087.js
> script regress-274152.js
> script regress-320854.js
>-script regress-327170.js
>+skip script regress-327170.js # RegExp.compile removed.
> script regress-368516.js
> script regress-385393-03.js
> script regress-429248.js
> script regress-430740.js
>diff --git a/js/src/tests/js1_2/regexp/jstests.list b/js/src/tests/js1_2/regexp/jstests.list
>--- a/js/src/tests/js1_2/regexp/jstests.list
>+++ b/js/src/tests/js1_2/regexp/jstests.list
>@@ -15,17 +15,17 @@ script RegExp_object.js
> script RegExp_rightContext.js
> script RegExp_rightContext_as_array.js
> script alphanumeric.js
> script asterisk.js
> script backslash.js
> script backspace.js
> script beginLine.js
> script character_class.js
>-script compile.js
>+skip script compile.js # RegExp.compile removed.
> script control_characters.js
> script digit.js
> script dot.js
> script endLine.js
> script everything.js
> script exec.js
> script flags.js
> script global.js

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 506345 [details] [diff] [review]
1. Clean regexp constructor.

> #define HANDLE_FLAG(__name)                                             \
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Pre-existing nits: house style puts \ in column 79...

>         JS_BEGIN_MACRO                                                  \
>-            if (flagsOut & (__name))                                    \

... and wants name_ not __name.

>+    /*
>+     * Per ECMAv5 15.10.4.1, we act on combinations of (pattern, flags):
>+     *  RegExp, undefined => flags := pattern.flags
>+     *  RegExp, _ => throw TypeError
>+     *  _ => pattern := ToString(pattern) if defined(pattern) else ''
>+     *       flags := ToString(flags) if defined(flags) else ''
>+     */

Love the pseudo-ML, but why := instead of =?

>+        if (!flagStr ||
>+            !RegExp::parseFlags(cx, flagStr, &flags))
>             return false;

Nit: (||) fits on one line, if it didn't you'd want to brace the consequent.

r=me, thanks.

/be

Comment 7

[Brendan Eich [:brendan]]

Filed comment 5 disaster against bugzilla as bug 628471.

/be

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #6)
> Love the pseudo-ML, but why := instead of =?

I sure yearn for pattern matching sometimes! Wanted to emphasize it was a rebinding of the original (pattern, flags) we were casing on.

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/a23e97df1b9f

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/4bc54aa1e566 (using js::Anchor)
http://hg.mozilla.org/tracemonkey/rev/4003aacbc675 (using JSStack-rooting)

The js::Anchor ends up with one two C-stack stores on non-GCC builds, so brendan asked to switch it back to use JSStack rooting.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a23e97df1b9f
http://hg.mozilla.org/mozilla-central/rev/4bc54aa1e566
http://hg.mozilla.org/mozilla-central/rev/4003aacbc675

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Why are dev docs needed for a method that should no longer exist? Removing until there's justification.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

Oh, I see the potential confusion. We had to back this out in bug 630284, see comment 12. Unfortunately, it lives on, but we shouldn't document it more. :-)