JSDOMWindow::getOwnPropertySlot should just call getStaticPropertySlot

• Component: Bindings
• Status: RESOLVED FIXED

[Gavin Barraclough]

It currently reimplements this.

[Gavin Barraclough]

Created attachment 271373 [details]
Fix

[Chris Dumez]

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 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]

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 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]

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 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]

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]

(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]

(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]

Created attachment 271477 [details]
Fix

[Gavin Barraclough]

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 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 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]

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 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]

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 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]

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 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]

Created attachment 271505 [details]
Fix

[Chris Dumez]

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 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]

Created attachment 271513 [details]
Patch for EWS (already reviewed, retesting post bug#154304)

[Chris Dumez]

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]

All reviewed patches have been landed.  Closing bug.