Bug 186692 – Properly zero unused property storage offsets

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Properly zero unused property storage offsets

[Keith Miller]

Created attachment 342843 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 342854 [details]
Patch

[EWS Watchlist]

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.

[Geoffrey Garen]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Keith Miller]

rdar://problem/39489288

[Saam Barati]

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?

[Keith Miller]

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.

[Keith Miller]

Created attachment 342872 [details]
Patch

[EWS Watchlist]

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.

[Keith Miller]

Created attachment 342873 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Mark Lam]

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.

[Keith Miller]

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.

[Filip Pizlo]

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.

[Keith Miller]

Created attachment 342959 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 342972 [details]
Patch

[EWS Watchlist]

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.

[Keith Miller]

Created attachment 342981 [details]
Patch

[EWS Watchlist]

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.

[Keith Miller]

Created attachment 342983 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 342983 [details]
Patch

Clearing flags on attachment: 342983

Committed r232951: <https://trac.webkit.org/changeset/232951>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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

[Saam Barati]

(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;
```

[Keith Miller]

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.