Bug 155922 – Update treatment of invoking RegExp.prototype methods on RegExp.prototype.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

In r198698, for the purpose of enabling feature testing, I changed the RegExp.prototype flag property getters to always return undefined instead of throwing when "this" is not a RegExp object.  However, for the purpose of feature testing, we only need to apply this change for the cases where the getters are called on the RegExp.prototype itself.  If called with any other "this" values that is not a RegExp object, they should throw a TypeError as the ES6 spec expects.

[Mark Lam]

Created attachment 274977 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 274977 [details]
proposed patch.

r=me

[Geoffrey Garen]

Comment on attachment 274977 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=274977&action=review

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:249
> +        if (thisValue == exec->lexicalGlobalObject()->regExpPrototype())
> +            return JSValue::encode(jsUndefined());

Actually, you should check inherits(RegExpPrototype::info()) instead. There's no reason to tie this to a global object.

[Mark Lam]

I'm going to hold off on implementing this for now.  This issue is being discussed at https://github.com/tc39/ecma262/issues/262?, and there doesn't appear to be an agreed on solution yet.

[Mark Lam]

For the record, Chrome is only special casing RegExp.prototype and returning undefined for it (see https://github.com/v8/v8/blob/master/src/js/regexp.js#L1091).  For everything else, it throws the ES6 spec'ed TypeError.

[Mark Lam]

From a recent TC39 meeting, we were told that "the committee also agreed to Daniel Ehrenberg’s pull request #262 (https://github.com/tc39/ecma262/issues/262), slides.  Daniel will write up the details.  The end result is that we will no longer throw for “RegExp like” accesses to RegExp.prototype, e.g. RegExp.prototype.sticky.  In the case of individual flags, we return undefined.  For .flags, .source and .toString(), we return what we do now, “”, "(?:)" and /(?:)/“ respectively.  RegExp.prototype however IS NOT an instance of RegExp."

[Radar WebKit Bug Importer]

<rdar://problem/25717387>

[Mark Lam]

Created attachment 276403 [details]
proposed patch.

[Mark Lam]

Created attachment 276404 [details]
proposed patch: rebased to ToT.

[Keith Miller]

Comment on attachment 276404 [details]
proposed patch: rebased to ToT.

r=me.

[Keith Miller]

Did you run benchmarks on this?

[Mark Lam]

(In reply to comment #11)
> Did you run benchmarks on this?

No.  But the only changes in this patch are on error paths.  If these results in a perf regression, then we have a serious problem with code cache lines.

If it's ok with you, I'll put UNLIKELY() on the "if (!thisValue.inherits(RegExpObject::info()))" checks, and just monitor the perf bots for regressions.

[Keith Miller]

(In reply to comment #12)
> (In reply to comment #11)
> > Did you run benchmarks on this?
> 
> No.  But the only changes in this patch are on error paths.  If these
> results in a perf regression, then we have a serious problem with code cache
> lines.
> 
> If it's ok with you, I'll put UNLIKELY() on the "if
> (!thisValue.inherits(RegExpObject::info()))" checks, and just monitor the
> perf bots for regressions.

Seems reasonable.

[Mark Lam]

Thanks.  Landed in r199545: <http://trac.webkit.org/r199545>.