Bug 162351

Summary: Turn on exception scope verification for JSC tests.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, buildbot, commit-queue, fpizlo, ggaren, jfbastien, jsbell, keith_miller, msaboff, rmorisset, rniwa, ryanhaddad, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 165105, 162521, 162583, 162584, 162637, 162793, 162797, 164958, 164959, 164964, 164966, 164968, 164969, 164971, 164972, 164975, 164976, 164995, 164996, 164998, 165011, 165012, 165013, 165014, 165015, 165016, 165020, 165021, 165022, 165025, 165035, 165046, 165047, 165049, 165050, 165051, 165053, 165054, 165055, 165066, 165067, 165096, 165102, 176662, 176924, 177609    
Bug Blocks:    
Attachments:
Description Flags
x86_64 benchmark results.
none
work in progress: has new JSC test failures that need to be resolved.
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2
none
proposed patch.
none
proposed patch.
none
proposed patch. sbarati: review+

Description Mark Lam 2016-09-21 11:23:54 PDT
This patch takes the number of test failures down from around 13000+ down to 2000+.  Perf appears to be neutral.  Will upload the patch shortly.
Comment 1 Mark Lam 2016-09-21 11:27:40 PDT
Created attachment 289468 [details]
x86_64 benchmark results.

The benchmark results shows that perf is neutral.  All the "definitely" differences appear to be noise.  They don't reproduce when I do another round of benchmarking.  The only one that persisted is the int-or-other-abs-then-get-by-val.js microbenchmark, and I've fixed that (in the patch that I'm preparing for upload).

Archiving these results for reference.
Comment 2 Mark Lam 2016-09-21 14:10:40 PDT
Created attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.
Comment 3 WebKit Commit Bot 2016-09-21 14:12:51 PDT
Attachment 289476 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSONObject.cpp:829:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 95 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2016-09-21 15:14:28 PDT
Comment on attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.

Attachment 289476 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2120609

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html
fast/custom-elements/CustomElementRegistry.html
http/tests/plugins/cross-frame-object-access.html
Comment 5 Build Bot 2016-09-21 15:14:33 PDT
Created attachment 289479 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-09-21 15:26:49 PDT
Comment on attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.

Attachment 289476 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2120645

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
plugins/npruntime/object-from-destroyed-plugin.html
fast/custom-elements/CustomElementRegistry.html
http/tests/plugins/cross-frame-object-access.html
Comment 7 Build Bot 2016-09-21 15:26:53 PDT
Created attachment 289482 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 8 Build Bot 2016-09-21 15:28:23 PDT
Comment on attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.

Attachment 289476 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2120638

New failing tests:
plugins/npruntime/object-from-destroyed-plugin-in-subframe.html
http/tests/security/cross-frame-access-object-prototype.html
plugins/npruntime/object-from-destroyed-plugin.html
fast/custom-elements/CustomElementRegistry.html
http/tests/plugins/cross-frame-object-access.html
sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.6/15.10.6.2_RegExp.prototype.exec/S15.10.6.2_A4_T11.html
Comment 9 Build Bot 2016-09-21 15:28:28 PDT
Created attachment 289483 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Build Bot 2016-09-21 15:32:51 PDT
Comment on attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.

Attachment 289476 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2120642

New failing tests:
fast/custom-elements/CustomElementRegistry.html
Comment 11 Build Bot 2016-09-21 15:32:57 PDT
Created attachment 289484 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Geoffrey Garen 2016-09-22 14:13:34 PDT
Is it practical to separate out the a patch that adds new exception checks from the patch that adds lots more validation?

I think it would be helpful to see just the things that needed exception checks, as distinct from the extra validation.
Comment 13 Geoffrey Garen 2016-09-22 15:01:16 PDT
Comment on attachment 289476 [details]
work in progress: has new JSC test failures that need to be resolved.

View in context: https://bugs.webkit.org/attachment.cgi?id=289476&action=review

> Source/JavaScriptCore/ChangeLog:15
> +        1. Deleted Error.cppâs hasErrorInfo() which is not used anywhere.

ASCII please

> Source/JavaScriptCore/runtime/ExceptionScope.h:83
> +#define RETURN_IF_EXCEPTION(scope__) do { \
> +        if (UNLIKELY((scope__).exception())) \
> +            return; \
> +    } while (false)
> +
> +#define RETURN_VALUE_IF_EXCEPTION(scope__, value__) do { \
> +        if (UNLIKELY((scope__).exception())) \
> +            return (value__); \
> +    } while (false)
> +

I think you can simplify and join these macros. Functions that return void can "RETURN_VALUE_IF_EXCEPTION(scope, void)". Then you can shorten "RETURN_VALUE_IF_EXCEPTION" TO "RETURN_IF_EXCEPTION" everywhere.
Comment 14 Mark Lam 2016-11-21 09:00:32 PST
Created attachment 295296 [details]
proposed patch.
Comment 15 Mark Lam 2016-11-21 09:01:20 PST
We won't land this patch until all the dependent bugs to fix missing exception checks have landed.
Comment 16 Mark Lam 2016-11-21 09:02:30 PST
Created attachment 295297 [details]
proposed patch.
Comment 17 Mark Lam 2016-12-07 16:09:35 PST
<rdar://problem/29563911>
Comment 18 Mark Lam 2017-09-11 11:32:11 PDT
Created attachment 320447 [details]
proposed patch.
Comment 19 Mark Lam 2017-09-11 11:43:53 PDT
Thanks for the review.  Landed in r221868: <http://trac.webkit.org/r221868>.
Comment 20 Ryan Haddad 2017-09-14 11:20:15 PDT
Reverted r221868 for reason:

Rolling out this change while we investigate test262 failures.

Committed r222037: <http://trac.webkit.org/changeset/222037>
Comment 21 Mark Lam 2017-09-28 11:26:40 PDT
I've relanded this patch in r222618: <http://trac.webkit.org/r222618>.