Bug 158059 - Static table lookup should not require getOwnPropertySlot override.
Summary: Static table lookup should not require getOwnPropertySlot override.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-24 23:10 PDT by Gavin Barraclough
Modified: 2016-05-27 11:29 PDT (History)
9 users (show)

See Also:


Attachments
untested WIP (60.64 KB, patch)
2016-05-24 23:11 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
untested WIP (60.56 KB, patch)
2016-05-24 23:15 PDT, Gavin Barraclough
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (406.45 KB, application/zip)
2016-05-24 23:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (365.48 KB, application/zip)
2016-05-25 00:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (383.73 KB, application/zip)
2016-05-25 00:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (199.66 KB, application/zip)
2016-05-25 00:02 PDT, Build Bot
no flags Details
Fix (63.23 KB, patch)
2016-05-25 12:56 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Missed One! (63.58 KB, patch)
2016-05-25 13:32 PDT, Gavin Barraclough
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (814.21 KB, application/zip)
2016-05-25 14:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.01 MB, application/zip)
2016-05-25 14:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (693.95 KB, application/zip)
2016-05-25 14:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.44 MB, application/zip)
2016-05-25 14:50 PDT, Build Bot
no flags Details
Skip GlobalObject (61.97 KB, patch)
2016-05-25 14:56 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix (74.34 KB, patch)
2016-05-25 23:47 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2016-05-24 23:10:06 PDT
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).
Comment 1 Gavin Barraclough 2016-05-24 23:11:18 PDT
Created attachment 279739 [details]
untested WIP
Comment 2 Gavin Barraclough 2016-05-24 23:15:12 PDT
Created attachment 279740 [details]
untested WIP
Comment 3 Build Bot 2016-05-24 23:59:23 PDT
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.
Comment 4 Build Bot 2016-05-24 23:59:26 PDT
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
Comment 5 Build Bot 2016-05-25 00:00:41 PDT
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.
Comment 6 Build Bot 2016-05-25 00:00:44 PDT
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
Comment 7 Build Bot 2016-05-25 00:01:58 PDT
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.
Comment 8 Build Bot 2016-05-25 00:02:01 PDT
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
Comment 9 Build Bot 2016-05-25 00:02:45 PDT
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.
Comment 10 Build Bot 2016-05-25 00:02:48 PDT
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
Comment 11 Gavin Barraclough 2016-05-25 12:56:13 PDT
Created attachment 279794 [details]
Fix
Comment 12 Gavin Barraclough 2016-05-25 13:32:45 PDT
Created attachment 279800 [details]
Missed One!
Comment 13 Build Bot 2016-05-25 14:31:48 PDT
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
Comment 14 Build Bot 2016-05-25 14:31:51 PDT
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
Comment 15 Build Bot 2016-05-25 14:36:00 PDT
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
Comment 16 Build Bot 2016-05-25 14:36:03 PDT
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
Comment 17 Build Bot 2016-05-25 14:37:36 PDT
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
Comment 18 Build Bot 2016-05-25 14:37:40 PDT
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
Comment 19 Build Bot 2016-05-25 14:50:12 PDT
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
Comment 20 Build Bot 2016-05-25 14:50:17 PDT
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
Comment 21 Gavin Barraclough 2016-05-25 14:56:24 PDT
Created attachment 279825 [details]
Skip GlobalObject
Comment 22 Gavin Barraclough 2016-05-25 23:47:28 PDT
Created attachment 279869 [details]
Fix
Comment 23 Darin Adler 2016-05-26 09:29:43 PDT
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?
Comment 24 Gavin Barraclough 2016-05-27 00:01:43 PDT
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.
Comment 25 Gavin Barraclough 2016-05-27 00:23:30 PDT
Committed revision 201448.
Comment 26 Darin Adler 2016-05-27 11:29:19 PDT
(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!