688069 - fix String.prototype.{replace,match,search,split} for transparently wrapped regexp args

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

This bug is spun off from bug 683361.  The simple solution is to add a toRegExp hook in ProxyHandler that returns a RegExp*.  This works since RegExp is stateless and ref-counted (or perhaps one day GC'd via the atoms compartment).  The only rub is that the RegExp's jitcode is allocated in a per-compartment allocator.  With single-threaded runtime (bug 650411), the allocator can be hoisted to the JSRuntime, so the issue goes away and the simple fix should work.

Comment 2

[Luke Wagner [:luke]]

It seems that the RegExp constructor and RegExp.prototype.compile also have .[[Class]] = "RegExp" tests.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm zeroLastIndex for match/replace and tidy up

Since we completely do not implement the correct setting of lastIndex in match/replace, it is goofy that we set lastIndex to zero.  Rather than just being weird, this patch removes the dangling call to zeroLastIndex which makes our behavior consistent with jsc and v8 by my testing.  This single change allows RegExpPair to be removed.  The patch also renames RegExpPrivate to RegExpShared and shortens a few names that used to mention Private.

Comment 4

[Luke Wagner [:luke]]

Attachment: use CallArgs consistently throughout jsstr and part of builtin/RegExp

Bring good typey-ness to jsstr.  It was already there in bits.  Also regularize setting of args.rval() to always happen at the end (depended upon by the debugger) and use assignment rather than .setX.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix and tests

This patch contains the actual fixes.

Comment 6

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

Comment on attachment 592340 [details] [diff] [review]
rm zeroLastIndex for match/replace and tidy up

Review of attachment 592340 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, I never liked RegExpPair!

::: js/src/vm/RegExpObject-inl.h
@@ +198,5 @@
>  
> +inline void
> +RegExpMatcher::init(NeedsIncRef<RegExpShared> shared)
> +{
> +    shared_.reset(shared);

JS_ASSERT(!shared_), like in the other init. If that doesn't hold for the "init"s then I think they're really "reset"s.

@@ +207,1 @@
>  {

Ditto.

::: js/src/vm/RegExpObject.cpp
@@ -78,5 @@
> -    RegExpObject *dummy = builder.build(priv);
> -    if (!dummy) {
> -        priv->decref(cx);
> -        return false;
> -    }

Schwoops.

::: js/src/vm/RegExpObject.h
@@ +165,5 @@
> +    RegExpShared *maybeShared() {
> +        return static_cast<RegExpShared *>(JSObject::getPrivate());
> +    }
> +
> +    RegExpShared *getShared(JSContext *cx) {

I honestly hate it when "get" means "get or create", because the verb "get" is so innocuous and trivial sounding. Is this really the standard we've converged on? :-(

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 592341 [details] [diff] [review]
use CallArgs consistently throughout jsstr and part of builtin/RegExp

>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
> str_localeCompare(JSContext *cx, uintN argc, Value *vp)
> {
>-    if (argc == 0) {
>-        vp->setInt32(0);
>+    if (args.length()) {
>+        args.rval() = Int32Value(0);

You want to add a test for that.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 592344 [details] [diff] [review]
fix and tests

>--- a/js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
>+++ b/js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
>-throw "done";
>+throw "done";kkk

?

>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>     bool
>     init(CallArgs args, bool convertVoid = false)
>             if (!shared)
>-                return false;
>+                return NULL;

Eh?

>--- a/js/src/jswrapper.h
>+++ b/js/src/jswrapper.h
>@@ -89,16 +89,17 @@ class JS_FRIEND_API(Wrapper) : public Pr
>     virtual bool call(JSContext *cx, JSObject *wrapper, uintN argc, Value *vp) MOZ_OVERRIDE;
>     virtual bool construct(JSContext *cx, JSObject *wrapper, uintN argc, Value *argv, Value *rval) MOZ_OVERRIDE;
>     virtual bool nativeCall(JSContext *cx, JSObject *wrapper, Class *clasp, Native native, CallArgs args) MOZ_OVERRIDE;
>     virtual bool hasInstance(JSContext *cx, JSObject *wrapper, const Value *vp, bool *bp) MOZ_OVERRIDE;
>     virtual JSType typeOf(JSContext *cx, JSObject *proxy) MOZ_OVERRIDE;
>     virtual bool objectClassIs(JSObject *obj, ESClassValue classValue, JSContext *cx) MOZ_OVERRIDE;
>     virtual JSString *obj_toString(JSContext *cx, JSObject *wrapper) MOZ_OVERRIDE;
>     virtual JSString *fun_toString(JSContext *cx, JSObject *wrapper, uintN indent) MOZ_OVERRIDE;
>+    virtual RegExpShared *regexp_toShared(JSContext *cx, JSObject *proxy);

MOZ_OVERRIDE?

> class JS_FRIEND_API(SecurityWrapper) : public Base
> {
>     virtual bool objectClassIs(JSObject *obj, ESClassValue classValue, JSContext *cx) MOZ_OVERRIDE;
>+    virtual RegExpShared *regexp_toShared(JSContext *cx, JSObject *proxy);

And here?

Comment 9

[Luke Wagner [:luke]]

(In reply to Chris Leary [:cdleary] from comment #6)
> I honestly hate it when "get" means "get or create", because the verb "get"
> is so innocuous and trivial sounding. Is this really the standard we've
> converged on? :-(

"getX" implies a fallible operation (with some notable offenders like getProto).  When it is infallible, we just name it "x".  The "OrCreate" just seems verbose since the caller doesn't want to think about where it comes from, they just want to get it.

Comment 10

[Luke Wagner [:luke]]

Attachment: use CallArgs consistently throughout jsstr and part of builtin/RegExp (v2)

Thanks Ms2ger.  Rest assured, the browser tests caught that bug (specifically an xpconnect localCompare test).

Comment 11

[Luke Wagner [:luke]]

Green on try.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 592757 [details] [diff] [review]
use CallArgs consistently throughout jsstr and part of builtin/RegExp (v2)

Review of attachment 592757 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +110,1 @@
>      }

This if-else is equivalent to:

JSString *str = ToString(cx, arg);
if (!str)
    return NULL;
arg = StringValue(str);

which would be much cleaner.

@@ +2565,3 @@
>              if (!sep)
>                  return false;
> +            args[0].setString(sep);

This bit of change here looks wrong.  Don't you want

  sepstr = ArgToRootedString(...);
  if (!sepstr)
    return false;
  sepstr = sepstr->ensureLinear(cx);
  ...

@@ +2633,5 @@
>              str = cx->runtime->emptyString;
>              goto out;
>          }
>          if (begin < 0) {
>              begin += length; /* length + INT_MIN will always be less then 0 */

"than" while you're in the vicinity.

Comment 13

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

Comment on attachment 592344 [details] [diff] [review]
fix and tests

Review of attachment 592344 [details] [diff] [review]:
-----------------------------------------------------------------

Nice refresher on proxies and wrappers, thanks for r?ing me!

::: js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
@@ +168,5 @@
>  test("new Date()", function(d) justDontThrow(Date.prototype.toSource.call(d)));
>  test("new Date()", function(d) justDontThrow(Date.prototype.toString.call(d)));
>  test("new Date()", function(d) justDontThrow(Date.prototype.valueOf.call(d)));
>  
> +throw "done";kkk

Stray chars.

::: js/src/jsobjinlines.h
@@ +1850,5 @@
>      return false;
>  }
>  
> +inline bool
> +IsObjectWithClass(const Value &v, ESClassValue classValue, JSContext *cx)

Heh, for some reason this phrasing makes me think, IsObjectWithChild?

::: js/src/jsstr.cpp
@@ +1405,5 @@
>      /* init must succeed in order to call tryFlatMatch or normalizeRegExp. */
>      bool
>      init(CallArgs args, bool convertVoid = false)
>      {
> +        if (args.length() != 0 && IsObjectWithClass(args[0], ESClass_RegExp, cx)) {

Random nit: no args.empty()?

Comment 14

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #12)
> This if-else is equivalent to:
> 
> JSString *str = ToString(cx, arg);
> if (!str)
>     return NULL;
> arg = StringValue(str);
> 
> which would be much cleaner.

I was worried that this introduced an extra value boxing/store compared to the original path.  But might as well just do it; the function seems somewhat hot (indexOf calls it, e.g.), but probably it doesn't matter.

Comment 15

[Luke Wagner [:luke]]

(In reply to Chris Leary [:cdleary] from comment #13)
> Random nit: no args.empty()?

That's a good idea.  I'm on the fence, though: 'vec.empty' is quite readable, but is 'args.empty' ?  I'm inclined to yes, since args is very vector-like.  What do you think Waldo?  We can do this as a follow-up rename-all-args.length()==0-to-args.empty() patch.

Comment 16

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54cda90d5f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d1483691674
https://hg.mozilla.org/integration/mozilla-inbound/rev/304182354c92

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/54cda90d5f3e
https://hg.mozilla.org/mozilla-central/rev/4d1483691674
https://hg.mozilla.org/mozilla-central/rev/304182354c92

Comment 18

[Jeff Walden [:Waldo]]

I dunno.  It's reasonable to think of a vector as empty.  On the other hand, I don't think of "empty" arguments, I think of "no" arguments.  So I don't think the empty() name matches the concept here.

(Incidentally, I seem to remember a conclusion on IRC that "empty" wasn't a good name, because of its ambiguity between verb/adjective meanings, and that we should use "isEmpty" instead.  But I guess no one's been bothered enough to file the bug and make the change.)