Bug 193713 – [JSC] NativeErrorConstructor should not have own IsoSubspace

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

It's too costly for NativeErrorConstructor. We should take the similar implementation to our TypedArrayViewConstructors.

[Yusuke Suzuki]

Created attachment 360196 [details]
Patch

[Yusuke Suzuki]

Created attachment 360198 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:73
> +// Instantiate JSGenericArrayBufferConstructors.

Oops, this comment is meaningless and wrong. Removed.

[Saam Barati]

Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:73
> +// Instantiate JSGenericArrayBufferConstructors.

Wrong comment.

[Saam Barati]

You have some build issues.

[Yusuke Suzuki]

Oops, yeah, I need to make ErrorType.h Private instead of Project in Xcode.

[Yusuke Suzuki]

Created attachment 360199 [details]
Patch

EWS

[Mark Lam]

Comment on attachment 360198 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        threads, this is OK. While TypeError constructor is eagerly allocated because it is touched from our builtin JS as @TypeError, we should
> +        offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference. This change removes

Add a bug and FIXME for "offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference"?

> Source/JavaScriptCore/runtime/ErrorType.cpp:39
> +static ASCIILiteral errorTypeNames[] = {
> +    "Error"_s,
> +    "EvalError"_s,
> +    "RangeError"_s,
> +    "ReferenceError"_s,
> +    "SyntaxError"_s,
> +    "TypeError"_s,
> +    "URIError"_s,
> +};

Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?

[Yusuke Suzuki]

Comment on attachment 360198 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference. This change removes
> 
> Add a bug and FIXME for "offer some function instead of exposing TypeError constructor in the future, and remove this @TypeError reference"?

Cool. Added https://bugs.webkit.org/show_bug.cgi?id=193858.

>> Source/JavaScriptCore/runtime/ErrorType.cpp:39
>> +};
> 
> Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?

Discussed with Mark, we should move this inside the errorTypeName function with `static const ASCIILiteral const errorTypeNames[]`.

[Yusuke Suzuki]

Comment on attachment 360198 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ErrorType.cpp:39
>>> +};
>> 
>> Suggestion: make this a static const char* const errorTypeNames[] and move it inside the errorTypeName() function below?
> 
> Discussed with Mark, we should move this inside the errorTypeName function with `static const ASCIILiteral const errorTypeNames[]`.

static const ASCIILiteral errorTypeNames[], and make field of ASCIILiteral `const char* const`.

[EWS Watchlist]

Comment on attachment 360199 [details]
Patch

Attachment 360199 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10896019

New failing tests:
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/specifier-error.html
http/tests/security/cross-frame-access-put.html
fast/frames/frame-window-as-callback.html
js/dom/function-names.html

[EWS Watchlist]

Created attachment 360206 [details]
Archive of layout-test-results from ews100 for mac-highsierra

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

[Yusuke Suzuki]

Comment on attachment 360199 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:406
> +    NativeErrorPrototype* prototype = NativeErrorPrototype::create(init.vm, m_nativeErrorPrototypeStructure.get(), errorTypeName(errorType));
> +    init.setPrototype(prototype);
> +    Structure* structure = ErrorInstance::createStructure(init.vm, this, prototype);
> +    init.setStructure(structure);
> +    init.setConstructor(NativeErrorConstructor<errorType>::create(init.vm, NativeErrorConstructor<errorType>::createStructure(init.vm, this, prototype), prototype));
> +    prototype->putDirect(init.vm, init.vm.propertyNames->constructor, init.constructor, static_cast<unsigned>(PropertyAttribute::DontEnum));

Oops, I fixed NativeErrorConstructor<errorType>::createStructure(init.vm, this, m_errorConstructor.get()). I aligned this code to LazyClassStructure's use.

init.setPrototype(NativeErrorPrototype::create(init.vm, NativeErrorPrototype::createStructure(init.vm, this, m_errorPrototype.get()), errorTypeName(errorType)));
init.setStructure(ErrorInstance::createStructure(init.vm, this, init.prototype));
init.setConstructor(NativeErrorConstructor<errorType>::create(init.vm, NativeErrorConstructor<errorType>::createStructure(init.vm, this, m_errorConstructor.get()), jsCast<NativeErrorPrototype*>(init.prototype)));

init.setConstructor automatically perform init.prototype->putDirectWithoutTransition("constructor", constructor ...) thing.

[Yusuke Suzuki]

Created attachment 360212 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 360213 [details]
Patch for landing

[EWS Watchlist]

Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10897233

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360216 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10897197

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360219 [details]
Archive of layout-test-results from ews114 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 360198 [details]
Patch

Attachment 360198 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10896540

New failing tests:
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-eager
stress/try-get-by-id-should-spill-registers-dfg.js.bytecode-cache
stress/try-get-by-id-should-spill-registers-dfg.js.no-cjit-validate-phases
ChakraCore.yaml/ChakraCore/test/Lib/error.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager
ChakraCore.yaml/ChakraCore/test/Error/ErrorCtorProps.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-validate-sampling-profiler
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager-no-cjit
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-small-pool
stress/try-get-by-id-should-spill-registers-dfg.js.no-llint
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-maximal-flush-validate-no-cjit
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-eager-no-cjit-b3o1
stress/try-get-by-id-should-spill-registers-dfg.js.dfg-eager-no-cjit-validate
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-no-put-stack-validate
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-b3o1
stress/try-get-by-id-should-spill-registers-dfg.js.no-cjit-collect-continuously
stress/try-get-by-id-should-spill-registers-dfg.js.no-ftl
stress/try-get-by-id-should-spill-registers-dfg.js.default
ChakraCore.yaml/ChakraCore/test/Lib/tostring.js.default
stress/try-get-by-id-should-spill-registers-dfg.js.ftl-no-cjit-no-inline-validate
apiTests

[EWS Watchlist]

Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10897308

Number of test failures exceeded the failure limit.

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10897395

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360221 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 360213 [details]
Patch for landing

Attachment 360213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10897609

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360222 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

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

[Yusuke Suzuki]

Created attachment 360224 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 360230 [details]
Patch for landing

[Yusuke Suzuki]

Committed r240543: <https://trac.webkit.org/changeset/240543>

[Radar WebKit Bug Importer]

<rdar://problem/47572897>

[Ryan Haddad]

The test added with this change is crashing on the debug JSC bot:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2080

[Yusuke Suzuki]

Committed r240629: <https://trac.webkit.org/changeset/240629>