Bug 93407

Summary: Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: 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 Flags
work in progress
none
Patch
none
Patch
none
Rebase to trunk
none
Patch none

Adam Barth
Reported 2012-08-07 16:10:47 PDT
Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity
Attachments
work in progress (10.52 KB, patch)
2012-08-07 16:11 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (9.54 KB, patch)
2012-08-07 17:15 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (22.90 KB, patch)
2012-08-08 19:31 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Rebase to trunk (31.31 KB, patch)
2012-08-20 17:33 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (34.74 KB, patch)
2012-08-20 17:50 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-08-07 16:11:35 PDT
Created attachment 157034 [details] work in progress
Adam Barth
Comment 2 2012-08-07 16:14:11 PDT
This patch depends on the patch in Bug 93382.
Adam Barth
Comment 3 2012-08-07 16:18:38 PDT
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.
Adam Barth
Comment 4 2012-08-07 17:15:36 PDT
Created attachment 157050 [details] Patch
Adam Barth
Comment 5 2012-08-08 19:31:26 PDT
Created attachment 157370 [details] Patch
Eric Seidel (no email)
Comment 6 2012-08-08 19:34:13 PDT
Comment on attachment 157370 [details] Patch LGTM.
WebKit Review Bot
Comment 7 2012-08-08 20:52:57 PDT
Comment on attachment 157370 [details] Patch Clearing flags on attachment: 157370 Committed r125146: <http://trac.webkit.org/changeset/125146>
WebKit Review Bot
Comment 8 2012-08-08 20:53:01 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 9 2012-08-08 22:08:51 PDT
Re-opened since this is blocked by 93578
Adam Barth
Comment 10 2012-08-20 17:33:19 PDT
Created attachment 159573 [details] Rebase to trunk
Adam Barth
Comment 11 2012-08-20 17:35:01 PDT
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.
Adam Barth
Comment 12 2012-08-20 17:50:05 PDT
Created attachment 159577 [details] Patch
Eric Seidel (no email)
Comment 13 2012-08-20 18:05:27 PDT
Comment on attachment 159577 [details] Patch Nice!
Gyuyoung Kim
Comment 14 2012-08-20 19:54:27 PDT
Comment on attachment 159577 [details] Patch Attachment 159577 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13544574
Adam Barth
Comment 15 2012-08-20 23:05:59 PDT
Looks like a bug in EFL's dependency tracking.
Adam Barth
Comment 16 2012-08-21 09:53:53 PDT
Comment on attachment 159577 [details] Patch Clearing flags on attachment: 159577 Committed r126165: <http://trac.webkit.org/changeset/126165>
Adam Barth
Comment 17 2012-08-21 09:53:56 PDT
All reviewed patches have been landed. Closing bug.
Filip Pizlo
Comment 18 2012-09-02 00:47:01 PDT
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.
Adam Barth
Comment 19 2012-09-02 00:55:59 PDT
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?
Adam Barth
Comment 20 2012-09-02 00:56:22 PDT
work -> write
Note You need to log in before you can comment on or make changes to this bug.