Bug 154257 - JSDOMWindow::getOwnPropertySlot should just call getStaticPropertySlot
Summary: JSDOMWindow::getOwnPropertySlot should just call getStaticPropertySlot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks: 157662
  Show dependency treegraph
 
Reported: 2016-02-15 14:06 PST by Gavin Barraclough
Modified: 2016-05-12 20:57 PDT (History)
8 users (show)

See Also:


Attachments
Fix (5.51 KB, patch)
2016-02-15 14:25 PST, Gavin Barraclough
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
Fix (9.16 KB, patch)
2016-02-16 13:30 PST, Gavin Barraclough
cdumez: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details
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 Details
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 Details
Fix (10.93 KB, patch)
2016-02-16 16:22 PST, Gavin Barraclough
cdumez: review+
Details | Formatted Diff | Diff
Patch for EWS (already reviewed, retesting post bug#154304) (7.63 KB, patch)
2016-02-16 17:43 PST, Gavin Barraclough
no flags 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-02-15 14:06:26 PST
It currently reimplements this.
Comment 1 Gavin Barraclough 2016-02-15 14:25:49 PST
Created attachment 271373 [details]
Fix
Comment 2 Chris Dumez 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*
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Chris Dumez 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.
Comment 10 Chris Dumez 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).
Comment 11 Gavin Barraclough 2016-02-16 13:30:57 PST
Created attachment 271477 [details]
Fix
Comment 12 Gavin Barraclough 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.
Comment 13 Chris Dumez 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?
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Build Bot 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
Comment 20 Chris Dumez 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
Comment 21 Gavin Barraclough 2016-02-16 16:22:13 PST
Created attachment 271505 [details]
Fix
Comment 22 Chris Dumez 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.
Comment 23 Chris Dumez 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.
Comment 24 Gavin Barraclough 2016-02-16 17:43:18 PST
Created attachment 271513 [details]
Patch for EWS (already reviewed, retesting post bug#154304)
Comment 25 Chris Dumez 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>
Comment 26 Chris Dumez 2016-02-16 18:34:41 PST
All reviewed patches have been landed.  Closing bug.