| Summary: | Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||||
| Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | darin, eric, haraken, japhet, sam, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | 93382, 93578 | ||||||||||||||
| Bug Blocks: | 75793, 95652 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Adam Barth
2012-08-07 16:10:47 PDT
Created attachment 157034 [details]
work in progress
Comment on attachment 157034 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=157034&action=review > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:204 > + return checkThatThing(activeDOMWindow(exec), impl()); Note: This implementation isn't quite right. We're supposed to indirect though the WindowShell here. See line 68 that was removed from JSDOMWindowCustom.h. Created attachment 157050 [details]
Patch
Created attachment 157370 [details]
Patch
Comment on attachment 157370 [details]
Patch
LGTM.
Comment on attachment 157370 [details] Patch Clearing flags on attachment: 157370 Committed r125146: <http://trac.webkit.org/changeset/125146> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 93578 Created attachment 159573 [details]
Rebase to trunk
Now that we've gotten more of Bug 75793 landed, we can write this match more cleanly. The changes in test results make these tests behave more similarly to the way they behave in V8. They're still not quite identical, but at least we only printing one error message instead of two. Created attachment 159577 [details]
Patch
Comment on attachment 159577 [details]
Patch
Nice!
Comment on attachment 159577 [details] Patch Attachment 159577 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13544574 Looks like a bug in EFL's dependency tracking. Comment on attachment 159577 [details] Patch Clearing flags on attachment: 159577 Committed r126165: <http://trac.webkit.org/changeset/126165> All reviewed patches have been landed. Closing bug. Comment on attachment 159577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159577&action=review > Source/WebCore/bindings/js/JSDOMBinding.cpp:236 > + bool result = BindingSecurity::shouldAllowAccessToDOMWindow(exec, target, DoNotReportSecurityError); > + // FIXME: The following line of code should move somewhere that it can be shared with immediatelyReportUnsafeAccessTo. > + message = target->crossDomainAccessErrorMessage(activeDOMWindow(exec)); Is the unconditional creation of the message intentional? This thing turns out to be a substantial performance regression, which can be entirely undone if I only create the message when result is false. Comment on attachment 159577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159577&action=review > Source/WebCore/bindings/js/JSDOMWindowCustom.h:-65 > - message = crossDomainAccessErrorMessage(exec->lexicalGlobalObject()); > - return false; You're right. We used to only create the error message when returning false. Sorry for missing that! Yes. Would you like to work the patch, or should I? work -> write |