Bug 176645 – Error should compute .stack and friends lazily

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 320331 [details]
possible patch

I haven't tried compiling it yet.

[Filip Pizlo]

Created attachment 320332 [details]
the patch

[Filip Pizlo]

Created attachment 320335 [details]
the patch

Updated patch.

I'm pretty sure this will have WebCore build errors.  But I'll put it up anyway while I run some benchmarks.

[Saam Barati]

Comment on attachment 320335 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        This is an enormous speed-up for programs that allocate and throw exceptions.

Awesome

> Source/JavaScriptCore/runtime/Error.cpp:41
> +#include "SuperSampler.h"

Please remove it doesn’t look like it’s used

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:29
> +#include "SuperSampler.h"

Please remove it doesn’t look like it’s used

[Saam Barati]

Comment on attachment 320335 [details]
the patch

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

> Source/JavaScriptCore/runtime/Error.cpp:-184
> -        if (bytecodeOffset) {

This is no longer needed?

[Build Bot]

Comment on attachment 320335 [details]
the patch

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

New failing tests:
stress/error-stack-trace-limit.js.no-llint
stress/error-stack-trace-limit.js.ftl-no-cjit-small-pool
stress/error-stack-trace-limit.js.ftl-no-cjit-b3o1
ChakraCore.yaml/ChakraCore/test/StackTrace/ErrorDotStackAlreadyExists.js.default
stress/error-stack-trace-limit.js.ftl-no-cjit-no-put-stack-validate
stress/error-stack-trace-limit.js.ftl-eager-no-cjit
stress/error-stack-trace-limit.js.ftl-eager-no-cjit-b3o1
stress/error-stack-trace-limit.js.default
stress/error-stack-trace-limit.js.no-cjit-validate-phases
stress/error-stack-trace-limit.js.ftl-eager
ChakraCore.yaml/ChakraCore/test/Error/NativeErrors.js.default
stress/error-stack-trace-limit.js.dfg-eager-no-cjit-validate
stress/error-stack-trace-limit.js.no-cjit-collect-continuously
stress/error-stack-trace-limit.js.no-ftl
stress/error-stack-trace-limit.js.dfg-eager
stress/error-stack-trace-limit.js.dfg-maximal-flush-validate-no-cjit
stress/error-stack-trace-limit.js.ftl-no-cjit-no-inline-validate
stress/error-stack-trace-limit.js.ftl-no-cjit-validate-sampling-profiler

[Build Bot]

Comment on attachment 320335 [details]
the patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320337 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320335 [details]
the patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320338 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

(In reply to Saam Barati from comment #5)
> Comment on attachment 320335 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320335&action=review
> 
> > Source/JavaScriptCore/runtime/Error.cpp:-184
> > -        if (bytecodeOffset) {
> 
> This is no longer needed?

It's in getBytecodeOffset() now.

[Build Bot]

Comment on attachment 320335 [details]
the patch

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

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
http/tests/security/contentSecurityPolicy/function-constructor-blocked.html
js/dom/parse-error-external-script-in-eval.html
fast/dom/NamedNodeMap-setNamedItem-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html
imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
http/tests/security/xss-DENIED-contentWindow-eval.html
http/tests/workers/worker-importScriptsOnError.html
imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.html
js/dom/global-recursion-on-full-stack.html
fast/dom/Window/window-custom-prototype-crash.html
fast/dom/timer-clear-interval-in-handler-and-generate-error.html
imported/w3c/web-platform-tests/streams/piping/general.html
http/tests/security/contentSecurityPolicy/eval-blocked.html
fast/events/window-onerror5.html
http/tests/security/contentSecurityPolicy/module-eval-blocked.html
fast/events/window-onerror4.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-subframe.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-about-blank-iframe.html
js/dom/concat-large-strings-crash.html
http/tests/security/window-named-proto.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-external-script.html
http/tests/security/contentSecurityPolicy/module-eval-blocked-in-external-script.html

[Build Bot]

Created attachment 320339 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Filip Pizlo]

Created attachment 320351 [details]
possible patch for landing

Fixing various bugs.

[Build Bot]

Comment on attachment 320351 [details]
possible patch for landing

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

New failing tests:
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-ftl-no-cjit
ChakraCore.yaml/ChakraCore/test/StackTrace/ErrorDotStackAlreadyExists.js.default
ChakraCore.yaml/ChakraCore/test/Error/NativeErrors.js.default
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/exception-properties.js.layout-no-ftl

[Build Bot]

Comment on attachment 320351 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320354 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320351 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320355 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320351 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320356 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Created attachment 320357 [details]
possible patch for landing

It took me a while to figure out exactly what to override if you want to handle for-in by materializing.

[Filip Pizlo]

I think we can ignore the remaining JSC test failure, because it's a ChakraCore test that asserts that the stack property and its friends are in a particular order as observable by for-in. Because these are non-standard properties, I don't think we have to worry about this.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #23)
> I think we can ignore the remaining JSC test failure, because it's a
> ChakraCore test that asserts that the stack property and its friends are in
> a particular order as observable by for-in. Because these are non-standard
> properties, I don't think we have to worry about this.

(so I'll skip or modify the test.)

[Build Bot]

Comment on attachment 320357 [details]
possible patch for landing

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

New failing tests:
ChakraCore.yaml/ChakraCore/test/StackTrace/ErrorDotStackAlreadyExists.js.default

[Filip Pizlo]

Looks like I still have DOM exception issues.

[Build Bot]

Comment on attachment 320357 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320361 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320357 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320362 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320357 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320363 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320357 [details]
possible patch for landing

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

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
http/tests/security/contentSecurityPolicy/function-constructor-blocked.html
js/dom/parse-error-external-script-in-eval.html
fast/dom/NamedNodeMap-setNamedItem-crash.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
fast/events/window-onerror-syntax-error-in-attr.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html
imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
http/tests/security/xss-DENIED-contentWindow-eval.html
http/tests/workers/worker-importScriptsOnError.html
imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.html
js/dom/global-recursion-on-full-stack.html
fast/dom/Window/window-custom-prototype-crash.html
fast/dom/timer-clear-interval-in-handler-and-generate-error.html
imported/w3c/web-platform-tests/streams/piping/general.html
http/tests/security/contentSecurityPolicy/eval-blocked.html
fast/events/window-onerror5.html
http/tests/security/contentSecurityPolicy/module-eval-blocked.html
fast/events/window-onerror4.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-subframe.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-about-blank-iframe.html
js/dom/concat-large-strings-crash.html
http/tests/security/window-named-proto.html
http/tests/security/contentSecurityPolicy/eval-blocked-in-external-script.html
http/tests/security/contentSecurityPolicy/module-eval-blocked-in-external-script.html

[Build Bot]

Created attachment 320364 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Filip Pizlo]

Created attachment 320371 [details]
possible patch for landing

Fixed more bugs.

There was code relying on being able to get the line/etc properties using getDirect.  That doesn't work if we're lazily materializing them.

[Build Bot]

Comment on attachment 320371 [details]
possible patch for landing

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

New failing tests:
userscripts/window-onerror-for-isolated-world-2.html
fast/events/window-onerror4.html
inspector/unit-tests/globals-uncaught-exception-from-timer-callback.html
userscripts/window-onerror-for-isolated-world-1.html
js/dom/parse-error-external-script-in-new-Function.html
fast/events/window-onerror-syntax-error-in-attr.html
inspector/debugger/setBreakpoint-options-exception.html
inspector/console/messageAdded-from-named-evaluations.html

[Build Bot]

Created attachment 320372 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320371 [details]
possible patch for landing

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

New failing tests:
fast/events/window-onerror4.html
inspector/unit-tests/globals-uncaught-exception-from-timer-callback.html
js/dom/parse-error-external-script-in-new-Function.html
fast/events/window-onerror-syntax-error-in-attr.html
inspector/debugger/setBreakpoint-options-exception.html
inspector/console/messageAdded-from-named-evaluations.html

[Build Bot]

Created attachment 320373 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320371 [details]
possible patch for landing

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

New failing tests:
userscripts/window-onerror-for-isolated-world-2.html
fast/events/window-onerror4.html
inspector/unit-tests/globals-uncaught-exception-from-timer-callback.html
userscripts/window-onerror-for-isolated-world-1.html
js/dom/parse-error-external-script-in-new-Function.html
fast/events/window-onerror-syntax-error-in-attr.html
inspector/debugger/setBreakpoint-options-exception.html
inspector/console/messageAdded-from-named-evaluations.html

[Build Bot]

Created attachment 320374 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320371 [details]
possible patch for landing

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

New failing tests:
js/dom/parse-error-external-script-in-new-Function.html
fast/events/window-onerror4.html
fast/events/window-onerror-syntax-error-in-attr.html

[Build Bot]

Created attachment 320375 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Filip Pizlo]

Created attachment 320377 [details]
possible patch for landing

Fixed a few random bugs in how the DOM now sees exceptions.

[Build Bot]

Comment on attachment 320377 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320378 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320377 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320379 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320377 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320380 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320377 [details]
possible patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320381 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Filip Pizlo]

Created attachment 320396 [details]
possible patch for landing

Boy, getting these line/column heuristics right is tough.

Turns out that ScriptCallStackFactory does need to handle the case where column isn't set.

[Filip Pizlo]

Created attachment 320398 [details]
possible patch for landing

Cleaned things up more

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/221836/webkit

[Ryan Haddad]

This change appears to have introduced a large number of memory leaks: https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29/builds/3097

I can't get the leaks viewer to work for this build and the ones after it (I guess the log is too large?). Here is a link to download the results: https://build.webkit.org/results/Apple%20Sierra%20%28Leaks%29/r221836%20%283097%29.zip

[Yusuke Suzuki]

(In reply to Ryan Haddad from comment #56)
> This change appears to have introduced a large number of memory leaks:
> https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29/builds/3097
> 
> I can't get the leaks viewer to work for this build and the ones after it (I
> guess the log is too large?). Here is a link to download the results:
> https://build.webkit.org/results/Apple%20Sierra%20%28Leaks%29/
> r221836%20%283097%29.zip

I think we should have ErrorInstance::destroy to call ~ErrorInstance.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #57)
> (In reply to Ryan Haddad from comment #56)
> > This change appears to have introduced a large number of memory leaks:
> > https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29/builds/3097
> > 
> > I can't get the leaks viewer to work for this build and the ones after it (I
> > guess the log is too large?). Here is a link to download the results:
> > https://build.webkit.org/results/Apple%20Sierra%20%28Leaks%29/
> > r221836%20%283097%29.zip
> 
> I think we should have ErrorInstance::destroy to call ~ErrorInstance.

Yeah, I just saw that.  Working on a patch.

[Filip Pizlo]

(In reply to Ryan Haddad from comment #56)
> This change appears to have introduced a large number of memory leaks:
> https://build.webkit.org/builders/Apple%20Sierra%20%28Leaks%29/builds/3097
> 
> I can't get the leaks viewer to work for this build and the ones after it (I
> guess the log is too large?). Here is a link to download the results:
> https://build.webkit.org/results/Apple%20Sierra%20%28Leaks%29/
> r221836%20%283097%29.zip

Should be fixed in https://trac.webkit.org/changeset/222186/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34693606>