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

Assign a name to iframes when loading the initial about:blank #21976

Merged
merged 3 commits into from Oct 29, 2018

Conversation

Projects
None yet
6 participants
@notriddle
Contributor

notriddle commented Oct 17, 2018

Before, it would assign the name too late, causing scripts (which will not wait for another tick) to accidentally spawn pop-up windows instead of loading into the iframe.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21886
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Oct 17, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/htmliframeelement.rs
  • @KiChjang: components/script/dom/htmliframeelement.rs
@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 17, 2018

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 17, 2018

Warning: the test case in this PR doesn't pass yet

This PR allows slither.io to be opened and played.

However, due to a second bug in the iframe implementation, the load event handler which performs the assertion is being called too early. It should be called only when the form is finished, but is instead being called by the initial about:blank document.

@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 17, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

1 similar comment
@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 17, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

Assign a name to iframes when loading the initial about:blank
Before, it would assign the name too late,
causing scripts (which will not wait for another tick)
to accidentally spawn pop-up windows instead of loading
into the iframe.
@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 19, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 19, 2018

WIP marker removed -- ready to review

Turns out I was misunderstanding a few things, and the Servo behavior is within the documented behavior of the spec (it's Firefox that's being weird, by synchronously firing events). The mute load event flag wouldn't be set even if it was implemented, because the about:blank load event task would be run before the form plan-navigate task would.

So, fixed the test to ignore the blank document.

  • At documentElement.appendChild(frame), the task queue will contain [ iframe_load_event ]
  • Then, at form.submit(), the task queue will contain [ iframe_load_event, form_plan_navigate ]
  • Then, when the end of the script is reached, it will first run iframe_load_event: notice that the mute load event flag is not set. The task queue now contains [ form_plan_navigate ], and my callback already got called once.
  • Then, it pops off the next task, finally running form_plan_navigate, which now sets the mute load event flag (after it has already been fired), and eventually leads to my callback being called a second time.
@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 19, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 19, 2018

⌛️ Trying commit eafcd91 with merge 5dbf83a...

bors-servo added a commit that referenced this pull request Oct 19, 2018

Auto merge of #21976 - notriddle:iframe-target-form-race, r=<try>
Assign a name to iframes when loading the initial about:blank

Before, it would assign the name too late, causing scripts (which will not wait for another tick) to accidentally spawn pop-up windows instead of loading into the iframe.

---
<!-- 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 #21886
- [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/21976)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 19, 2018

💔 Test failed - linux-rel-wpt

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 19, 2018

{"status": "TIMEOUT", "group": "default", "message": "Test timed out", "stack": null, "subtest": "Targeting nested browsing contexts", "test": "/html/browsers/windows/targeting-cross-origin-nested-browsing-contexts.html", "line": 170625, "action": "test_result", "expected": "FAIL"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/browsers/windows/targeting-cross-origin-nested-browsing-contexts.html", "line": 170626, "action": "test_result", "expected": "OK"}
Skip pop-up centric WPT test
It was already broken, since Servo doesn't support pop-up windows.
The iframe changes just made it broken in a different way.
@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 23, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 23, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

⌛️ Trying commit 7270333 with merge ff8cadf...

bors-servo added a commit that referenced this pull request Oct 23, 2018

Auto merge of #21976 - notriddle:iframe-target-form-race, r=<try>
Assign a name to iframes when loading the initial about:blank

Before, it would assign the name too late, causing scripts (which will not wait for another tick) to accidentally spawn pop-up windows instead of loading into the iframe.

---
<!-- 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 #21886
- [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/21976)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@jdm

Thanks for looking into this so carefully!

<script src="/resources/testharnessreport.js"></script>
<script>
async_test(function(t) {
window.addEventListener("load", function() {

This comment has been minimized.

@jdm

jdm Oct 29, 2018

Member

This should pass t.step_func(function() { instead.

form.appendChild(input);
document.documentElement.appendChild(form);
form.submit();
frame.addEventListener("load", function() {

This comment has been minimized.

@jdm

jdm Oct 29, 2018

Member

This should also pass t.step_func(function() {

document.documentElement.appendChild(form);
form.submit();
frame.addEventListener("load", function() {
if (frame.contentWindow.location.href != "about:blank") {

This comment has been minimized.

@jdm

jdm Oct 29, 2018

Member

Could we make this check for the actual expected value instead? Maybe using new URL("form-target-iframe-helper.py", window.location)?

@jdm jdm assigned jdm and unassigned SimonSapin Oct 29, 2018

@jdm jdm removed the S-awaiting-review label Oct 29, 2018

@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 29, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13585.

@jdm

This comment has been minimized.

Member

jdm commented Oct 29, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

📌 Commit 10442ae has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

⌛️ Testing commit 10442ae with merge 2f8dc65...

bors-servo added a commit that referenced this pull request Oct 29, 2018

Auto merge of #21976 - notriddle:iframe-target-form-race, r=jdm
Assign a name to iframes when loading the initial about:blank

Before, it would assign the name too late, causing scripts (which will not wait for another tick) to accidentally spawn pop-up windows instead of loading into the iframe.

---
<!-- 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 #21886
- [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/21976)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

💔 Test failed - linux-rel-wpt

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 29, 2018

@bors-servo retry

Out-of-memory

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 29, 2018

@bors-servo

This comment has been minimized.

@bors-servo bors-servo merged commit 10442ae into servo:master Oct 29, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@notriddle notriddle deleted the notriddle:iframe-target-form-race branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment