RESOLVED FIXED 162351
Turn on exception scope verification for JSC tests.
https://bugs.webkit.org/show_bug.cgi?id=162351
Summary Turn on exception scope verification for JSC tests.
Mark Lam
Reported 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.
Attachments
x86_64 benchmark results. (86.87 KB, text/plain)
2016-09-21 11:27 PDT, Mark Lam
no flags
work in progress: has new JSC test failures that need to be resolved. (340.26 KB, patch)
2016-09-21 14:10 PDT, Mark Lam
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite (1.43 MB, application/zip)
2016-09-21 15:14 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.15 MB, application/zip)
2016-09-21 15:26 PDT, Build Bot
no flags
Archive of layout-test-results from ews114 for mac-yosemite (1.87 MB, application/zip)
2016-09-21 15:28 PDT, Build Bot
no flags
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2 (10.14 MB, application/zip)
2016-09-21 15:32 PDT, Build Bot
no flags
proposed patch. (1.65 KB, patch)
2016-11-21 09:00 PST, Mark Lam
no flags
proposed patch. (1.66 KB, patch)
2016-11-21 09:02 PST, Mark Lam
no flags
proposed patch. (2.93 KB, patch)
2017-09-11 11:32 PDT, Mark Lam
saam: review+
Mark Lam
Comment 1 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.
Mark Lam
Comment 2 2016-09-21 14:10:40 PDT
Created attachment 289476 [details] work in progress: has new JSC test failures that need to be resolved.
WebKit Commit Bot
Comment 3 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.
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Build Bot
Comment 11 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
Geoffrey Garen
Comment 12 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.
Geoffrey Garen
Comment 13 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.
Mark Lam
Comment 14 2016-11-21 09:00:32 PST
Created attachment 295296 [details] proposed patch.
Mark Lam
Comment 15 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.
Mark Lam
Comment 16 2016-11-21 09:02:30 PST
Created attachment 295297 [details] proposed patch.
Mark Lam
Comment 17 2016-12-07 16:09:35 PST
Mark Lam
Comment 18 2017-09-11 11:32:11 PDT
Created attachment 320447 [details] proposed patch.
Mark Lam
Comment 19 2017-09-11 11:43:53 PDT
Thanks for the review. Landed in r221868: <http://trac.webkit.org/r221868>.
Ryan Haddad
Comment 20 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>
Mark Lam
Comment 21 2017-09-28 11:26:40 PDT
I've relanded this patch in r222618: <http://trac.webkit.org/r222618>.
Note You need to log in before you can comment on or make changes to this bug.