Bug 165054 - Fix exception scope verification failures in runtime/RegExp* files.
Summary: Fix exception scope verification failures in runtime/RegExp* files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-23 20:48 PST by Mark Lam
Modified: 2016-11-29 16:07 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (17.73 KB, patch)
2016-11-23 20:54 PST, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (14.12 KB, patch)
2016-11-25 14:07 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (14.71 KB, patch)
2016-11-29 12:15 PST, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.75 MB, application/zip)
2016-11-29 13:26 PST, Build Bot
no flags Details
patch for landing. (14.80 KB, patch)
2016-11-29 13:36 PST, Mark Lam
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.