Bug 165054 – Fix exception scope verification failures in runtime/RegExp* files.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 295392 [details]
proposed patch.

[Mark Lam]

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.

[Mark Lam]

Created attachment 295429 [details]
proposed patch.

[Saam Barati]

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))?

[Mark Lam]

(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.

[Mark Lam]

(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.

[Mark Lam]

(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);".

[Mark Lam]

Created attachment 295619 [details]
patch for landing.

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

(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.

[Mark Lam]

Created attachment 295632 [details]
patch for landing.

[Mark Lam]

Comment on attachment 295632 [details]
patch for landing.

All tests have passed.  Let's land this.

[WebKit Commit Bot]

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

[Mark Lam]

(In reply to comment #14)
> ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Never mind.  Landing manually.

[Mark Lam]

Landed in r209101: <http://trac.webkit.org/r209101>.