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

@yutakahirano yutakahirano commented Sep 17, 2020

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

@yutakahirano yutakahirano commented Sep 17, 2020

@annevk: Can you take a look?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 Sep 17, 2020 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 Sep 18, 2020 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25597 Sep 18, 2020 Inactive
Copy link
Member

@annevk annevk left a comment

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.

annevk
annevk approved these changes Sep 18, 2020
@domenic
Copy link
Member

@domenic 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 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 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 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

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
21 checks passed
@yutakahirano yutakahirano deleted the yhirano/document.domain branch Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants