Patch coming.
Created attachment 295392 [details] proposed patch.
Comment on attachment 295392 [details] proposed patch. It is invalid to replace returning encodedJSValue() with returning { }. On 32-bit builds, the former is non-zero, while the latter is 0. Will fix this patch.
Created attachment 295429 [details] proposed patch.
Comment on attachment 295429 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=295429&action=review > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:239 > + ASSERT(!scope.exception() || (flags == InvalidFlags)); should this be (!!scope.exception() == (flags == InvalidFlags))? > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:294 > + RETURN_IF_EXCEPTION(scope, nullptr); > + if (flagsArg.isUndefined()) { > flagsArg = patternArg.get(exec, vm.propertyNames->flags); > + RETURN_IF_EXCEPTION(scope, nullptr); > + } This seems easily testable through JS code by ensuring the second get doesn't execute. Maybe you can write such a test? > Source/JavaScriptCore/runtime/RegExpObjectInlines.h:78 > + ASSERT(!scope.exception() || lastIndex == UINT_MAX); should this be (!!scope.excetpion() == (lastIndex == UINT_MAX))?
(In reply to comment #4) > > Source/JavaScriptCore/runtime/RegExpObjectInlines.h:78 > > + ASSERT(!scope.exception() || lastIndex == UINT_MAX); > > should this be (!!scope.excetpion() == (lastIndex == UINT_MAX))? No, because getRegExpObjectLastIndexAsUnsigned() can return UINT_MAX as a failure result that does not necessarily mean that an exception was thrown. Note: an exception being thrown means that lastIndex must be UINT_MAX. The reverse is not true that a lastIndex being UINT_MAX means that an exception was thrown.
(In reply to comment #4) > Comment on attachment 295429 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295429&action=review > > > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:239 > > + ASSERT(!scope.exception() || (flags == InvalidFlags)); > > should this be (!!scope.exception() == (flags == InvalidFlags))? I think this is correct. I will fix this and also change the "ASSERT(scope.exception() || flagsString);" in toFlags() to "ASSERT(!scope.exception() == !flagsString);". Will retest the code before committing. > > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:294 > > + RETURN_IF_EXCEPTION(scope, nullptr); > > + if (flagsArg.isUndefined()) { > > flagsArg = patternArg.get(exec, vm.propertyNames->flags); > > + RETURN_IF_EXCEPTION(scope, nullptr); > > + } > > This seems easily testable through JS code by ensuring the second get > doesn't execute. Maybe you can write such a test? While it's easy to reproduce the condition of an exception being thrown when getting the "source" property, it's not easy to have this observable in JS code in the process to "get" the "flags" property. This is because there are many other exception checks before we execute that getter, which in turn masks the issue.
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 295429 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=295429&action=review > > > > > Source/JavaScriptCore/runtime/RegExpConstructor.cpp:239 > > > + ASSERT(!scope.exception() || (flags == InvalidFlags)); > > > > should this be (!!scope.exception() == (flags == InvalidFlags))? > > I think this is correct. I will fix this and also change the > "ASSERT(scope.exception() || flagsString);" in toFlags() to > "ASSERT(!scope.exception() == !flagsString);". Will retest the code before > committing. Correction: will replace "ASSERT(scope.exception() || flagsString);" in toFlags() with "ASSERT(!!scope.exception() == !flagsString);".
Created attachment 295619 [details] patch for landing.
Comment on attachment 295619 [details] patch for landing. Attachment 295619 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2590440 New failing tests: sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T12.html sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T13.html
Created attachment 295631 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #9) > New failing tests: > sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T12. > html > sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.4/S15.10.4.1_A8_T13. > html These are because RegExpConstructor's toFlags() uses JSValue::toString() instead of JSValue::toStringOrNull(). As a result, the returned flagsString is not null when an exception is thrown. Will fix this in my soon to be uploaded patch.
Created attachment 295632 [details] patch for landing.
Comment on attachment 295632 [details] patch for landing. All tests have passed. Let's land this.
Comment on attachment 295632 [details] patch for landing. Rejecting attachment 295632 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 295632, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/2591410
(In reply to comment #14) > ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Never mind. Landing manually.
Landed in r209101: <http://trac.webkit.org/r209101>.