1079919 - Make RegExp.prototype.toString to be a generic function

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[ziyunfei]

In ES6, RegExp.prototype.toString is a generic function:

>RegExp.prototype.toString.call({source:"foo", sticky:true})
"/foo/y"
>RegExp.prototype.toString.call({})
"/undefined/"

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Make RegExp.prototype.toString to be a generic function.

Spec seems to be changed since this bug is reported, and now it uses `flags` instead of each flags accessors.

Most part could be implemented straightforward.
(I don't think `IsObject` is the best name for the function which checks the `this` value is an object, and report error if not. If you know better name, please tell me :)

Then, in toolkit/devtools/server/actors/script.js, originally it calls `RegExp.prototype.toString` with wrapped object, and it results "/undefined/undefined" because we cannot get `source` nor `flags` property from it. So I modified it to call Cu.waiveXrays/Cu.unwaiveXrays. Is it a correct way?

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46fa4cb2b362

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8562323 [details] [diff] [review]
Make RegExp.prototype.toString to be a generic function.

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

I think it'd be much better to self-host this function. The implementation would boil down to:

var O = ToObject(this);
var pattern = O.source;
var flags = O.flags;
return '/' + pattern + '/' + flags;

(Comments omitted, but that's about it.)

The `ToObject` intrinsic takes care of throwing an exception if `this` isn't an object, so you don't have to deal with that. (Note that the same is true in native code: ToObject in C++ also throws the correct exception if the conversion fails.)

The only thing making self-hosting slightly more onerous in this case is that we don't yet have any self-hosted RegExp methods. You can just create a builtin/RegExp.js file with just the license header and your RegExpToSource function and add it to the list of files in Makefile.in (at line 326). Finally, change the regexp_methods entries for toString and toSource to target the new self-hosted function. Look for examples in array_methods if anything's unclear. Or at the documentation: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

There are same problem as self-hosting flags (bug 1120170), bug 1120168.
I'll try to fix it first.

Comment 4

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> Thank you for reviewing :D
> 
> There are same problem as self-hosting flags (bug 1120170), bug 1120168.
> I'll try to fix it first.

Ooh, yes. Great analysis in bug 1120168. Let me know if you get stuck there and I'll help out.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Make RegExp.prototype.toString to be a generic function.

Implemented as self-hosted function. RegExp.js was already added by ziyunfei's patch in bug 1120170 :)

The issue in toolkit/devtools/server/actors/script.js remains, and same solution is applicable.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=822867b1bfb8

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8571347 [details] [diff] [review]
Make RegExp.prototype.toString to be a generic function.

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

I won't be able to review this at least until the week after next, perhaps even later. I know it's a really small change, but I wasn't able to convince myself of fully understanding it, so punting for now. Waldo, feel free to bounce it back to me if you think that it'd take you even longer to get to it than me.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8571347 [details] [diff] [review]
Make RegExp.prototype.toString to be a generic function.

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

I can't evaluate the script.js changes, except that they fill me with fear.  :-)  Over to jimb for that, maybe?.  Rest is good, r=me on it.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8571347 [details] [diff] [review]
Make RegExp.prototype.toString to be a generic function.

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

Stealing review from :jimb because he has a large review queue and he hasn't touched the devtools server code in a long while.

::: toolkit/devtools/server/actors/script.js
@@ +3786,5 @@
>        return false;
>      }
>  
> +    let raw = obj.unsafeDereference();
> +    let str = RegExp.prototype.toString.call(Cu.waiveXrays(raw));

Is this still safe now that the self hosted function is just accessing the properties directly? Do self hosted functions get some kind of xray equivalent automatically? Why do we need to waive here? Because xrays don't understand .source and .flags?

What happens if content does this:

    Object.defineProperty(RegExp.prototype, "flags", {
      get: () => { window.secretelyMuckWithStuff(); throw Error("lol"); }
    })

It seems to me that we will no longer be able to stringify RegExps in the devtools with this patch. Furthermore, it can unexpectedly run debuggee code when we have a contract with the debugger's user that we will never do that when paused unless they explicitly ask us to do it! Imagine how frustrating it is for your environment to change underneath you while you are paused in a debugger!

We could wrap this in a try/catch, but that doesn't solve the debuggee-code-possibly-running problem.

If that's not the case, and I'm just misunderstanding, then we need (a) a test for this behavior and (b) a nice comment explaining to future readers why this waiveXrays call is safe.

Otherwise, if this can run debuggee code, then I think the proper solution is to introduce .source and .flags to the xray machinery.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support RegExp prototype in Xray.

Thank you for reviewing!

I added support for RegExp.prototype in Xray (as Part 1), and the test for it with RegExp.prototype.toString change (as Part 3). previous patch is turned into Part 2, with removing the chnage in script.js.
Is this a correct way?

(In reply to Nick Fitzgerald [:fitzgen] from comment #8)
> Is this still safe now that the self hosted function is just accessing the
> properties directly?

About the "debuggee-code-possibly-running problem", yes, it does exist, with previous patch.
Sorry I didn't realize that's the problem.

> Do self hosted functions get some kind of xray
> equivalent automatically?

I don't think so.

> Why do we need to waive here? Because xrays don't
> understand .source and .flags?

Yes, I thought that's the solution. but it was wrong. Adding RegExp support in Xray should be better.

> What happens if content does this:
> 
>     Object.defineProperty(RegExp.prototype, "flags", {
>       get: () => { window.secretelyMuckWithStuff(); throw Error("lol"); }
>     })
> 
> It seems to me that we will no longer be able to stringify RegExps in the
> devtools with this patch. Furthermore, it can unexpectedly run debuggee code
> when we have a contract with the debugger's user that we will never do that
> when paused unless they explicitly ask us to do it! Imagine how frustrating
> it is for your environment to change underneath you while you are paused in
> a debugger!
> 
> We could wrap this in a try/catch, but that doesn't solve the
> debuggee-code-possibly-running problem.

With this patch, debuggee-code won't be called any more.


Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=490fcff0afc5

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add test for RegExp.prototype properties in webconsole.

Test for modified RegExp.prototype in content script.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8577860 [details] [diff] [review]
Part 1: Support RegExp prototype in Xray.

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

Thanks, this looks great! This patch probably needs review from either bholley or jorendorff.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8577862 [details] [diff] [review]
Part 3: Add test for RegExp.prototype properties in webconsole.

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

This is great! Thanks!

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8577860 [details] [diff] [review]
Part 1: Support RegExp prototype in Xray.

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Comment on attachment 8577860 [details] [diff] [review]
> Part 1: Support RegExp prototype in Xray.
> 
> Review of attachment 8577860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks great! This patch probably needs review from either
> bholley or jorendorff.

Thank you!

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8577860 [details] [diff] [review]
Part 1: Support RegExp prototype in Xray.

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

Impressive patch - thanks for the great work!

Can you explain why exactly we can't give RegExp a ClassSpec? Setting it up the way it is required a lot of careful design and refactoring, and so I'm not really excited about the hack in js::GetPrototypeFunctionAndProperties.

This will need tests in js/xpconnect/tests/chrome/test_xrayToJS.xul.

Comment 15

[Tooru Fujisawa [:arai]]

(In reply to Bobby Holley (:bholley) from comment #14)
> Comment on attachment 8577860 [details] [diff] [review]
> Part 1: Support RegExp prototype in Xray.
> 
> Review of attachment 8577860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Impressive patch - thanks for the great work!
> 
> Can you explain why exactly we can't give RegExp a ClassSpec? Setting it up
> the way it is required a lot of careful design and refactoring, and so I'm
> not really excited about the hack in js::GetPrototypeFunctionAndProperties.

Oops, I've just been confused. What I need to do is just declaring relevant functions in RegExp.h and use it from JS class definition in RegExpObject.cpp, noticed while writing the reply :)
I'll post updated patch shortly.

> This will need tests in js/xpconnect/tests/chrome/test_xrayToJS.xul.

Okay, thanks!

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add RegExp ClassSpec.

Part 0 adds ClassSpec to RegExp.
Also, added constructorProperties to ClassSpec to define constructor properties in RegExp. This could also be done in finishInit function, instead of adding nre member to ClassSpec. which is better?

Green on try run, except hg.m.o trouble
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=77af5e6e539a

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Support RegExp in Xray.

Part 1 adds Xray support for RegExp. Almost part could be done by just adding JSProto_RegExp in IsJSXraySupported.
One exception is lastIndex, which is not listed in prototypeProperties, but defined both in prototype and instance (am I correct?)

RegExp.prototype.flags is self-hosted accessor, so I removed comment and assertion in JSXrayTraits::enumerateNames. Is there any other test should be added in test_XrayToJS.xul for it ?

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Add Xray test for RegExp.prototype.toString.

Part 4 adds test for RegExp.prototype.toString/toSource in test_XrayToJS.xul, almost same way as Part 1.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8578994 [details] [diff] [review]
Part 0: Add RegExp ClassSpec.

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

Nice work! r=bholley with that fixed.

::: js/src/builtin/RegExp.cpp
@@ +594,5 @@
>      RegExpObjectBuilder builder(cx, proto);
>      if (!builder.build(empty, RegExpFlag(0)))
>          return nullptr;
>  
> +    if (!DefinePropertiesAndFunctions(cx, proto, js::regexp_properties, js::regexp_methods))

This already happens in GlobalObject::resolveConstructor for ClassSpec classes - please remove it.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8578997 [details] [diff] [review]
Part 1: Support RegExp in Xray.

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

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +588,5 @@
>  
> +  function testRegExp() {
> +    testXray('RegExp', new iwin.RegExp('foo'), new iwin.RegExp());
> +
> +    // Test the self-hosted flags.

I'd say "test the self-hosted |flags| property".

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +429,5 @@
> +            if (id == GetRTIdByIndex(cx, XPCJSRuntime::IDX_LASTINDEX)) {
> +                JSAutoCompartment ac(cx, target);
> +                if (!getOwnPropertyFromTargetIfSafe(cx, target, wrapper, id, desc))
> +                    return false;
> +                return true;

just make this:

return getOwnPropertyFromTargetIfSafe(...);

@@ +740,5 @@
>          return false;
>  
> +    // For RegExp protoypes, add the 'lastIndex' property.
> +    if (key == JSProto_RegExp && !props.append(GetRTIdByIndex(cx, XPCJSRuntime::IDX_LASTINDEX)))
> +        return false;

Technically to make this lastIndex handling complete we need to also do the right thing in defineProperty as well. But honestly I suspect it's not worth adding code for.

@@ -740,5 @@
> -        // Xrayable class, so we can't test it. Assert against it to make sure that we get
> -        // test coverage in test_XrayToJS.xul when the time comes.
> -        MOZ_ASSERT(!ps->isSelfHosted(),
> -                   "Self-hosted accessor added to Xrayable class - ping the XPConnect "
> -                   "module owner about adding test coverage");

Nice - Thanks for the awesome test coverage. :-)

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8578998 [details] [diff] [review]
Part 4: Add Xray test for RegExp.prototype.toString.

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

This is all really top-notch work - I'm extremely impressed that you just jumped into the Xray code (scary stuff) and produced more or less perfect patches. Hats off to you, and a Mozillians vouch as well. :-)

Comment 22

[Tooru Fujisawa [:arai]]

Thank you all for reviewing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/f82a7f0db599
https://hg.mozilla.org/integration/mozilla-inbound/rev/da88e17551a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/de01752cf8ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/113dd9e05803
https://hg.mozilla.org/integration/mozilla-inbound/rev/a918ec552801

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f82a7f0db599
https://hg.mozilla.org/mozilla-central/rev/da88e17551a8
https://hg.mozilla.org/mozilla-central/rev/de01752cf8ae
https://hg.mozilla.org/mozilla-central/rev/113dd9e05803
https://hg.mozilla.org/mozilla-central/rev/a918ec552801

Comment 24

[Tooru Fujisawa [:arai]]

Updated following documentation
  https://developer.mozilla.org/en-US/Firefox/Releases/39
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/toString

Comment 25

[Florian Scholz (Open Web Docs)]

Thanks for the doc updates, :arai!