1192038 - RegExp.prototype should be an ordinary object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Claude Pache]

Breaking change introduced in ES6. Don't know how web-compatible it is.

Comment 1

[Tom S. (please needinfo tschuster)]

Calling RegExp.prototype.toString now throws, because we invoke the "source" getter on a non-regexp.

Comment 2

[Tom S. (please needinfo tschuster)]

RegExp.prototype.lastIndex is going away as well with this change.

I am having problems getting this patch to work with JS-Xrays, because those use the cached proto-key and the clasp of the prototype to lookup the original methods. However the new plain RegExp.prototype has neither of those things. Should we add a special case for regexps?

Comment 3

[Bobby Holley (:bholley)]

I vaguely remember this issue being discussed before, but not sure if a patch ever got written. Or maybe I'm mixing it up with something else? Are there any other objects whose prototype is a blank object?

Seems like you should just change CreateRegExpPrototype to create a blank object (or ideally, inline that operation into the ClassSpec in vm/RegExpObject.cpp). Is there anything else that needs to be done?

Comment 4

[Tom S. (please needinfo tschuster)]

I don't want to work on this at the moment. Feel free to take this.

Comment 5

[Claude Pache]

FTR, RegExp.prototype.{global,ignoreCase,multiline,sticky,unicode}, as well as RegExp.prototype.source, has been tweaked in order to not throw on RegExp.prototype, b/c webbreaking.

Comment 6

[Tom S. (please needinfo tschuster)]

Next round.

Comment 7

[Tom S. (please needinfo tschuster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=752f48d8da06b1e716dc7ec69a12baccc9705a39

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: Use ordinary object for RegExp prototype

I am going to ask for review soon, but I just want to add some more JS tests.

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: Handle RegExp correctly in devtools

RegExp.prototype is not going to match that if statement anymore.

Comment 10

[Tom S. (please needinfo tschuster)]

Attachment: Update RegExp Xray code

I found that test in my patch queue from the last time I was debugging this, might as well land it. Not sure why we even bothered with 'lastIndex' on the prototype before.

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: Use ordinary object for RegExp prototype

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: Tests

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8814221 [details] [diff] [review]
Use ordinary object for RegExp prototype

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

Great, thanks :D

::: js/src/builtin/RegExp.cpp
@@ +589,1 @@
>  /* ES6 draft rev32 21.2.5.4. */

Can you update it to ES 2017 draft ?
Step 3.a is not in ES6.

::: js/src/vm/ObjectGroup.cpp
@@ +577,4 @@
>  
>          const JSAtomState& names = cx->names();
>  
> +        if (StandardProtoKeyOrNull(obj) == JSProto_RegExp)

RegExp.prototype doesn't have lastIndex property,
so we don't have to add type here.
Can you revert this line?

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8814222 [details] [diff] [review]
Tests

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

::: js/src/tests/ecma_6/RegExp/prototype.js
@@ +19,5 @@
> +assertEq(getter("source"), "(?:)");
> +assertEq(getter("sticky"), undefined);
> +assertEq(getter("unicode"), undefined);
> +
> +assertEq(t.toString(), "/(?:)/");

It would be nice to test that t.lastIndex is undefined

Comment 15

[Tom S. (please needinfo tschuster)]

Wow that was quick thanks!
(In reply to Tooru Fujisawa [:arai] from comment #13)
> Comment on attachment 8814221 [details] [diff] [review]
> Use ordinary object for RegExp prototype
> 
> Review of attachment 8814221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks :D
> 
> ::: js/src/builtin/RegExp.cpp
> @@ +589,1 @@
> >  /* ES6 draft rev32 21.2.5.4. */
> 
> Can you update it to ES 2017 draft ?
> Step 3.a is not in ES6.
> 
https://tc39.github.io/ecma262/#sec-get-regexp.prototype.global I see a Step 3.a.
> ::: js/src/vm/ObjectGroup.cpp
> @@ +577,4 @@
> >  
> >          const JSAtomState& names = cx->names();
> >  
> > +        if (StandardProtoKeyOrNull(obj) == JSProto_RegExp)
> 
> RegExp.prototype doesn't have lastIndex property,
> so we don't have to add type here.
> Can you revert this line?
While this isn't perfect, this change is required. In this case obj is actually the proto of the object we are creating.

Comment 16

[Tom S. (please needinfo tschuster)]

(In reply to Tooru Fujisawa [:arai] from comment #14)
> Comment on attachment 8814222 [details] [diff] [review]
> Tests
> 
> Review of attachment 8814222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/ecma_6/RegExp/prototype.js
> @@ +19,5 @@
> > +assertEq(getter("source"), "(?:)");
> > +assertEq(getter("sticky"), undefined);
> > +assertEq(getter("unicode"), undefined);
> > +
> > +assertEq(t.toString(), "/(?:)/");
> 
> It would be nice to test that t.lastIndex is undefined

The Reflect.ownKeys check already ensures that lastIndex doesn't exist.

Comment 17

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #15)
> > ::: js/src/builtin/RegExp.cpp
> > @@ +589,1 @@
> > >  /* ES6 draft rev32 21.2.5.4. */
> > 
> > Can you update it to ES 2017 draft ?
> > Step 3.a is not in ES6.
> > 
> https://tc39.github.io/ecma262/#sec-get-regexp.prototype.global I see a Step
> 3.a.

Yeah, it's ES 2017 draft :)


> > ::: js/src/vm/ObjectGroup.cpp
> > @@ +577,4 @@
> > >  
> > >          const JSAtomState& names = cx->names();
> > >  
> > > +        if (StandardProtoKeyOrNull(obj) == JSProto_RegExp)
> > 
> > RegExp.prototype doesn't have lastIndex property,
> > so we don't have to add type here.
> > Can you revert this line?
> While this isn't perfect, this change is required. In this case obj is
> actually the proto of the object we are creating.

I think I should forward the review of this part to jandem.

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8814221 [details] [diff] [review]
Use ordinary object for RegExp prototype

jandem, can you review the change to ObjectGroup::defaultNewGroup ?
I think I'm not capable of reviewing that part.

Comment 19

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #16)
> > It would be nice to test that t.lastIndex is undefined
> 
> The Reflect.ownKeys check already ensures that lastIndex doesn't exist.

I misunderstood the behavior of Reflect.ownKeys.
yeah, it surely checks it.

thanks!

Comment 20

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

Comment on attachment 8813880 [details] [diff] [review]
Handle RegExp correctly in devtools

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

I guess?

::: devtools/server/actors/object.js
@@ -1160,5 @@
>    RegExp: [function ({obj, hooks}, grip) {
> -    // Avoid having any special preview for the RegExp.prototype itself.
> -    if (!obj.proto || obj.proto.class != "RegExp") {
> -      return false;
> -    }

And removing this doesn't break any tests? What do we get if we evaluate `RegExp.prototype` in the console now?

Comment 21

[Tom S. (please needinfo tschuster)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #20)
> Comment on attachment 8813880 [details] [diff] [review]
> Handle RegExp correctly in devtools
> 
> Review of attachment 8813880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess?
> 
> ::: devtools/server/actors/object.js
> @@ -1160,5 @@
> >    RegExp: [function ({obj, hooks}, grip) {
> > -    // Avoid having any special preview for the RegExp.prototype itself.
> > -    if (!obj.proto || obj.proto.class != "RegExp") {
> > -      return false;
> > -    }
> 
> And removing this doesn't break any tests? What do we get if we evaluate
> `RegExp.prototype` in the console now?

RegExp gives me `Object { }`. I don't see any related test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57b0fd0f45b206fbc917e21ad493907ca2b33cd1&selectedJob=31763418
In my current Nightly without this patch RegExp.prototype just prints nothing btw.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8814221 [details] [diff] [review]
Use ordinary object for RegExp prototype

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

The ObjectGroup change LGTM for now. I've a patch to clean this code up.

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8813882 [details] [diff] [review]
Update RegExp Xray code

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

Tooru added lastIndex support in bug 1079919. Tooru, do we still need it?

r=me if not.

Comment 24

[Tooru Fujisawa [:arai]]

Comment on attachment 8813882 [details] [diff] [review]
Update RegExp Xray code

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

The code won't be required now.
Can you add an explicit testcase for |evalInSandbox("RegExp.prototype", ...).lastIndex === undefined|, like toString ?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d405339a6f
Use ordinary object for RegExp prototype. r=arai,jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/023c20b89e6a
Tests. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f955c5e7166
Handle RegExp correctly in devtools. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0356af174c
Update RegExp Xray code. r=bholley,arai

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/47d405339a6f
https://hg.mozilla.org/mozilla-central/rev/023c20b89e6a
https://hg.mozilla.org/mozilla-central/rev/3f955c5e7166
https://hg.mozilla.org/mozilla-central/rev/8f0356af174c