Bug 93407 – Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Implement JSDOMWindow*::allowsAccessFrom* in terms of BindingSecurity

[Adam Barth]

Created attachment 157034 [details]
work in progress

[Adam Barth]

This patch depends on the patch in Bug 93382.

[Adam Barth]

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]

Created attachment 157050 [details]
Patch

[Adam Barth]

Created attachment 157370 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 157370 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 157370 [details]
Patch

Clearing flags on attachment: 157370

Committed r125146: <http://trac.webkit.org/changeset/125146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 93578

[Adam Barth]

Created attachment 159573 [details]
Rebase to trunk

[Adam Barth]

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]

Created attachment 159577 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 159577 [details]
Patch

Nice!

[Gyuyoung Kim]

Comment on attachment 159577 [details]
Patch

Attachment 159577 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13544574

[Adam Barth]

Looks like a bug in EFL's dependency tracking.

[Adam Barth]

Comment on attachment 159577 [details]
Patch

Clearing flags on attachment: 159577

Committed r126165: <http://trac.webkit.org/changeset/126165>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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 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]

work -> write