Closed Bug 607799 Opened 10 years ago Closed 10 years ago

typeof(regexp from sandbox) is "function"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: adw, Assigned: gal)

References

Details

(Whiteboard: [compartments], fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

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.
Regexp objects are
Callable and hence use a function proxy. Not sure
This is wrong.
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".
Andreas, it's a bug, see the code in js_TypeOf which special-cases regular expressions, and the comment next to it.
What does the spec say about regexp? Is it callable in ES5?
We have to figure this out for 2.0 but not for b7.
blocking2.0: --- → ?
blocking2.0: ? → beta8+
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.
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.
(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.
Attached patch Possible fix (obsolete) — Splinter Review
Andreas, I was also thinking of:

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

do you have a preference?
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #486533 - Flags: review?(gal)
Well, actually
        return TypeOfValue(cx, obj->getProxyPrivate());
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
What does IE do here?
(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
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.
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
Attachment #486533 - Flags: review?(gal) → review+
Attached patch patch (obsolete) — Splinter Review
This is a cleaner fix IMO. But the quick hack is fine too, whatever you prefer.
Attached patch patch (obsolete) — Splinter Review
No security check for typeOf and use TypeOfValue so we don't have to switch compartments (Blake made me do it).
Assignee: mrbkap → gal
Attachment #486565 - Attachment is obsolete: true
Attachment #486735 - Flags: review?(mrbkap)
Attachment #486533 - Attachment is obsolete: true
Comment on attachment 486735 [details] [diff] [review]
patch

r=me with a comment on why we don't enter/leave. Also, add a test.
Attachment #486735 - Flags: review?(mrbkap) → review+
Attached patch patchSplinter Review
Attachment #486735 - Attachment is obsolete: true
Comment on attachment 486815 [details] [diff] [review]
patch

This patch disallows wrapping XML, which is impossible due to its GetMethod weirdness.
Attachment #486815 - Flags: review?
Attachment #486815 - Flags: review? → review?(brendan)
Comment on attachment 486815 [details] [diff] [review]
patch

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

/be
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.
(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 on attachment 486815 [details] [diff] [review]
patch

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

/be
Attachment #486815 - Flags: review?(brendan) → review+
Whiteboard: [compartments]
http://hg.mozilla.org/tracemonkey/rev/f52f5d7feb29
Whiteboard: [compartments] → [compartments], fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/f52f5d7feb29
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 616711
You need to log in before you can comment on or make changes to this bug.