Closed Bug 1192038 Opened 4 years ago Closed 3 years ago

RegExp.prototype should be an ordinary object

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: claude.pache, Assigned: evilpie)

References

(Blocks 2 open bugs, )

Details

Attachments

(4 files, 1 obsolete file)

Breaking change introduced in ES6. Don't know how web-compatible it is.
Blocks: 797686, 1109577
Assignee: nobody → evilpies
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Calling RegExp.prototype.toString now throws, because we invoke the "source" getter on a non-regexp.
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?
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley)
I don't want to work on this at the moment. Feel free to take this.
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
Blocks: es6
Assignee: nobody → jorendorff
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.
Blocks: test262
No longer blocks: es6
Next round.
Assignee: jorendorff → evilpies
I am going to ask for review soon, but I just want to add some more JS tests.
RegExp.prototype is not going to match that if statement anymore.
Attachment #8813880 - Flags: review?(nfitzgerald)
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.
Attachment #8813882 - Flags: review?(bobbyholley)
Attachment #8814221 - Flags: review?(arai.unmht)
Attached patch TestsSplinter Review
Attachment #8814222 - Flags: review?(arai.unmht)
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?
Attachment #8814221 - Flags: review?(arai.unmht) → review+
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
Attachment #8814222 - Flags: review?(arai.unmht) → review+
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.
(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.
(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 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.
Attachment #8814221 - Flags: review?(jdemooij)
(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 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?
Attachment #8813880 - Flags: review?(nfitzgerald) → review+
(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 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.
Attachment #8814221 - Flags: review?(jdemooij) → review+
Attachment #8813879 - Attachment is obsolete: true
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.
Attachment #8813882 - Flags: review?(bobbyholley)
Attachment #8813882 - Flags: review?(arai.unmht)
Attachment #8813882 - Flags: review+
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 ?
Attachment #8813882 - Flags: review?(arai.unmht) → review+
Depends on: 1323868
You need to log in before you can comment on or make changes to this bug.