Bug 155711

<rdar://problem/25263672>

Created attachment 274668 [details] Patch

Created attachment 274678 [details] Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec.

Committed r198554: <http://trac.webkit.org/changeset/198554>

Comment on attachment 274678 [details] Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec. View in context: https://bugs.webkit.org/attachment.cgi?id=274678&action=review r=me with the noted suggestions. I think it's also worth adding a test or two that uses Proxy to verify that operations are performed only the number of needed times, in the right order, etc. > Source/JavaScriptCore/builtins/RegExpPrototype.js:59 > + if (regexp.exec !== @RegExp.prototype.@exec && typeof(regexp.exec) === "function") { Style: I think our "typeof" style is usually this: typeof regex.exec === "function" Also, I'm not sure this is correct. You're calling "get" twice here. That is an observable operation. It looks like RegExpExec only does this get once in the spec. > Source/JavaScriptCore/builtins/StringPrototype.js:40 > + var matcher = regexp[@symbolMatch]; This isn't correct w.r.t to the spec. It looks like the spec calls GetMethod here. GetMethod will throw if the result isn't undefined or null. So if this returned something like 42, then we should throw. If it returned undefined or null, we shouldn't throw.

(In reply to comment #4) > Committed r198554: <http://trac.webkit.org/changeset/198554> It caused a serious regression on ARMv7 Thumb2 platforms: bug155784

Comment on attachment 274678 [details] Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to only check that it is an object, per spec. View in context: https://bugs.webkit.org/attachment.cgi?id=274678&action=review >> Source/JavaScriptCore/builtins/StringPrototype.js:40 >> + var matcher = regexp[@symbolMatch]; > > This isn't correct w.r.t to the spec. > It looks like the spec calls GetMethod here. GetMethod will throw if the result isn't undefined or null. > So if this returned something like 42, then we should throw. If it returned undefined or null, > we shouldn't throw. Ah, oops! I missed that when implementing String.prototype.search, nice! I think `if (searcher !== @undefined)` to `if (searcher != @undefined)` will solve the issue. If the returned value is non undefined/null and non-function value (like 42), `searcher.@call(regexp, this);` will throw an exception, "Exception: TypeError: 42 is not a function". I'll upload the patch fixing the issues in String.prototype.match and String.prototype.search :D

(In reply to comment #5) > Comment on attachment 274678 [details] > Updated patch - relaxed the |this| check in RegExp.prototype[@@match] to > only check that it is an object, per spec. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274678&action=review > > r=me with the noted suggestions. > I think it's also worth adding a test or two that uses Proxy to verify that > operations are performed only the number of needed times, in the right > order, etc. > > > Source/JavaScriptCore/builtins/RegExpPrototype.js:59 > > + if (regexp.exec !== @RegExp.prototype.@exec && typeof(regexp.exec) === "function") { > > Style: I think our "typeof" style is usually this: typeof regex.exec === > "function" > Also, I'm not sure this is correct. You're calling "get" twice here. That is > an observable > operation. It looks like RegExpExec only does this get once in the spec. Going to handle this issue as well as adding Proxy based tests in a new patch. Tracked with <https://bugs.webkit.org/show_bug.cgi?id=155807> > > Source/JavaScriptCore/builtins/StringPrototype.js:40 > > + var matcher = regexp[@symbolMatch]; > > This isn't correct w.r.t to the spec. > It looks like the spec calls GetMethod here. GetMethod will throw if the > result isn't undefined or null. > So if this returned something like 42, then we should throw. If it returned > undefined or null, > we shouldn't throw. This was handled in <https://bugs.webkit.org/show_bug.cgi?id=155785> and landed in change set <http://trac.webkit.org/changeset/198581>.