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

An iframe element's name and its contained child browsing context's name interact improperly #25057

Closed
pshaughn opened this issue Dec 3, 2019 · 4 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 3, 2019

This is the iframe case of html/browsers/windows/nested-browsing-contexts/name-attribute.window.html (#25056 for other cases). iframe.setAttribute("name","meh") causes code in the iframe to see window.name as "meh". WPT expects the name that the iframe originally had to be visible as window,name, but for subsequent iframe.setAttributes by the parent to not change window.name. Firefox fails this in the same way; other browsers pass.

@jdm jdm added the A-content/dom label Dec 3, 2019
@jdm jdm added this to To do in web-platform-test failures via automation Dec 3, 2019
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 20, 2020

In an unmerged branch where the window named getter is generally able to return browsing contexts (based on #21869 but with the iframe TODOs filled in), html/browsers/windows/the-window-object/window-named-properties.html fails cases "Dynamic name" and "Ghost name" and it seems like another manifestation of the same cause as the above problem.

@pshaughn pshaughn changed the title Setting an iframe element's name causes the child global's .name to change, WPT considers this incorrect An iframe element's name and its contained child browsing context's name interact improperly Jan 20, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 20, 2020

Despite the test name, html/browsers/the-window-object/named-access-on-the-window-object/window-null-names.html is actually failing for reasons relating to this (in the aforementioned branch). Changing it to use another character instead of the null-byte doesn't make it pass; the problem is about the iframe and its child browsing context not having the right name interaction, not about the null byte.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 21, 2020

I'm going to have to put a subset of html/browsers/windows/nested-browsing-contexts/name-attribute.window.html into the _mozilla tests so we can observe it passing or failing; in an automated run, the test harness times out on the embed and object cases so we don't get to see what happens to the iframe cases.

bors-servo added a commit that referenced this issue Jan 22, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access

<!-- Please describe your changes on the following line: -->
All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method.

Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer.

A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements.

I also made a _mozilla copy of the working part of an existing but not-working test, as I explain in #25057 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25570 and make progress on #25057

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 22, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access

<!-- Please describe your changes on the following line: -->
All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method.

Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer.

A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements.

I also made a _mozilla copy of the working part of an existing but not-working test, as I explain in #25057 (comment)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25570 and make progress on #25057

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 12, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access

<!-- Please describe your changes on the following line: -->
All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method.

Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer.

A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25570 and make progress on #25057

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 13, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access

<!-- Please describe your changes on the following line: -->
All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method.

Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer.

A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25570 and make progress on #25057

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Feb 13, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access

<!-- Please describe your changes on the following line: -->
All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method.

Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer.

A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25570 and make progress on #25057

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 15, 2020

#25572 fixed this.

@pshaughn pshaughn closed this Feb 15, 2020
web-platform-test failures automation moved this from To do to Done Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.