Skip to content

/ servo

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

Jump to bottom

Iframe's contentDocument is incorrect before load event is fired #22503

Closed
jdm opened this issue Dec 19, 2018 · 5 comments
Closed

Iframe's contentDocument is incorrect before load event is fired #22503

jdm opened this issue Dec 19, 2018 · 5 comments
Assignees
@jdm
Labels
A-content/dom C-has-manual-testcase C-needs-test

Comments

@jdm
Copy link
Member

@jdm jdm commented Dec 19, 2018 •
edited

frame.html

<body>
<script>
var i = document.createElement('iframe');
document.body.appendChild(i);
i.contentWindow.goodbye = function() { console.log("failure!") };
window.hello = function(parent) { console.log("got callback"); parent.i.contentWindow.goodbye() };
i.src = "page.html";
</script>

page.html

<script>
window.goodbye = function() { console.log("success!") }
window.parent.hello(window.parent);
</script>

This prints failure! instead of success! when I load frame.html in Servo (a real HTTP server is required for same-origin access between the page and the iframe).

The stored pipeline id for HTMLIFrameElement isn't updated to the new pipeline until the UpdatePipelineId message is received from the constellation, so we're missing out on the expected synchronous behaviour. This is visible in tests like bailout-exception-vs-return-xml.window.js as well, which end up testing about:blank instead of the expected page.
@cbrewster
Copy link
Member

@cbrewster cbrewster commented Dec 19, 2018

Since frame.html and page.html are same-origin, we should be able to take a shortcut in the script thread to update the iframe's active PipelineId. When we create the local window proxy we check if it is inside an iframe, we could update the pipeline id then.
@jdm
Copy link
Member Author

@jdm jdm commented Dec 20, 2018

This also affects various referrer-policy tests that use iframes, preventing the checks for Message.source from succeeding (even when #22499 is implemented).
@jdm jdm self-assigned this Dec 20, 2018
@jdm
Copy link
Member Author

@jdm jdm commented Dec 20, 2018

I'm fixing this as part of #22514.
@atouchet
Copy link
Contributor

@atouchet atouchet commented Jan 8, 2019

@jdm is this fixed now?
@jdm
Copy link
Member Author

@jdm jdm commented Jan 9, 2019

Yes, but I'm going to leave this open to write an automated test that verifies this.
@jdm jdm added the C-needs-test label Jan 9, 2019
@jdm jdm mentioned this issue Feb 6, 2019
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Feb 11, 2019
@bors-servo
Auto merge of #22841 - jdm:iframe-nav-test, r=nox
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9358b6a 
Add test for contentDocument behaviour during parsing

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22503
- [x] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22841)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jdm jdm

Labels
A-content/dom C-has-manual-testcase C-needs-test
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Add test for contentDocument behaviour during parsing
3 participants
@jdm @cbrewster @atouchet
You can’t perform that action at this time.