Per https://people.mozilla.org/~jorendorff/es6-draft.html#sec-get-regexp.prototype.flags, the RegExp.prototype.flags getter should return a sorted string of the flag characters, suitable for being passed into the RegExp constructor.
Created attachment 254186 [details] Patch
Comment on attachment 254186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254186&action=review > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:76 > +void RegExpPrototype::finishCreation(VM& vm, JSGlobalObject* globalObject) > +{ > + JSFunction* flagsFunction = JSFunction::create(vm, globalObject, 0, vm.propertyNames->flags.string(), regExpProtoFuncGetFlags); > + GetterSetter* accessor = GetterSetter::create(vm, globalObject); > + accessor->setGetter(vm, globalObject, flagsFunction); > + putDirectNonIndexAccessor(vm, vm.propertyNames->flags, accessor, DontEnum | Accessor); > +} Can you use a standard host property instead of a Javascript getter? That's more efficient, and it should require less code. > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:139 > +static inline String getRegExpFlags(ExecState *exec, RegExpObject* regexp) We usually use "get" to mean "returns a value through an out parameter". This function should be named "flags()" or "flagsString()".
(In reply to comment #2) Can you use a standard host property instead of a Javascript getter? That's > more efficient, and it should require less code. No, the spec requires that it be a getter on the prototype. > > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:139 > > +static inline String getRegExpFlags(ExecState *exec, RegExpObject* regexp) > > We usually use "get" to mean "returns a value through an out parameter". > This function should be named "flags()" or "flagsString()". Sure, I'll rename it this evening.
Actually, perhaps I misunderstood your comment - can you point me to an example of a "standard host property" instead of a getter? Also, note that "flags" is meant to work with subclassing, so it'd always need to check the appropriate flag properties (global, etc), even if it's .call-ed with a RegExp subclass instance.
(In reply to comment #4) > Also, note that "flags" is meant to work with subclassing, so it'd always > need to check the appropriate flag properties (global, etc), even if it's > .call-ed with a RegExp subclass instance. Should cover that with a test case.
(In reply to comment #5) > (In reply to comment #4) > > Also, note that "flags" is meant to work with subclassing, so it'd always > > need to check the appropriate flag properties (global, etc), even if it's > > .call-ed with a RegExp subclass instance. > > Should cover that with a test case. Good call - WK doesn't support subclassing yet afaik, but I can definitely call it with a generic object in tests and make sure the right strings are produced. Thanks!
Created attachment 254242 [details] Patch
> Actually, perhaps I misunderstood your comment - can you point me to an > example of a "standard host property" instead of a getter? Consider the other properties of a RegExpObject: global; ignoreCase; multiline; and source. To produce a standard host property, they list themselves in a table: /* Source for RegExpObject.lut.h @begin regExpTable global regExpObjectGlobal DontDelete|ReadOnly|DontEnum ignoreCase regExpObjectIgnoreCase DontDelete|ReadOnly|DontEnum multiline regExpObjectMultiline DontDelete|ReadOnly|DontEnum source regExpObjectSource DontDelete|ReadOnly|DontEnum @end This table is processed by a script at compile time in order to produce a hash table of properties. The second entry in the table indicates the function to call in order to access the property. Can you make flags work this way? > Also, note that "flags" is meant to work with subclassing, so it'd always > need to check the appropriate flag properties (global, etc), even if it's > .call-ed with a RegExp subclass instance. Yup.
(In reply to comment #8) > > Actually, perhaps I misunderstood your comment - can you point me to an > > example of a "standard host property" instead of a getter? > > Consider the other properties of a RegExpObject: global; ignoreCase; > multiline; and source. To produce a standard host property, they list > themselves in a table: > > /* Source for RegExpObject.lut.h > @begin regExpTable > global regExpObjectGlobal DontDelete|ReadOnly|DontEnum > ignoreCase regExpObjectIgnoreCase DontDelete|ReadOnly|DontEnum > multiline regExpObjectMultiline DontDelete|ReadOnly|DontEnum > source regExpObjectSource DontDelete|ReadOnly|DontEnum > @end > > This table is processed by a script at compile time in order to produce a > hash table of properties. > > The second entry in the table indicates the function to call in order to > access the property. > > Can you make flags work this way? > > > Also, note that "flags" is meant to work with subclassing, so it'd always > > need to check the appropriate flag properties (global, etc), even if it's > > .call-ed with a RegExp subclass instance. > > Yup. However, currently `Object.getOwnPropertyDescriptor(RegExp.prototype, 'source')` returns a descriptor with "value" set, and not "get". The spec for "flags" requires that it be an actual getter function, so it'd be different from the four regExpTable examples you mentioned. I'm pretty new to C++ and Webkit both - do you think this would work in the same way if it was in the table?
> However, currently `Object.getOwnPropertyDescriptor(RegExp.prototype, > 'source')` returns a descriptor with "value" set, and not "get". The spec > for "flags" requires that it be an actual getter function, so it'd be > different from the four regExpTable examples you mentioned. Aha. I understand your motivation now. In fact, all of these properties are supposed to be getters according to the ES6 spec. It's a bug in our implementation of these automatic tables that they do not produce proper getters. Still, I think it is best to implement all of these properties through the same mechanism, rather than fixing the getter issue for one property through special-case code. Then, we should update the underlying mechanism to produce getters, as ES6 requires.
That's a fair point, but I'm super wary of introducing new functionality in a broken way. I'd prefer to add flags correctly, and migrate it to a "proper" mechanism once that exists - otherwise the es6-shim will have to overwrite Safari's broken implementation with its own, and the ES6 Compatibility Table will have to mark Webkit as failing to have that feature.
In other words, I feel strongly that spec correctness is far more important than "implementation code consistency", because the latter can be cleaned up later transparently - the former is a user-visible bug that the entire internet has to support for effectively forever.
I agree with Geoff.
In that case, this shouldn't land. It's horrific to suggest landing a known broken feature and condemning the internet to support it forever, so this should just wait until the other properties can be changed to getters.
Comment on attachment 254242 [details] Patch I've filed this bug to follow up on getter-ness for all properties: https://bugs.webkit.org/show_bug.cgi?id=145705. Maybe you can help us resolve it.
Comment on attachment 254242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254242&action=review Added comments :) > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:143 > + if (regexp->get(exec, exec->propertyNames().global).toBoolean(exec)) We need to check an exceptions per `get` since it may cause an exception. > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:157 > + JSValue flags = jsMakeNontrivialString(exec, flagsString(exec, asRegExpObject(thisValue))); Because we just check `thisValue.inherits(JSObject::info())`, `asRegExpObject` is unsafe. It may not be `RegExpObject`. For example, var desc = Object.getOwnPropertyDescriptor(RegExp.prototype, 'flags'); desc.get.call([]); According to the spec, RegExp.prototype.flags accepts all JSObject (indlucing JSArray). So I think using `isObject` guard and `asObject` is enough. > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:173 > String source = thisObject->get(exec, exec->propertyNames().source).toString(exec)->value(exec); We need to check an exception here before executing `flagsString`. And since `get` may cause an exception, `thisObject->get(exec, exec->propertyNames().source).toString(exec)` is not safe. ... thisObject->get(exec, exec->propertyNames().source); // check exception ....toString(exec)->value(exec); // check exception is needed.
Thanks, I'll update this patch with those comments after 145705 goes in.
Created attachment 254633 [details] Patch
Comment on attachment 254633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254633&action=review Patch itself looks nice. With nits. > Source/JavaScriptCore/runtime/RegExpPrototype.cpp:224 > + JSValue flags = jsMakeNontrivialString(exec, flagsString(exec, asObject(thisValue))); flags = flagsString(exec, asObject(thisValue)); // check exception return JSValue::encode(jsMakeNontrivialString(exec, flags)); is preferable.
Created attachment 254663 [details] Patch
Comment on attachment 254663 [details] Patch r=me
Comment on attachment 254663 [details] Patch Clearing flags on attachment: 254663 Committed r185432: <http://trac.webkit.org/changeset/185432>
All reviewed patches have been landed. Closing bug.
Yay! Thanks everyone for the reviews, and the patience as we worked this out, and special thanks to Yusuke for taking on 145705 to pave the way for this.
Looks like js/regexp-flags.html started crashing on several bots after this change: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r185432%20(4870)/results.html Please address ASAP.
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000110c6fd77 WTFCrash + 39 1 com.apple.JavaScriptCore 0x0000000110686da8 JSC::jsNontrivialString(JSC::VM*, WTF::String const&) + 72 2 com.apple.JavaScriptCore 0x0000000110686965 JSC::jsNontrivialString(JSC::ExecState*, WTF::String const&) + 37 3 com.apple.JavaScriptCore 0x0000000110b674e5 JSC::JSValue JSC::jsMakeNontrivialString<WTF::String&>(JSC::ExecState*, WTF::String&&&) + 37 4 com.apple.JavaScriptCore 0x0000000110b669ff JSC::regExpProtoGetterFlags(JSC::ExecState*) + 191 5 com.apple.JavaScriptCore 0x0000000110a07399 vmEntryToNative + 367 6 com.apple.JavaScriptCore 0x000000011084f30f JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1535 7 com.apple.JavaScriptCore 0x00000001102c670e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 190 8 com.apple.JavaScriptCore 0x000000011078dc5d JSC::callGetter(JSC::ExecState*, JSC::JSValue, JSC::JSValue) + 221 9 com.apple.JavaScriptCore 0x0000000110b51861 JSC::PropertySlot::functionGetter(JSC::ExecState*) const + 145 10 com.apple.JavaScriptCore 0x000000011025a3cd JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const + 93 11 com.apple.JavaScriptCore 0x000000011025d86b JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const + 91 12 com.apple.JavaScriptCore 0x00000001109fbd21 llint_slow_path_get_by_id + 241 13 com.apple.JavaScriptCore 0x0000000110a0a1b0 llint_entry + 11668 14 com.apple.JavaScriptCore 0x0000000110a071d9 vmEntryToJavaScript + 361 15 com.apple.JavaScriptCore 0x000000011086b01c JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 252 16 com.apple.JavaScriptCore 0x000000011084bcff JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) + 2543 17 com.apple.JavaScriptCore 0x000000011084b2d9 JSC::eval(JSC::ExecState*) + 1065 18 com.apple.JavaScriptCore 0x0000000110a00c48 llint_slow_path_call_eval + 328 19 com.apple.JavaScriptCore 0x0000000110a0dd36 llint_entry + 26906 20 ??? 0x0000549da7601afa 0 + 93036094692090
Looks like an assertion hit in JSC::jsNontrivialString(JSC::VM*, WTF::String const&): ASSERT(s.length() > 1);
(In reply to comment #27) > Looks like an assertion hit in JSC::jsNontrivialString(JSC::VM*, WTF::String > const&): > ASSERT(s.length() > 1); Nice catch. Since jsNontrivialString requires the given string is *non-trivial*, the length of it should be larger than 1. I'll fix it with a unreviewed very simple follow up patch.
Committed r185440: <http://trac.webkit.org/changeset/185440>