Properly zero unused property storage offsets
Created attachment 342843 [details] Patch
Attachment 342843 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:166: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:166: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1184: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342843 [details] Patch Attachment 342843 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8202573 Number of test failures exceeded the failure limit.
Created attachment 342851 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 342854 [details] Patch
Attachment 342854 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1184: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342854&action=review Test case please. r=me with test case and fix for test time regression if needed. > Source/JavaScriptCore/ChangeLog:12 > + or creating a RegExp matches array we never cleared the unused array, we never clear > Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp:94 > + for (int64_t i = 0; i < static_cast<int64_t>(structure->outOfLineCapacity()); i++) { > + // We rely on properties past the last offset be zero for concurrent GC. > + if (i + firstOutOfLineOffset > structure->lastOffset()) > + ASSERT(!butterfly->propertyStorage()[-i - 1].get()); > + else if (isScribbledValue(butterfly->propertyStorage()[-i - 1].get())) { > + dataLogLn("Found scribbled property at i = ", -i - 1); > + ASSERT_NOT_REACHED(); > + } > + } If this increases debug test time too much, you need to find a way to do this assertion conditionally.
Comment on attachment 342854 [details] Patch Attachment 342854 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8204362 Number of test failures exceeded the failure limit.
Created attachment 342859 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 342854 [details] Patch Attachment 342854 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8204839 New failing tests: http/tests/preload/onload_event.html
Created attachment 342865 [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.9.0-0.318-5-3-x86_64-64bit
rdar://problem/39489288
Comment on attachment 342854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342854&action=review > Source/JavaScriptCore/runtime/RegExpMatchesArray.h:-52 > - unsigned i = (createUninitialized ? initialLength : 0); Why this change?
Comment on attachment 342854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342854&action=review >> Source/JavaScriptCore/runtime/RegExpMatchesArray.h:-52 >> - unsigned i = (createUninitialized ? initialLength : 0); > > Why this change? createUninitialized was always false. We only ever pass a custom structure.
Created attachment 342872 [details] Patch
Attachment 342872 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1184: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342873 [details] Patch
Attachment 342873 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1184: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342873 [details] Patch Attachment 342873 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8207177 Number of test failures exceeded the failure limit.
Created attachment 342875 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 342873 [details] Patch Attachment 342873 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/8207436 New failing tests: css3/filters/backdrop/add-remove-add-backdrop-filter.html
Created attachment 342876 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 342873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342873&action=review > Source/JavaScriptCore/ChangeLog:13 > + proprety storage. This patch fixes that issue and gets most users typo: /proprety/property/ > Source/JavaScriptCore/runtime/Butterfly.h:166 > + static Butterfly* tryCreateUninitialized(VM&, ObjectInitializationScope*, JSObject* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes, GCDeferralContext* = nullptr); > + static Butterfly* createUninitialized(VM&, ObjectInitializationScope*, JSObject* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes); I see that operationAllocateSimplePropertyStorageWithInitialCapacity, operationAllocateSimplePropertyStorage, and tryCreateUninitializedRegExpMatchesArray are the only 2 clients that calls these without a ObjectInitializationScope. I suggest that you: 1. Change all clients to instantiate and pass a ObjectInitializationScope anyway. 2. Pass a ObjectInitializationScope& instead of a ObjectInitializationScope*. 3. Remove the VM& argument. You can get the VM& from the ObjectInitializationScope&. ObjectInitializationScope was designed to work this way. That will keep the interface simpler, and removes the question of whether ObjectInitializationScope is needed or not (preventing someone from making the wrong choice in the future when adding new code). > Source/JavaScriptCore/runtime/ButterflyInlines.h:89 > + if (initializationScope) > + initializationScope->notifyAllocated(intendedOwner, wasCreatedUnitializated); Remove the initializationScope condition (after changing this function to take a ObjectInitializationScope& instead of ObjectInitializationScope*). > Source/JavaScriptCore/runtime/ButterflyInlines.h:102 > + if (initializationScope) > + initializationScope->notifyAllocated(intendedOwner, wasCreatedUnitializated); Remove the initializationScope condition (after changing this function to take a ObjectInitializationScope& instead of ObjectInitializationScope*). > Source/JavaScriptCore/runtime/JSObject.cpp:1185 > + Please remove whitespace.
Comment on attachment 342873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342873&action=review >> Source/JavaScriptCore/ChangeLog:13 >> + proprety storage. This patch fixes that issue and gets most users > > typo: /proprety/property/ fixed. >> Source/JavaScriptCore/runtime/Butterfly.h:166 >> + static Butterfly* createUninitialized(VM&, ObjectInitializationScope*, JSObject* intendedOwner, size_t preCapacity, size_t propertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes); > > I see that operationAllocateSimplePropertyStorageWithInitialCapacity, operationAllocateSimplePropertyStorage, and tryCreateUninitializedRegExpMatchesArray are the only 2 clients that calls these without a ObjectInitializationScope. I suggest that you: > 1. Change all clients to instantiate and pass a ObjectInitializationScope anyway. > 2. Pass a ObjectInitializationScope& instead of a ObjectInitializationScope*. > 3. Remove the VM& argument. You can get the VM& from the ObjectInitializationScope&. ObjectInitializationScope was designed to work this way. > > That will keep the interface simpler, and removes the question of whether ObjectInitializationScope is needed or not (preventing someone from making the wrong choice in the future when adding new code). I don't see how that would work. The DFG/FTL operations don't actually initialize the object while the ObjectInitializationScope would be alive. Are you proposing that there should be a new method to drop the initialization scope? >> Source/JavaScriptCore/runtime/JSObject.cpp:1185 >> + > > Please remove whitespace. Fixed.
Comment on attachment 342873 [details] Patch I would rip out the part of this patch that adds more uses of ObjectInitializationScope. In RegExpMatchesArray, we already use GCDeferralContext, which may not interact great with the DisallowGC that ObjectInitializationScope does.
Created attachment 342959 [details] Patch
Attachment 342959 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1181: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342959 [details] Patch Attachment 342959 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8235221 Number of test failures exceeded the failure limit.
Created attachment 342967 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 342972 [details] Patch
Attachment 342972 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1181: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342981 [details] Patch
Attachment 342981 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:168: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1181: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 342983 [details] Patch
Attachment 342983 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:167: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:167: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1181: Declaration has space between type name and * in propertyCapacity * sizeof [whitespace/declaration] [3] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 342983 [details] Patch Clearing flags on attachment: 342983 Committed r232951: <https://trac.webkit.org/changeset/232951>
All reviewed patches have been landed. Closing bug.
Comment on attachment 342983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342983&action=review > Source/JavaScriptCore/runtime/JSArray.cpp:55 > + ASSERT_UNUSED(globalObject, globalObject->isOriginalArrayStructure(structure) || structure == globalObject->regExpMatchesArrayStructure() || structure == globalObject->regExpMatchesArrayWithGroupsStructure()); This is crashing for me on a debug build
(In reply to Saam Barati from comment #38) > Comment on attachment 342983 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342983&action=review > > > Source/JavaScriptCore/runtime/JSArray.cpp:55 > > + ASSERT_UNUSED(globalObject, globalObject->isOriginalArrayStructure(structure) || structure == globalObject->regExpMatchesArrayStructure() || structure == globalObject->regExpMatchesArrayWithGroupsStructure()); > > This is crashing for me on a debug build repro test case: ``` class A extends Array { } new A; ```
Comment on attachment 342983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342983&action=review >>> Source/JavaScriptCore/runtime/JSArray.cpp:55 >>> + ASSERT_UNUSED(globalObject, globalObject->isOriginalArrayStructure(structure) || structure == globalObject->regExpMatchesArrayStructure() || structure == globalObject->regExpMatchesArrayWithGroupsStructure()); >> >> This is crashing for me on a debug build > > repro test case: > > ``` > class A extends Array { } > new A; > ``` Yeah, I can see how this would break for that... I should just get rid of that assert. It doesn't do very much.