Bug 165054

Summary: Fix exception scope verification failures in runtime/RegExp* files.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162351    
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch.
saam: review+
patch for landing.
buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite
none
patch for landing. commit-queue: commit-queue-

Description Mark Lam 2016-11-23 20:48:34 PST
Patch coming.
Comment 1 Mark Lam 2016-11-23 20:54:01 PST
Created attachment 295392 [details]
proposed patch.
Comment 2 Mark Lam 2016-11-24 13:38:14 PST
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.
Comment 3 Mark Lam 2016-11-25 14:07:02 PST
Created attachment 295429 [details]
proposed patch.
Comment 4 Saam Barati 2016-11-28 13:58:36 PST
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))?
Comment 5 Mark Lam 2016-11-29 11:21:27 PST
(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.
Comment 6 Mark Lam 2016-11-29 12:06:54 PST
(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.
Comment 7 Mark Lam 2016-11-29 12:09:57 PST
(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);".
Comment 8 Mark Lam 2016-11-29 12:15:41 PST
Created attachment 295619 [details]
patch for landing.
Comment 9 Build Bot 2016-11-29 13:26:49 PST
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
Comment 10 Build Bot 2016-11-29 13:26:52 PST
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
Comment 11 Mark Lam 2016-11-29 13:32:00 PST
(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.
Comment 12 Mark Lam 2016-11-29 13:36:29 PST
Created attachment 295632 [details]
patch for landing.
Comment 13 Mark Lam 2016-11-29 16:01:39 PST
Comment on attachment 295632 [details]
patch for landing.

All tests have passed.  Let's land this.
Comment 14 WebKit Commit Bot 2016-11-29 16:03:06 PST
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
Comment 15 Mark Lam 2016-11-29 16:04:11 PST
(In reply to comment #14)
> ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Never mind.  Landing manually.
Comment 16 Mark Lam 2016-11-29 16:07:42 PST
Landed in r209101: <http://trac.webkit.org/r209101>.