607799 - typeof(regexp from sandbox) is "function"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Drew Willcoxon :adw]

Creating a regexp in a sandbox and calling typeof() on it outside the sandbox results in "function".

e.g.:

  var sp = Components.classes["@mozilla.org/systemprincipal;1"].
           createInstance(Components.interfaces.nsIPrincipal);
  var s = Components.utils.Sandbox(sp);
  Components.utils.evalInSandbox("re = /foo/;", s);
  typeof(s.re); /* "function" */

This breaks some Jetpack unit tests.

Comment 1

[Andreas Gal :gal]

Regexp objects are
Callable and hence use a function proxy. Not sure
This is wrong.

Comment 2

[Drew Willcoxon :adw]

Well, not only is this a regression from previous behavior, but when everything's done entirely in the sandbox or if sandboxes are not involved at all, typeof(regexp) is "object".

Comment 3

[Jeff Walden [:Waldo]]

Andreas, it's a bug, see the code in js_TypeOf which special-cases regular expressions, and the comment next to it.

Comment 4

[Andreas Gal :gal]

What does the spec say about regexp? Is it callable in ES5?

Comment 5

[Andreas Gal :gal]

We have to figure this out for 2.0 but not for b7.

Comment 6

[Jeff Walden [:Waldo]]

Nothing is stated about whether regular expressions have a [[Call]] internal property or not, so the answer is "either way", but we did explicitly switch from their being "function" to "object" at one point, because it was a constant developer bugaboo.  We plan to remove regular expression callability at some point; see bug 582717.

Comment 7

[Robert Sayre]

I marked this as b8+. If this actually breaks a Jetpack test in way that's more serious than (probably superfluous) type checking, please speak up.

Comment 8

[Drew Willcoxon :adw]

(In reply to comment #7)
> I marked this as b8+. If this actually breaks a Jetpack test in way that's more
> serious than (probably superfluous) type checking, please speak up.

Thanks Rob.  It does not.  If it turns out I'm wrong I'll comment here again.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

Andreas, I was also thinking of:

    if (obj->isWrapper())
        return JS_TypeOfValue(cx, Jsvalify(obj->getProxyPrivate()));
    return JSTYPE_FUNCTION;

do you have a preference?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Well, actually
        return TypeOfValue(cx, obj->getProxyPrivate());

Comment 11

[Brendan Eich [:brendan]]

We had to back off from making typeof /hi/ === "function" a while ago. See bug 61911 and bug 305959.

The whole mess of callable regexps is an extension that violatles ES3's complete list of built-in classes that implement [[Call]], but last we checked, some web or XUL content counted on it, and other browsers' engines cloned callable regexps based on us (they all say typeof /hi/ === "object" as we do).

Plan is to take coordinated post-Firefox-4 action to remove callability from regexps in SpiderMonkey, JSC, and V8.

/be

Comment 12

[Andreas Gal :gal]

What does IE do here?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> What does IE do here?

Why do you ask? It's not relevant at all. We're not changing typeof /hi/ to be "function" (IE says "object"). We're not yet removing callability (IE regexps are not callable, but again this is not relevant).

/be

Comment 14

[Andreas Gal :gal]

I was just wondering. If its not callable in IE either, I would advocate for revoking call-ability right now. Otherwise this is a death match where everyone stares down everyone else and nobody wants to do the first step. If we are serious about making regexp not-callable, we should do it before we add yet another major market share that does support callable regexp. I am happy to do the regexp patch for b8, or make proxies transparent for b8. Either one is not difficult, and I am not super attached to either.

Comment 15

[Brendan Eich [:brendan]]

This is a disorderly conversation. We already decided based on content and other engines to defer removing callability in Firefox 4. Firefox 4 is late. You are now overtly trying to make it later, by adding work we'd have to do and betas we would have to ship to (try to -- we don't own it all) find and fix all of that content.

This is a recent regression due to wrappers, meanwhile. It has a small patch. Fix it and get on with stabilizing Firefox 4 so it is not any later!

/be

Comment 16

[Andreas Gal :gal]

Attachment: patch

This is a cleaner fix IMO. But the quick hack is fine too, whatever you prefer.

Comment 17

[Andreas Gal :gal]

Attachment: patch

No security check for typeOf and use TypeOfValue so we don't have to switch compartments (Blake made me do it).

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 486735 [details] [diff] [review]
patch

r=me with a comment on why we don't enter/leave. Also, add a test.

Comment 19

[Andreas Gal :gal]

Attachment: patch

Comment 20

[Andreas Gal :gal]

Comment on attachment 486815 [details] [diff] [review]
patch

This patch disallows wrapping XML, which is impossible due to its GetMethod weirdness.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 486815 [details] [diff] [review]
patch

This seems to have hasInstance changes mixed in, and the test is missing.

/be

Comment 22

[Andreas Gal :gal]

There are no changes to hasInstance, I just added the missing static hop that guards against re-entry. I will add a test. Can you review in the meantime, this is hurting jetpack.

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> There are no changes to hasInstance, I just added the missing static hop that
> guards against re-entry. I will add a test. Can you review in the meantime,
> this is hurting jetpack.

Ok. Where's the test?

/be

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 486815 [details] [diff] [review]
patch

r=me with a reftest landed now :-/.

/be

Comment 25

[Andreas Gal :gal]

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

Comment 26

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f52f5d7feb29