Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check document.domain mutation with cross-origin isolated #25597

Merged
merged 4 commits into from Sep 23, 2020

Conversation

yutakahirano
Copy link
Contributor

Check that the mutation is no-op when cross-origin isolated.

Check that the mutation is no-op when cross-origin isolated.
@yutakahirano
Copy link
Contributor Author

@annevk: Can you take a look?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 September 17, 2020 12:28 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 September 18, 2020 04:34 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 September 18, 2020 09:22 Inactive
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other cases that would be interesting to test is what if document.domain threw an exception. There are a number of validation steps that throw, before cross-origin isolation would make it return. It seems somewhat worthwhile testing those conditions too.

I think @domenic has done that somewhere, but I cannot find the test right now.

@domenic
Copy link
Member

domenic commented Sep 18, 2020

Would document.domain actually change here? That is, isn't it already equal to {{host}}, so this test would pass no matter what?

@domenic
Copy link
Member

domenic commented Sep 18, 2020

Quoting from my comment at https://chromium-review.googlesource.com/c/chromium/src/+/2417872/2#message-050d44f47c5fd0a348193f72035a1d5c0bd3824d:

I've posted a CL at https://chromium-review.googlesource.com/c/chromium/src/+/2419144 which is meant to test the situation for origin isolation. It uses a very different test technique, which I know fails with the current origin isolation implementation.

I might be missing something and your test is fine (i.e., it fails before this CL). But if my understanding is correct, and your test passes no matter what, then you might consider a different test style, similar to the one I posted in my CL.

@domenic
Copy link
Member

domenic commented Sep 18, 2020

The other cases that would be interesting to test is what if document.domain threw an exception. There are a number of validation steps that throw, before cross-origin isolation would make it return. It seems somewhat worthwhile testing those conditions too.

In https://chromium-review.googlesource.com/c/chromium/src/+/2419144 I wrote a test of this sort for origin isolation. (None existed before.) I only checked the easy-to-test registrable suffix condition. Happily, that is the last condition, so it's pretty unlikely that implementations would somehow pass that test but mess up the other conditions.

@yutakahirano
Copy link
Contributor Author

yutakahirano commented Sep 23, 2020

Would document.domain actually change here? That is, isn't it already equal to {{host}}, so this test would pass no matter what?

There are two document.domain substitutions; one is in window-domain-failure.https.sub.html and the other is in resources/iframe-domain-failure.sub.html. The added check checks the latter. Please note that resources/iframe-domain-failure.sub.html is loaded as a cross-origin iframe in window-domain-failure.https.sub.html, and the initial document.domain is {{domains[www1]}}.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it; LGTM then. I will add a similar test to https://chromium-review.googlesource.com/c/chromium/src/+/2419144 for origin isolation.

@yutakahirano yutakahirano merged commit 7a7bc32 into master Sep 23, 2020
@yutakahirano yutakahirano deleted the yhirano/document.domain branch September 23, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants