RESOLVED FIXED 154257
JSDOMWindow::getOwnPropertySlot should just call getStaticPropertySlot
https://bugs.webkit.org/show_bug.cgi?id=154257
Summary JSDOMWindow::getOwnPropertySlot should just call getStaticPropertySlot
Gavin Barraclough
Reported 2016-02-15 14:06:26 PST
It currently reimplements this.
Attachments
Fix (5.51 KB, patch)
2016-02-15 14:25 PST, Gavin Barraclough
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.06 MB, application/zip)
2016-02-15 15:17 PST, Build Bot
no flags
Archive of layout-test-results from ews100 for mac-yosemite (753.66 KB, application/zip)
2016-02-15 15:22 PST, Build Bot
no flags
Archive of layout-test-results from ews116 for mac-yosemite (818.07 KB, application/zip)
2016-02-15 15:29 PST, Build Bot
no flags
Fix (9.16 KB, patch)
2016-02-16 13:30 PST, Gavin Barraclough
cdumez: review-
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-yosemite (1.48 MB, application/zip)
2016-02-16 14:24 PST, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-02-16 14:32 PST, Build Bot
no flags
Archive of layout-test-results from ews115 for mac-yosemite (920.68 KB, application/zip)
2016-02-16 14:39 PST, Build Bot
no flags
Fix (10.93 KB, patch)
2016-02-16 16:22 PST, Gavin Barraclough
cdumez: review+
Patch for EWS (already reviewed, retesting post bug#154304) (7.63 KB, patch)
2016-02-16 17:43 PST, Gavin Barraclough
no flags
Gavin Barraclough
Comment 1 2016-02-15 14:25:49 PST
Chris Dumez
Comment 2 2016-02-15 14:56:27 PST
Comment on attachment 271373 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271373&action=review EWS reports a failure as well. > Source/JavaScriptCore/runtime/Lookup.h:219 > + const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:249 > const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:269 > + const HashTableValue* entry = table.entry(propertyName); auto*
Build Bot
Comment 3 2016-02-15 15:17:22 PST
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/837196 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Build Bot
Comment 4 2016-02-15 15:17:24 PST
Created attachment 271378 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 5 2016-02-15 15:22:01 PST
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/837221 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Build Bot
Comment 6 2016-02-15 15:22:04 PST
Created attachment 271379 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 7 2016-02-15 15:29:50 PST
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/837223 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Build Bot
Comment 8 2016-02-15 15:29:52 PST
Created attachment 271380 [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
Chris Dumez
Comment 9 2016-02-15 20:54:12 PST
(In reply to comment #7) > Comment on attachment 271373 [details] > Fix > > Attachment 271373 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/837223 > > New failing tests: > js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html Note that JSNavigator::getOwnPropertySlot() uses getStaticValueSlot<JSNavigator, Base>() internally and that all its attributes are on the instance. You updated getStaticPropertySlot() to search for overrides before checking the property table. Also note that Navigator.geolocation is (wrongly but for compatibility reasons) marked as replaceable so it is not really readonly. I think the reason you don't see the deprecation warning for the second access is because the setter actually succeeded and navigator.geolocation was actually set to 1. Therefore, on second access we read the override, don't call the native getter and thus don't print the deprecation warning anymore. It should be easy to confirm.
Chris Dumez
Comment 10 2016-02-15 21:05:32 PST
(In reply to comment #9) > (In reply to comment #7) > > Comment on attachment 271373 [details] > > Fix > > > > Attachment 271373 [details] did not pass mac-debug-ews (mac): > > Output: http://webkit-queues.webkit.org/results/837223 > > > > New failing tests: > > js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html > > Note that JSNavigator::getOwnPropertySlot() uses > getStaticValueSlot<JSNavigator, Base>() internally and that all its > attributes are on the instance. You updated getStaticPropertySlot() to > search for overrides before checking the property table. > > Also note that Navigator.geolocation is (wrongly but for compatibility > reasons) marked as replaceable so it is not really readonly. I think the > reason you don't see the deprecation warning for the second access is > because the setter actually succeeded and navigator.geolocation was actually > set to 1. Therefore, on second access we read the override, don't call the > native getter and thus don't print the deprecation warning anymore. It > should be easy to confirm. Note that if I am right though, then it means that https://bugs.webkit.org/show_bug.cgi?id=133559 needs to be fixed some other way. That bug was trying to make sure we did not throw a TypeError when trying to set navigator.geolocation in strict mode by marking the attribute as [Replaceable]. The idea was that marking it as [Replaceable] was a minimal change, stopped the exception throwing and the assignment was still a no-op (due to the bug you fixed in getStaticValueSlot).
Gavin Barraclough
Comment 11 2016-02-16 13:30:57 PST
Gavin Barraclough
Comment 12 2016-02-16 13:32:37 PST
Updated patch, which makes [Replaceable] work correctly. Will work with Chris to determine desired behavior for navigator.geolocation [i.e. whether we still need a hack here] in bug#154304.
Chris Dumez
Comment 13 2016-02-16 13:34:43 PST
Comment on attachment 271477 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271477&action=review r=me > Source/JavaScriptCore/runtime/Lookup.h:219 > + const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:249 > const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:269 > + const HashTableValue* entry = table.entry(propertyName); auto* > LayoutTests/js/dom/script-tests/shadow-navigator-geolocation-in-strict-mode-does-not-throw.js:-15 > -shouldBe("myNavigator.geolocation", "navigator.geolocation"); Maybe a FIXME comment explaining that returning 1 here is actually wrong?
Build Bot
Comment 14 2016-02-16 14:24:33 PST
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/841814 New failing tests: fast/dom/Window/get-set-properties.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Build Bot
Comment 15 2016-02-16 14:24:37 PST
Created attachment 271482 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 16 2016-02-16 14:32:43 PST
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/841832 New failing tests: fast/dom/Window/get-set-properties.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Build Bot
Comment 17 2016-02-16 14:32:46 PST
Created attachment 271485 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 18 2016-02-16 14:39:38 PST
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/841835 New failing tests: fast/dom/Window/get-set-properties.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Build Bot
Comment 19 2016-02-16 14:39:42 PST
Created attachment 271488 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Chris Dumez
Comment 20 2016-02-16 15:55:18 PST
Comment on attachment 271477 [details] Fix The interfaces.html failure seems real: --- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/html/dom/interfaces-actual.txt @@ -3824,11 +3824,11 @@ PASS Window interface: attribute statusbar PASS Window interface: attribute toolbar PASS Window interface: attribute status -PASS Window interface: operation close() +FAIL Window interface: operation close() assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: attribute closed PASS Window interface: operation stop() -PASS Window interface: operation focus() -PASS Window interface: operation blur() +FAIL Window interface: operation focus() assert_equals: property should be writable if and only if not unforgeable expected true but got false +FAIL Window interface: operation blur() assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: attribute frames PASS Window interface: attribute length PASS Window interface: attribute opener @@ -3845,7 +3845,7 @@ PASS Window interface: operation prompt(DOMString,DOMString) PASS Window interface: operation print() PASS Window interface: operation showModalDialog(DOMString,any) -PASS Window interface: operation postMessage(any,DOMString,[object Object]) +FAIL Window interface: operation postMessage(any,DOMString,[object Object]) assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: operation captureEvents() PASS Window interface: operation releaseEvents() PASS Window interface: attribute onabort
Gavin Barraclough
Comment 21 2016-02-16 16:22:13 PST
Chris Dumez
Comment 22 2016-02-16 16:29:47 PST
It might be nice to land https://bugs.webkit.org/show_bug.cgi?id=154304 first so that we don't temporarily allow shadowing Navigator.geolocation.
Chris Dumez
Comment 23 2016-02-16 16:49:22 PST
Comment on attachment 271505 [details] Fix r=me but I think it'd be cleaner to land https://bugs.webkit.org/show_bug.cgi?id=154304 first.
Gavin Barraclough
Comment 24 2016-02-16 17:43:18 PST
Created attachment 271513 [details] Patch for EWS (already reviewed, retesting post bug#154304)
Chris Dumez
Comment 25 2016-02-16 18:34:35 PST
Comment on attachment 271513 [details] Patch for EWS (already reviewed, retesting post bug#154304) Clearing flags on attachment: 271513 Committed r196678: <http://trac.webkit.org/changeset/196678>
Chris Dumez
Comment 26 2016-02-16 18:34:41 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.