Bug 93407 - Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity
Summary: Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 93382 93578
Blocks: 75793 95652
  Show dependency treegraph
 
Reported: 2012-08-07 16:10 PDT by Adam Barth
Modified: 2012-09-02 00:56 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (10.52 KB, patch)
2012-08-07 16:11 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2012-08-07 17:15 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (22.90 KB, patch)
2012-08-08 19:31 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Rebase to trunk (31.31 KB, patch)
2012-08-20 17:33 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (34.74 KB, patch)
2012-08-20 17:50 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-08-07 16:10:47 PDT
Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity
Comment 1 Adam Barth 2012-08-07 16:11:35 PDT
Created attachment 157034 [details]
work in progress
Comment 2 Adam Barth 2012-08-07 16:14:11 PDT
This patch depends on the patch in Bug 93382.
Comment 3 Adam Barth 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.
Comment 4 Adam Barth 2012-08-07 17:15:36 PDT
Created attachment 157050 [details]
Patch
Comment 5 Adam Barth 2012-08-08 19:31:26 PDT
Created attachment 157370 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-08-08 19:34:13 PDT
Comment on attachment 157370 [details]
Patch

LGTM.
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2012-08-08 20:53:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2012-08-08 22:08:51 PDT
Re-opened since this is blocked by 93578
Comment 10 Adam Barth 2012-08-20 17:33:19 PDT
Created attachment 159573 [details]
Rebase to trunk
Comment 11 Adam Barth 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.
Comment 12 Adam Barth 2012-08-20 17:50:05 PDT
Created attachment 159577 [details]
Patch
Comment 13 Eric Seidel (no email) 2012-08-20 18:05:27 PDT
Comment on attachment 159577 [details]
Patch

Nice!
Comment 14 Gyuyoung Kim 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
Comment 15 Adam Barth 2012-08-20 23:05:59 PDT
Looks like a bug in EFL's dependency tracking.
Comment 16 Adam Barth 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>
Comment 17 Adam Barth 2012-08-21 09:53:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Filip Pizlo 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.
Comment 19 Adam Barth 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?
Comment 20 Adam Barth 2012-09-02 00:56:22 PDT
work -> write