Bug 158059 – Static table lookup should not require getOwnPropertySlot override.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

JSObject is inconsistent as to when it consults static tables, and getOwnPropertySlot does not do so. This means that all subclasses that use static properties must override getOwnPropertySlot, and all uncached property access is virtual (via JSC method table).

[Gavin Barraclough]

Created attachment 279739 [details]
untested WIP

[Gavin Barraclough]

Created attachment 279740 [details]
untested WIP

[Build Bot]

Comment on attachment 279740 [details]
untested WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279742 [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 on attachment 279740 [details]
untested WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 279743 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Build Bot]

Comment on attachment 279740 [details]
untested WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 279740 [details]
untested WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Gavin Barraclough]

Created attachment 279794 [details]
Fix

[Gavin Barraclough]

Created attachment 279800 [details]
Missed One!

[Build Bot]

Comment on attachment 279800 [details]
Missed One!

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

New failing tests:
http/tests/security/cross-origin-window-property-access.html
fast/dom/beforeload/frame-before-load.html
js/dom/getOwnPropertyDescriptor.html
fast/dom/beforeload/video-before-load.html
webarchive/test-link-rel-subresource-beforeload.html
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html
fast/encoding/parser-tests-50.html
fast/dom/unforgeable-attributes.html
fast/dom/Window/forbid-showModalDialog.html
fast/dom/beforeload/script-before-load.html
fast/dom/beforeload/image-before-load.html

[Build Bot]

Created attachment 279813 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 279800 [details]
Missed One!

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

New failing tests:
http/tests/security/cross-origin-window-property-access.html
fast/dom/beforeload/frame-before-load.html
js/dom/getOwnPropertyDescriptor.html
fast/dom/beforeload/video-before-load.html
webarchive/test-link-rel-subresource-beforeload.html
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html
fast/encoding/parser-tests-50.html
fast/dom/unforgeable-attributes.html
fast/dom/Window/forbid-showModalDialog.html
fast/dom/beforeload/script-before-load.html
fast/dom/beforeload/image-before-load.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 279800 [details]
Missed One!

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

New failing tests:
http/tests/security/cross-origin-window-property-access.html
fast/dom/beforeload/frame-before-load.html
js/dom/getOwnPropertyDescriptor.html
fast/dom/beforeload/video-before-load.html
webarchive/test-link-rel-subresource-beforeload.html
fast/replaced/table-percent-height.html
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html
fast/encoding/parser-tests-50.html
fast/dom/unforgeable-attributes.html
fast/dom/Window/forbid-showModalDialog.html
fast/dom/beforeload/script-before-load.html
fast/dom/beforeload/image-before-load.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 279800 [details]
Missed One!

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

New failing tests:
http/tests/security/cross-origin-window-property-access.html
fast/dom/beforeload/frame-before-load.html
js/dom/getOwnPropertyDescriptor.html
fast/dom/beforeload/video-before-load.html
webarchive/test-link-rel-subresource-beforeload.html
imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html
fast/encoding/parser-tests-50.html
fast/dom/unforgeable-attributes.html
fast/dom/Window/forbid-showModalDialog.html
fast/dom/beforeload/script-before-load.html
fast/dom/beforeload/image-before-load.html

[Build Bot]

Created attachment 279821 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Gavin Barraclough]

Created attachment 279825 [details]
Skip GlobalObject

[Gavin Barraclough]

Created attachment 279869 [details]
Fix

[Darin Adler]

Comment on attachment 279869 [details]
Fix

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

Looks good to me. I don’t see any obvious downside.

> Source/JavaScriptCore/runtime/ArrayConstructor.h:37
> +    static const unsigned StructureFlags = HasStaticPropertyTable | InternalFunction::StructureFlags;

Some stray thoughts around the edges:

- Should we be using constexpr for these instead of const?
- Should we get more consistent about whether we put the new flags before or after the old ones in the "|" expression? I prefer putting the new flags after.
- Should we get more consistent about using Base rather than repeating the base class name? This one repeats the name, but most use Base.
- For Base, should we switch from typedef to using? I like the using style better, even outside of its best use for template types.

> Source/JavaScriptCore/runtime/JSObject.cpp:1679
> +    for (const ClassInfo* info = classInfo(); info; info = info->parentClass) {

This is the kind of code where I think auto* wins out over explicitly naming a class.

> Source/JavaScriptCore/runtime/JSObject.cpp:1680
> +        if (const HashTable* table = info->staticPropHashTable) {

Here too.

> Source/JavaScriptCore/runtime/Lookup.h:211
> +inline bool getStaticPropertySlotFromTable(VM& vm, const HashTable& table, JSObject* thisObj, PropertyName propertyName, PropertySlot& slot)

Could we spell out thisObject instead of using thisObj?

[Gavin Barraclough]

Fixed all review comments.

> Some stray thoughts around the edges:
> 
> - Should we be using constexpr for these instead of const?
> - Should we get more consistent about whether we put the new flags before or
> after the old ones in the "|" expression? I prefer putting the new flags
> after.
> - Should we get more consistent about using Base rather than repeating the
> base class name? This one repeats the name, but most use Base.
> - For Base, should we switch from typedef to using? I like the using style
> better, even outside of its best use for template types.

Re constexpr & using – yes, we probably should.

It seems pretty unfortunate that we have to have these flag declarations at all. The HasStaticPropertyTable flag should be set if and only if the ClassInfo contains a static table, OverridesGetOwnPropertySlot should be set if the class has an implementation of GetOwnPropertySlot differing to JSObject, etc. We'd need a way to specify some custom flags, but I think most classes could stop needing to declare structureFlags at all, with them being generated by a common templates helper function. I haven't worked this idea through yet. I'm not sure that this can be determined as a constexpr, and it might be error prone if any class failed to declare dependencies correctly (i.e. if a helper template relied on Base, and a class failed to define this, you might have a subtle error - as it could pick up the parent's Base).

If computing of these flags could be performed by a common helper, then order & usage of Base would necessarily be consistent.

[Gavin Barraclough]

Committed revision 201448.

[Darin Adler]

(In reply to comment #24)
> The HasStaticPropertyTable flag should be set if and only if the
> ClassInfo contains a static table, OverridesGetOwnPropertySlot should be set
> if the class has an implementation of GetOwnPropertySlot differing to
> JSObject, etc.

Good point. We live in a magical era where template meta-programming can accomplish wonders!