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

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.

Updated

6 years ago
Version: unspecified → Trunk
(Assignee)

Updated

5 years ago
Duplicate of this bug: 720973
(Assignee)

Comment 2

5 years ago
It seems that the RegExp constructor and RegExp.prototype.compile also have .[[Class]] = "RegExp" tests.
(Assignee)

Comment 3

5 years ago
Created attachment 592340 [details] [diff] [review]
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.
Attachment #592340 - Flags: review?(christopher.leary)
(Assignee)

Comment 4

5 years ago
Created attachment 592341 [details] [diff] [review]
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.
Attachment #592341 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #592341 - Attachment description: use CallArgs consistently through jsstr and part of builtin/RegExp → use CallArgs consistently throughout jsstr and part of builtin/RegExp
(Assignee)

Comment 5

5 years ago
Created attachment 592344 [details] [diff] [review]
fix and tests

This patch contains the actual fixes.
Attachment #592344 - Flags: review?(christopher.leary)
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? :-(
Attachment #592340 - Flags: review?(christopher.leary) → review+
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 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?
(Assignee)

Comment 9

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

Comment 10

5 years ago
Created attachment 592757 [details] [diff] [review]
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).
Attachment #592757 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #592341 - Attachment is obsolete: true
Attachment #592341 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 11

5 years ago
Green on try.
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.
Attachment #592757 - Flags: review?(jwalden+bmo) → review+
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()?
Attachment #592344 - Flags: review?(christopher.leary) → review+
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

5 years ago
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
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/54cda90d5f3e
https://hg.mozilla.org/mozilla-central/rev/4d1483691674
https://hg.mozilla.org/mozilla-central/rev/304182354c92
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.)
Depends on: 748212
You need to log in before you can comment on or make changes to this bug.