WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Gavin Barraclough
Comment 1
2016-02-15 14:25:49 PST
Created
attachment 271373
[details]
Fix
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
Created
attachment 271477
[details]
Fix
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
Created
attachment 271505
[details]
Fix
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.
Top of Page
Format For Printing
XML
Clone This Bug